Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Abstracted the different config validation routes #977

Conversation

JulesFaucherre
Copy link
Contributor

@JulesFaucherre JulesFaucherre commented Aug 1, 2023

Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have checked for similar issues and haven't found anything relevant.
  • This is not a security issue (which should be reported here: https://circleci.com/security/)
  • I have read Contribution Guidelines.

Changes

Abstracted the API that compiles config so that it's transparent whether you use the new API route or the GraphQL endpoint

Rationale

The config compilation has recently been moved from the GraphQL to a new API route. At first it was implemented such as if the compilation request failed with a 404, we tried falling back to the legacy GraphQL endpoint because it mostly meant that we were on old Server version that did not have the new API route
Now you have to pass an APIClient to the config compiler upon creation. This client should be obtained with config.NewWithConfig(*settings.Config). This API client is an interface which implementation either request the new API route or the GraphQL
To detect which implementation must be given, a call is made to the new route, on 404 the GraphQL implementation is returned, else the new API route implementation is returned

Screenshots

Nothing to show as it only changes how the code operates internally but do not add any product change

@JulesFaucherre JulesFaucherre requested a review from a team as a code owner August 1, 2023 14:36
@JulesFaucherre JulesFaucherre force-pushed the task/DEVEX-1028/cli-abstract-api-calls-to-allow-better-testing-and-backward-compatibility-CONFIG-ABSTRACTION branch 2 times, most recently from 3b3500d to 103bb43 Compare August 2, 2023 08:04
@JulesFaucherre JulesFaucherre force-pushed the task/DEVEX-1028/cli-abstract-api-calls-to-allow-better-testing-and-backward-compatibility-CONFIG-ABSTRACTION branch from 4547f3b to 62d9432 Compare August 7, 2023 13:21
Fixed the tests that needed to be adapted to this change
@JulesFaucherre JulesFaucherre force-pushed the task/DEVEX-1028/cli-abstract-api-calls-to-allow-better-testing-and-backward-compatibility-CONFIG-ABSTRACTION branch from 62d9432 to 416053e Compare August 7, 2023 13:34
Copy link
Contributor

@loderunner loderunner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very readable code, for a non-trivial problem. Well done! 👏

Minor comments, none should be blocking. I'll let you decide what you want to do with them and resolve the conversations as needed.

Comment on lines 99 to 102
body, err := io.ReadAll(httpResp.Body)
if err != nil {
return httpResp.StatusCode, err
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be returning an error from reading the response body along with the status code? It seems a little strange.

For example, we could have the server respond with a 200 OK, but for some reason, fail to read the body of the response. We'd then return 200 with an error message, unrelated to the HTTP protocol.

The way I see it, we should either return:

  • 2xx HTTP status code - no error
  • 4xx-5xx HTTP status code - HTTP error (from body or from status text)
  • HTTP status code zero value - non-HTTP error

Comment on lines 12 to 14
var (
compilePath = "compile-config-with-defaults"
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one doesn't seem to change. Can we make this a const?

Suggested change
var (
compilePath = "compile-config-with-defaults"
)
const compilePath = "compile-config-with-defaults"

}
}

func detectAPIClientVersion(restClient *rest.Client) (apiClientVersion, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the internals of this function could be explained. In a year's time, it's probable that another developer will face this and wonder why v1 results in a 404. 🙃

Do you think a comment would be justified here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree! Thanks

@JulesFaucherre JulesFaucherre merged commit 5b87507 into develop Aug 8, 2023
@JulesFaucherre JulesFaucherre deleted the task/DEVEX-1028/cli-abstract-api-calls-to-allow-better-testing-and-backward-compatibility-CONFIG-ABSTRACTION branch August 8, 2023 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants