⚠️ ImageFilter - add exclusive validation -> pointers #1939
Conversation
k8s-ci-robot
added
do-not-merge/hold
✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
dulek
force-pushed
the
filters-pointers
branch
4 times, most recently
from
b94518a
to
f7ccaaa
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
| // The ID of the desired image. If this is provided, the other filters will be ignored. | ||
| ID string `json:"id,omitempty"` | ||
| // The ID of the desired image. If ID is provided, the other filters cannot be provided. | ||
| // +kubebuilder:validation:Format:=uuid |
There was a problem hiding this comment.
Please can you also add a test for uuid format validation?
There was a problem hiding this comment.
There was a problem hiding this comment.
Nice! Please can you add one more test that an invalid uuid is rejected?
There was a problem hiding this comment.
| @@ -43,7 +43,7 @@ type OpenStackMachineSpec struct { | |||
|
|
|||
| // The image to use for your server instance. | |||
| // If the rootVolume is specified, this will be used when creating the root volume. | |||
| Image ImageFilter `json:"image,omitempty"` | |||
| Image ImageFilter `json:"image"` | |||
There was a problem hiding this comment.
Please can you add an explicit // +optional?
There was a problem hiding this comment.
I wasn't exactly sure if the image is optional. Is it? Should I make it Image *ImageFilter then?
There was a problem hiding this comment.
I think we can't make ImageFilter optional, code requires that the exactly one image will be found. An empty ImageFilter might be possible and resolvable if you have just one image in that tenant.
There was a problem hiding this comment.
D'oh. Of course this should not be optional. Yep, lets make it explicitly required.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dulek, 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 |
k8s-ci-robot
added
the
approved
k8s-ci-robot
removed
the
lgtm
|
@mdbooth: I fixed a comment, but I'm still unsure about |
|
/test pull-cluster-api-provider-openstack-e2e-full-test I think all comments were addressed (nice work!) but I'll let Matt hold cancel. |
k8s-ci-robot
added
the
lgtm
|
So we still wanted to add validation that when ID is not set, the filter has at least one other option set. Now I started working on Bastion to fix this, because this means we're unable to create an empty machine, but maybe it's an overkill. Technically an empty filter is still valid if you have just one image. |
This commit changes `ID` and `Name` of `ImageFilter` to pointers which should only affect go marshalling. Other than that it adds CEL validation of the ImageFilter, so that Name or Tags can only be set when ID is unset. Conversions are updated accordingly to make sure we only set Name when ID is unset. Moreover validation is added that ID has to be UUID. It's not enforced in conversions, as non-UUID IDs would produce clusters or machines that would not work properly.
This adds tests related to kubebuilder validations of ImageFilter.
k8s-ci-robot
removed
the
lgtm
|
I added the explicit |
|
/retest |
|
/test pull-cluster-api-provider-openstack-e2e-full-test |
k8s-ci-robot
added
the
lgtm
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
What this PR does / why we need it:
This commit changes
IDandNameofImageFilterto pointers which should only affect go marshalling.Other than that it adds CEL validation of the ImageFilter, so that Name or Tags can only be set when ID is unset. Conversions are updated accordingly to make sure we only set Name when ID is unset.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
TODOs:
/hold