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: Moved the orb validation in a orb API module #968

Conversation

JulesFaucherre
Copy link
Contributor

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

Started implementing an orb API abstraction by moving the orb validation. The interest of this module is that it will handle the different server version and return a client that is matching the Server version used

Rationale

An issue had been opened before to allow users to validate orbs that use private orbs. An ownerId has been added to the GraphQL API to enable this. Because of this change to the API, this created a potential difference between Server customers and standard customers. We handled this potential difference in the API call code directly but decided that we would handle in a better way in the near future. This is the new version of how we handle this backward compatibility.

Previous Jira ticket: https://circleci.atlassian.net/browse/DEVEX-856

Considerations

Created the OrbClient as a lazy-evaluated singleton because it allows it to be imported from anywhere in the code

Screenshots

Not much to show, the behaviour is still the same

@JulesFaucherre JulesFaucherre requested review from a team as code owners July 19, 2023 08:22
Copy link
Contributor

@abdelDriowya abdelDriowya left a comment

Choose a reason for hiding this comment

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

LGTM & TESTED

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.

A lot of comments, but mostly nitpicking. The general idea is great.

However, I feel this implementation could prove to be hard to extend in the future. Without changing too much, I think we can make this future-proof. Let's discuss.

api/orb/client.go Outdated Show resolved Hide resolved
api/orb/client.go Outdated Show resolved Hide resolved
api/orb/deprecated.go Outdated Show resolved Hide resolved
api/orb/client.go Outdated Show resolved Hide resolved
api/orb/deprecated.go Outdated Show resolved Hide resolved
api/orb/deprecated.go Outdated Show resolved Hide resolved
api/orb/latest.go Outdated Show resolved Hide resolved
api/orb/yaml.go Outdated Show resolved Hide resolved
cmd/orb.go Outdated Show resolved Hide resolved
cmd/orb.go Outdated Show resolved Hide resolved
@JulesFaucherre JulesFaucherre force-pushed the task/DEVEX-1028/cli-abstract-api-calls-to-allow-better-testing-and-backward-compatibility-ORB-ABSTRACTION branch from 6b037c0 to e1efa1a Compare July 24, 2023 14:06
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 nice. I'm not sure about the "kinda singleton" pattern. Let's discuss if we want to keep it.

}
}

func detectClientVersion(gql *graphql.Client) (clientVersion, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The internals of this function aren't necessarily clear. It might be worth commenting why "handling owner ID" results in being able to differentiate between v1 and v2. What do you think?

api/orb/client.go Outdated Show resolved Hide resolved
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.

All good. Address the 2nd conversation if you think it's useful, or just go ahead and merge.

@JulesFaucherre JulesFaucherre force-pushed the task/DEVEX-1028/cli-abstract-api-calls-to-allow-better-testing-and-backward-compatibility-ORB-ABSTRACTION branch from bc2b2f3 to d3625e8 Compare August 8, 2023 09:31
@JulesFaucherre JulesFaucherre merged commit 7c6c1ea into develop Aug 8, 2023
@JulesFaucherre JulesFaucherre deleted the task/DEVEX-1028/cli-abstract-api-calls-to-allow-better-testing-and-backward-compatibility-ORB-ABSTRACTION branch August 8, 2023 12:25
@the-fine the-fine mentioned this pull request Aug 10, 2023
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.

3 participants