search

kubernetes-sigs / cluster-api-provider-openstack

Cluster API implementation for OpenStack
https://cluster-api-openstack.sigs.k8s.io/
Apache License 2.0
292 stars 253 forks source link

⚠️Move CloudName into IdentityRef and make cluster IdentityRef required #1915

Closed mdbooth closed 7 months ago

mdbooth commented 7 months ago

This change came from attempting to write validation markers for CloudName and IdentityRef in both the machine and cluster specs.

Firstly I noticed that IdentityRef was marked optional in the cluster spec, but it is certainly required: the cluster cannot be provisioned without cloud credentials. I made IdentityRef required in the cluster spec.

In contrast, IdentityRef is genuinely optional in the machine spec because, if not specified, we will use the credentials defined in the cluster spec.

CloudName on the machine spec is also marked optional. However, it is required if IdentityRef was specified. This is because it refers to the same object as IdentityRef. The most sensible way to to represent this in the API is to put it in the IdentityRef. This means that if IdentityRef is provided, it must be provided completely, including CloudName.

/hold

netlify[bot] commented 7 months ago

Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Name Link
Latest commit 564b6bd899062bf4fc06ac4a6a10ddfbe7f7a082
Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/65e0965590da6b0008487f05
Deploy Preview https://deploy-preview-1915--kubernetes-sigs-cluster-api-openstack.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

dulek commented 7 months ago

This is so much better design! It looks good to me, but since it already has approved label, I'll leave setting lgtm to the next reviewer.

EmilienM commented 7 months ago

Nicely done, I suspect the API doc wasn't generated for some reason?

EmilienM commented 7 months ago

/lgtm

mdbooth commented 7 months ago

/cc @lentzi90 @mkjpryor @mnaser

k8s-ci-robot commented 7 months ago

@mdbooth: GitHub didn't allow me to request PR reviews from the following users: mkjpryor.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to [this](https://github.com/kubernetes-sigs/cluster-api-provider-openstack/pull/1915#issuecomment-1971462167): >/cc @lentzi90 @mkjpryor @mnaser Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
k8s-ci-robot commented 7 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lentzi90, mdbooth

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: - ~~[OWNERS](https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/main/OWNERS)~~ [lentzi90,mdbooth] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment