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

Initial refactor to prepare to move some packages to ocm-common #630

Merged
merged 3 commits into from
Aug 19, 2024

Conversation

iamkirkbater
Copy link
Contributor

@iamkirkbater iamkirkbater commented Jul 3, 2024

Some initial refactors in order to prepare to move the pkg/ocm/connection-builder (formerly pkg/ocm) and pkg/config to ocm-common to be reusable between multiple projects.

Key Features

  • Removes the Connection method from pkg/config - this just felt completely out of place in the config loader logic and felt like it overloaded the config package and was out of scope
  • Allows passing a custom logger to the SDK (this is more of a feature for when this moves to ocm-common so that consumers can pass their own logger in so long as it implements the interface expected)
  • Allows overriding the URL via a function in order to keep env var checking out of the connection builder itself - move this to the consumer to define (you can see how this would be used in the cmd/login function - we explicitly want to override the URL with the same value as the config in the case that the env var is set and the order in which the new pkg/ocm/connection.go will check for these things
  • Speaking of pkg/ocm/connection.go - this file is intended to set the defaults for all of this CLI while not changing the flow that's already implemented in most of the cmd/* files.
  • Updated the few cmd/* files that were still modifying the config directly - some are new from when we merged the PR but the login command also needed to be changed to work with the new model.
  • ConnectionBuilder Allows overriding the Agent string - also more important for when this moves to ocm-common so that downstream consumers of ocm-common will be able to define their own agent string - so you'll know if any given request is coming from external tooling rather than the OCM cli itself.

I'd very much like to add some unit tests to this, and will be going on PTO for the next few weeks, so I will be horribly unresponsive, but I just wanted to get a sneak preview of this in now so that any feedback could be collected.

I didn't get a chance today to write any unit tests like I had hoped, so I am removing this from Draft status and will freely allow you to merge this if you want in order to potentially migrate to ocm-common to be used by other repos before I get back from PTO and don't want to keep this all blocked up for no real good reason.

@ciaranRoche
Copy link
Collaborator

Gave this a first pass, and compared to what we have in ROSA CLI, I think this is a good starting point to lift both pkg/config and pkg/ocm to ocm-common and share between both clients.

Thanks for putting this together

@iamkirkbater iamkirkbater marked this pull request as ready for review July 8, 2024 19:53
@iamkirkbater
Copy link
Contributor Author

I didn't get a chance today to write any unit tests like I had hoped, so I am removing this from Draft status and will freely allow you to merge this if you want in order to potentially migrate to ocm-common to be used by other repos before I get back from PTO and don't want to keep this all blocked up for no real good reason.

@iamkirkbater
Copy link
Contributor Author

Looking at the connection-builder package here I'm not sure if it's worth writing tests for, since the logic is relatively basic of just setting and getting values. Let me know if you think otherwise and I'll add them, though. With that said, I think this is ready for review whenever y'all get a chance :)

cmd/ocm/login/cmd.go Outdated Show resolved Hide resolved
@tylercreller
Copy link
Member

Overall LGTM - one remaining comment

Copy link
Collaborator

@ciaranRoche ciaranRoche left a comment

Choose a reason for hiding this comment

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

/lgtm

@ciaranRoche ciaranRoche merged commit ac42b5c into openshift-online:main Aug 19, 2024
4 of 5 checks passed
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