Start work on equinix metal provider#1
Conversation
|
/cc @displague |
|
@detiber: GitHub didn't allow me to request PR reviews from the following users: displague. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
7ff7603 to
f4a6ae1
Compare
|
Was this provider implementation based off of another? I notice a lot of the boilerplate stuff seems to be familiar, just wondering if I could diff with the other implementation to review any changes/diffs in the boilerplate |
|
@JoelSpeed It's kind of an amalgamation of openshift/cluster-api-provider-gcp openshift/cluster-api-provider-aws and some bits taken from kubernetes-sigs/cluster-api-provider-packet circa v1alpha1. It wasn't overly clear if there were preferred patterns or scaffolding for openshift machine api providers. I'd be happy to refactor based on top of a particular provider if you feel it would be more aligned with best practices, though. |
|
No worries, I was just thinking if it's close to a previous provider I might be able to do a diff to reduce the review load. Eg compare the boilerplate/structure and make sure it looks correct and in keeping with prior art. I have been rushed this week so haven't had an opportunity to look into it yet, but will try to do so soon, sorry for the delay. |
|
hey @detiber and @displague , i'm just starting to review this and i was going through the basics of building and reviewing the base artifacts. but when i run the the Makefile command looks good to me, so i'm not quite sure what is causing this but i'll keep poking around. edit: |
- API types - basic controller/actuator scaffolding - basic reconciliation of Machine resources
Well that should have been fixed, will chase 🤔 |
|
/verify-owners |
|
with the updated Makefile changes in place i was able to build the binary and run the unit tests. i hit a couple failures in the tests, doing some investigation to make sure i didn't miss something. |
|
@elmiko The tests are likely pretty broken, most of them where copy/paste from another provider. I'm going to start digging into those to fix them up and mock out the api service for the unit style tests. |
ok cool, i'll focus more on reviewing the implementation details for now then. thanks for the heads up! |
|
I'm starting to adapt the installer to take advantage of Metros. It would be great to see that introduced here - I suspect the dependencies will need to be revisited too. |
|
@displague i am not familiar with Metros, do you have a link or something i could catch up on? |
|
Hey @elmiko, is there anything else you'd like to see changed before merging? We were hoping to merge this as is and then follow up with support for Metros later (it's dependent on some changes to the installer). |
|
I realized that the installer, which is using the legacy Packet TF provider, would need to use the Equinix Metal TF provider. The EM provider is not an https://github.com/hashicorp/ provider (as Packet was). I don't believe I can use the EM TF provider until openshift/installer#4837 is merged. The Packet provider will not be getting metro support, so I can't take advantage of metros even if this cluster-api provider included it. |
|
@crawford i have no specific requests at this time, but i haven't been able to test this code deeply. that said, i don't think my slowness here should be a blocker. i would like to have an opportunity to review more before we start packaging this as a supportable platform though. |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
|
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
|
/remove-lifecycle rotten |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
|
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
|
Rotten issues close after 30d of inactivity. Reopen the issue by commenting /close |
|
@openshift-bot: Closed this PR. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Basic bare functionality in place.