-
Notifications
You must be signed in to change notification settings - Fork 261
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
⚠️Move CloudName into IdentityRef and make cluster IdentityRef required #1915
Conversation
✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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. |
Nicely done, I suspect the API doc wasn't generated for some reason? |
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.
/lgtm |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
/hold cancel
[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:
Approvers can indicate their approval by writing |
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