⚠️Remove OpenStackMachineSpec.Subnet #1504
Conversation
k8s-ci-robot
added
the
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 settings. |
|
[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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
approved
|
Tests blocked by kubernetes/test-infra#29003 |
k8s-ci-robot
added
the
cncf-cla: yes
|
How do we achieve the same thing that this API offered without the field? |
We don't: the functionality is removed. However, the premise of this change is that the functionality isn't useful. Do we know of anybody using it? |
|
/retest |
|
What will be set as |
I don't think anybody's using it in practise. Even if they are it's an unnecessarily complex way to specify it and it doesn't support dual stack. I doubt we would bother, but if we did I'd prefer to implement a simpler way of achieving it in v1beta1. |
|
/test pull-cluster-api-provider-openstack-e2e-test |
|
Okay, so https://codesearch.opendev.org/?q=access_ip_v4&i=nope&literal=nope&files=.*py&excludeFiles=&repos= tells me that the field has no real logic in Nova besides saving data. It has some in openstacksdk, but I don't think we care too much. /lgtm |
k8s-ci-robot
added
the
lgtm
k8s-ci-robot
added
needs-rebase
k8s-ci-robot
removed
the
needs-rebase
api/v1alpha6/conversion.go
Outdated
| // Convert a source object of type S to dest type D using the provided conversion function. | ||
| // Store the original source object in the dest object's annotations. | ||
| // Also return any previous version of the object stored in the source object's annotations. | ||
| func convertAndRestore[S, D metav1.Object](src S, dst D, previous D, convert func(S, D, conversion.Scope) error) (bool, error) { | ||
| // Restore data from previous conversion except for metadata. | ||
| // We do this before conversion because after convert() src.Annotations | ||
| // will be aliased as dst.Annotations and will therefore be modified by | ||
| // MarshalData below. | ||
| restored, err := utilconversion.UnmarshalData(src, previous) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| func (r *OpenStackCluster) ConvertTo(dstRaw ctrlconversion.Hub) error { | ||
| dst := dstRaw.(*infrav1.OpenStackCluster) | ||
| if err := convert(src, dst, nil); err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| if err := Convert_v1alpha6_OpenStackCluster_To_v1alpha7_OpenStackCluster(r, dst, nil); err != nil { | ||
| return err | ||
| // Store the original source object in the dest object's annotations. | ||
| if err := utilconversion.MarshalData(src, dst); err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| // Manually restore data. | ||
| restored := &infrav1.OpenStackCluster{} | ||
| if ok, err := utilconversion.UnmarshalData(r, restored); err != nil || !ok { | ||
| return restored, nil | ||
| } |
There was a problem hiding this comment.
@lentzi90 This is the change to the conversion functions I was talking about.
With this in place I don't think we will have to ignore any fields in the fuzzer tests because all fields should either be losslessly convertible or restored from the stashed value.
There was a problem hiding this comment.
Interesting! The first use of golang generics I have come across in the wild 😄
There was a problem hiding this comment.
Yeah, I don't know how you would write this without generics. I think it would just have to be boilerplate.
| })) | ||
| } | ||
|
|
||
| // Test that mutation of a converted object survives a subsequent conversion. | ||
| func TestMutation(t *testing.T) { |
There was a problem hiding this comment.
@lentzi90 This is the other test I was talking about which ensures that we don't simply overwrite an object with the stashed version on conversion.
|
@lentzi90 This version is identical to the previous except for the addition of a HOW THIS WORKS section in |
|
@dulek This version just fixes api conversion from the version you previously reviewed. |
|
/lgtm |
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:
If set, OpenStackMachineSpec.Subnet must correspond to the subnet of at least 1 port on the machine. The IP in this subnet will be set as AccessIPv4 on the OpenStack server.
There are two principal problems with this API:
Supporting it adds complexity in code I would like to remove, so I propose removing it entirely.
/hold