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

🌱 clusterctl: add nutanix ipam & runtime extensions providers #11135

Merged
merged 3 commits into from
Nov 13, 2024

Conversation

jimmidyson
Copy link
Member

  • 🌱 clusterctl: add nutanix ipam provider
  • 🌱 clusterctl: add nutanix runtime extensions provider

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 4, 2024
@k8s-ci-robot k8s-ci-robot added do-not-merge/needs-area PR is missing an area label size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 4, 2024
@jimmidyson jimmidyson force-pushed the jimmi/add-caren-caipamx branch 2 times, most recently from a74ed48 to aca26cf Compare September 4, 2024 16:54
@jimmidyson
Copy link
Member Author

/area clusterctl

@k8s-ci-robot k8s-ci-robot added area/clusterctl Issues or PRs related to clusterctl and removed do-not-merge/needs-area PR is missing an area label labels Sep 4, 2024
@jimmidyson
Copy link
Member Author

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Sep 4, 2024
@jimmidyson
Copy link
Member Author

@elmiko @JoelSpeed if you have time for review 🙏 should be pretty quick and easy hopefully.

@@ -98,13 +98,19 @@ const (
// IPAM providers.
const (
InClusterIPAMProviderName = "in-cluster"
NutanixIPAMProviderName = "nutanix"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
NutanixIPAMProviderName = "nutanix"
NutanixIPAMProviderName = "nutanix-cloud-native-nutanix"

To follow the conventions at https://main.cluster-api.sigs.k8s.io/clusterctl/provider-contract.html#adding-a-provider-to-clusterctl

Copy link
Member Author

Choose a reason for hiding this comment

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

We already have an infra provider simply called nutanix at https://github.com/kubernetes-sigs/cluster-api/blob/main/cmd/clusterctl/client/config/providers_client.go#L55 and it would be great if we could use the same name for these other provider types for consistency 🙏

Copy link
Member

Choose a reason for hiding this comment

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

Good point. Maybe it's fine if a GitHub org already "claimed" the name via another provider

Copy link
Member

Choose a reason for hiding this comment

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

Would make sense. Fine for me if we have an agreement :-)

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I think we can allow this because https://github.com/nutanix is owned by the same org that owns https://github.com/nutanix-cloud-native.

Do you mind updating https://cluster-api.sigs.k8s.io/clusterctl/provider-contract.html#providers-github-org-prefix with something like

In case the entity proposing the new providers owns different github organizations, it is also possible to Provider's GitHub org prefix (in this case it is up to this entity to avoid or address conflicts).

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure we need to update any docs. Can't we just agree as we (Nutanix) already claimed the nutanix provider name for infra provider as mentioned above?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added some docs as requested. Please review when you get a chance 🙏

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 11, 2024
@sbueringer
Copy link
Member

sbueringer commented Oct 31, 2024

I'm fine with allowing to use simply "nutanix" given that the same org already owns the "nutanix" infra provider.
@fabriziopandini Do you agree?

@jimmidyson Is this the only thing blocking this PR (apart from rebase + the nit above)?

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 31, 2024
@jimmidyson
Copy link
Member Author

@sbueringer Yes this is now ready to go! Tested locally and all working perfectly!

@fabriziopandini
Copy link
Member

I'm fine with allowing to use simply "nutanix" given that the same org already owns the "nutanix" infra provider.
@fabriziopandini Do you agree?

commented above

@jimmidyson
Copy link
Member Author

@sbueringer @fabriziopandini Ping 🙏 Would love to get this merged.

@sbueringer
Copy link
Member

Thx!

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 13, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 21addfc95ae91d8a92e0405ff0a90ba65c1553cf

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 13, 2024
@sbueringer
Copy link
Member

Feel free to cherry-pick into release-1.8 if you want (might require a manual cherry-pick)

@jimmidyson
Copy link
Member Author

/cherrypick release-1.8

@k8s-infra-cherrypick-robot

@jimmidyson: once the present PR merges, I will cherry-pick it on top of release-1.8 in a new PR and assign it to you.

In response to this:

/cherrypick release-1.8

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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot merged commit c9654bb into kubernetes-sigs:main Nov 13, 2024
19 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.9 milestone Nov 13, 2024
@k8s-infra-cherrypick-robot

@jimmidyson: #11135 failed to apply on top of branch "release-1.8":

Applying: seedling: clusterctl: add nutanix ipam provider
Using index info to reconstruct a base tree...
M	cmd/clusterctl/client/config/providers_client.go
M	cmd/clusterctl/client/config_test.go
M	cmd/clusterctl/cmd/config_repositories_test.go
A	docs/book/src/developer/providers/contracts/clusterctl.md
M	docs/book/src/reference/glossary.md
M	docs/book/src/reference/providers.md
Falling back to patching base and 3-way merge...
Auto-merging docs/book/src/reference/providers.md
Auto-merging docs/book/src/reference/glossary.md
Auto-merging docs/book/src/clusterctl/provider-contract.md
Auto-merging cmd/clusterctl/cmd/config_repositories_test.go
CONFLICT (content): Merge conflict in cmd/clusterctl/cmd/config_repositories_test.go
Auto-merging cmd/clusterctl/client/config_test.go
Auto-merging cmd/clusterctl/client/config/providers_client.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 seedling: clusterctl: add nutanix ipam provider

In response to this:

/cherrypick release-1.8

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-sigs/prow repository.

phoban01 pushed a commit to phoban01/cluster-api that referenced this pull request Nov 13, 2024
…etes-sigs#11135)

* 🌱 clusterctl: add nutanix ipam provider

* 🌱 clusterctl: add nutanix runtime extensions provider

* docs: Add more info on github org prefix
jimmidyson added a commit to jimmidyson/cluster-api that referenced this pull request Nov 13, 2024
…viders

Backporting of kubernetes-sigs#11135.

* 🌱 clusterctl: add nutanix ipam provider

* 🌱 clusterctl: add nutanix runtime extensions provider

* docs: Add more info on github org prefix
jimmidyson added a commit to jimmidyson/cluster-api that referenced this pull request Nov 13, 2024
…viders

Backporting of kubernetes-sigs#11135.

* 🌱 clusterctl: add nutanix ipam provider

* 🌱 clusterctl: add nutanix runtime extensions provider

* docs: Add more info on github org prefix
@jimmidyson
Copy link
Member Author

Manual backport in #11414

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/clusterctl Issues or PRs related to clusterctl cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants