openshift-online / ocm-cli

CLI for the Red Hat OpenShift Cluster Manager
Apache License 2.0
76 stars 137 forks source link

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

Closed iamkirkbater closed 1 month ago

iamkirkbater commented 3 months ago

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

~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 commented 3 months ago

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 commented 3 months ago

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 commented 1 month ago

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 :)

tylercreller commented 1 month ago

Overall LGTM - one remaining comment