⚠️ API cleanup of PortOpts #1914
Conversation
k8s-ci-robot
added
do-not-merge/hold
k8s-ci-robot
added
approved
✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
@lentzi90 @EmilienM This is the first API cleanup change I've extracted. The API change itself is fairly simple. Some of the conversion is a bit hairy, which is mostly specific to this PR. The new optional package will become a feature though. This allows us to customise the conversion behaviour for specific fields whilst otherwise retaining the defaults in apimachinery. There will be more like this, but mostly simpler. |
mdbooth
force-pushed
the
portOptsCleanup
branch
2 times, most recently
from
a1437bd
to
5584a47
Compare
|
I really like it! |
k8s-ci-robot
added
the
lgtm
mdbooth
force-pushed
the
portOptsCleanup
branch
from
5584a47
to
6a649fd
Compare
k8s-ci-robot
removed
the
lgtm
|
/lgtm |
k8s-ci-robot
added
the
lgtm
| "k8s.io/apimachinery/pkg/conversion" | ||
| ) | ||
|
|
||
| func Convert_string_To_optional_String(in *string, out *String, _ conversion.Scope) error { |
There was a problem hiding this comment.
Trivial functions but still could be unit tests. We can do that later.
There was a problem hiding this comment.
I had a similar thought. I did actually include test coverage for them: notice I added a few 'empty object' conversion tests. These tests will fail if this method isn't doing its job correctly.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lentzi90, 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 |
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
Internally, all optional fields become pointers. This change has no direct effect on the CRD, but means that unset values and zero values now have different meanings. SecurityGroupFilters is renamed to SecurityGroups for consistency with other filter fields used throughout the API. Note that this change thoroughly confuses conversion-gen. Consequently we explicitly disable the conversion of these fields in v1alpha6 and v1alpha5 and do the conversion entirely manually.
EmilienM
force-pushed
the
portOptsCleanup
branch
from
6a649fd
to
4c01e65
Compare
k8s-ci-robot
removed
the
lgtm
|
/lgtm |
k8s-ci-robot
added
the
lgtm
I'm pretty sure this will have fixed it. Not sure why the test run against your most recent push is wedged. |
|
/test pull-cluster-api-provider-openstack-test |
Internally, all optional fields become pointers. This change has no direct effect on the CRD, but means that unset values and zero values now have different meanings.
SecurityGroupFilters is renamed to SecurityGroups for consistency with other filter fields used throughout the API. Note that this change thoroughly confuses conversion-gen. Consequently we explicitly disable the conversion of these fields in v1alpha6 and v1alpha5 and do the conversion entirely manually.
/hold