kubernetes-sigs / cluster-api-provider-openstack

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

⚠️ OpenStackCluster api general cleanup #1930

Closed mdbooth closed 5 months ago

mdbooth commented 6 months ago

Interestingly, this PR introduces no substantial changes to the CRDs. It primarily affects Go marshalling and documentation. The changes are mostly mechanical.

The PR should hopefully be simpler to review by commit.

The first 2 commits are preparatory, and slightly separate to the other commits. They add some new functionality to the pkg/utils/optional types and update the existing PortOpts to use that functionality.

The rest are structed as changes to single fields, or related sets of fields.

/hold

k8s-ci-robot commented 6 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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
netlify[bot] commented 6 months ago

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

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

mdbooth commented 6 months ago

This adds a fix for the externalNetwork issue which was causing CI failures. The issue was CI is emitting the following in the cluster spec:

  spec:
    externalNetwork:
      id: null

The previous logic in ReconcileExternalNetwork correctly treated this as an empty stanza and executed the fallback 'list all external networks' query. The new logic which failed treated this as an explicitly given empty external network filter and executed it, returning all networks including non-external networks. This was an error as there were more than one.

The principal problem here is that we were executing an external network query without the external flag. This is fixed by setting it for both kinds of query (explicit and implicit).

Separately I have also opened https://github.com/kubernetes-sigs/cluster-api-provider-openstack/pull/1932 to fix the weird external network stanza in the CI templates.

EmilienM commented 6 months ago

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

EmilienM commented 6 months ago

I've reviewed the commits one by one as suggested and everything looks good to me. I have a minor comment inline, but I think this PR is good to go. Thanks for this huge work!

/lgtm

huxcrux commented 6 months ago

This looks good /lgtm

mdbooth commented 5 months ago

/hold cancel