kubernetes-sigs / cluster-api-provider-openstack

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

🌱 Deduplicate AdoptMachinePorts and AdoptBastionPorts #1944

Closed mdbooth closed 4 months ago

mdbooth commented 4 months ago

Both of these methods rely on ReferencedMachineResources and DependentMachineResources, so they can be easily refactored to have a common implementation.

Fixes: #1942

This PR is based on #1941, so we should merge that first.

TODO:

/hold

k8s-ci-robot commented 4 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 4 months ago

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

Name Link
Latest commit 750af594e967893ad328897785b91ffc21b0fbeb
Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/65f95c4513caf0000898d7ab
Deploy Preview https://deploy-preview-1944--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 4 months ago

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

EmilienM commented 4 months ago

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

EmilienM commented 4 months ago

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

mdbooth commented 4 months ago

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

mdbooth commented 4 months ago

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

mdbooth commented 4 months ago

/hold cancel

mdbooth commented 4 months ago

https://github.com/kubernetes-sigs/cluster-api-provider-openstack/issues/1954 was the root cause of most of the full-test CI failures in this PR. This PR includes a change, which we needed to make anyway, which avoids the issue in most cases: we don't need to re-reconcile if we only adopted. However, the underlying issue remains. I'll push a separate PR to fix that.

dulek commented 4 months ago

Alright, I only found one issue with a change done in the last commit, when it should be done in the middle one. Let's fix that so that reverts would be possible? Or just squash the commits.

mdbooth commented 4 months ago

Alright, I only found one issue with a change done in the last commit, when it should be done in the middle one. Let's fix that so that reverts would be possible? Or just squash the commits.

Thanks. I'll tidy them up but keep them separate, as I've been trying to make smaller, functional commits. When you look at the next one in this series, though, you'll see how that's going 😟

mdbooth commented 4 months ago

I've cleaned that up and re-run the non-e2e tests against each commit individually. I also rebased on to main to pick up #1955, which is important for this series.

dulek commented 4 months ago

/lgtm