kubernetes-sigs / cluster-api-provider-openstack

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

⚠️ ImageFilter - add exclusive validation -> pointers #1939

Closed dulek closed 6 months ago

dulek commented 6 months ago

What this PR does / why we need it: 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.

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:

  1. Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

/hold

netlify[bot] commented 6 months ago

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

Name Link
Latest commit dc3959e0dd803c30361cbf6697293db6090dad66
Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/65f2de403df8320009748c6e
Deploy Preview https://deploy-preview-1939--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.

huxcrux commented 6 months ago

/lgtm

k8s-ci-robot commented 6 months ago

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

@mdbooth: I fixed a comment, but I'm still unsure about ImageFilter being optional. Digging into that now.

EmilienM commented 6 months ago

/test pull-cluster-api-provider-openstack-e2e-full-test /lgtm

I think all comments were addressed (nice work!) but I'll let Matt hold cancel.

dulek commented 6 months ago

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.

dulek commented 6 months ago

I added the explicit +required on the ImageFilter. I guess this is good to go, I can expand the filter in the Bastion patch.

dulek commented 6 months ago

/retest

EmilienM commented 6 months ago

/test pull-cluster-api-provider-openstack-e2e-full-test /lgtm

dulek commented 6 months ago

/hold cancel