⚠️ Move infrastructure namingStrategy field in ClusterClass #12216
Conversation
k8s-ci-robot
added
cncf-cla: yes
sbueringer
force-pushed
the
pr-move-infra-naming-strategy
branch
from
8f17b1f to
bc159d7
Compare
|
/test pull-cluster-api-e2e-main |
sbueringer
force-pushed
the
pr-move-infra-naming-strategy
branch
from
bc159d7 to
b666b41
Compare
|
/test pull-cluster-api-e2e-main |
sbueringer
changed the title
sbueringer
force-pushed
the
pr-move-infra-naming-strategy
branch
from
b666b41 to
afce0bd
Compare
|
/test pull-cluster-api-e2e-main |
| kind: DockerClusterTemplate | ||
| name: quick-start-cluster | ||
| infrastructureNamingStrategy: | ||
| namingStrategy: |
There was a problem hiding this comment.
This API version is v1beta1, so should we keep it ?
There was a problem hiding this comment.
Great catch! Thx! Reverted this diff
Signed-off-by: Stefan Büringer [email protected]
sbueringer
force-pushed
the
pr-move-infra-naming-strategy
branch
from
afce0bd to
a7fb549
Compare
|
/test pull-cluster-api-e2e-main |
| // infrastructureNamingStrategy allows changing the naming pattern used when creating the infrastructure object. | ||
| // +optional | ||
| InfrastructureNamingStrategy *InfrastructureNamingStrategy `json:"infrastructureNamingStrategy,omitempty"` | ||
| Infrastructure InfrastructureClass `json:"infrastructure,omitempty"` |
There was a problem hiding this comment.
I know this should be a pointer, but I chose to be consistent with the controlPlane field below for now.
Added a sub-task to the v1beta2 umbrella issue to follow-up
There was a problem hiding this comment.
We need to make sure that we correct this before we ship.
InfrastructureClass is a struct, and cannot be omitempty'd correctly.
LocalObjectTemplate is inlined and has a required field within.
The required field should not accept "", which means a structured client marshalling this right now creates an invalid object and cannot treat Infrastructure as truly optional
There was a problem hiding this comment.
I agree. I just don't want to create a different set of problems between this field and the ControlPlane field
Because even if we make the Infrastructure + ControlPlane fields a pointer it's still not perfect as the required Ref field in there is a pointer, so we have to change more to fix this all up
There was a problem hiding this comment.
Yep, that's fine, is the umbrella issue tracking what's required and what's optional to be completed before the next release?
There was a problem hiding this comment.
Currently we only flagged must have (P0) and nice to have (P1) for the alpha (in 2-3 weeks)
There was a problem hiding this comment.
My personal goal would be to fix all these marshalling problems if somehow possible once the KAL linter is available.
Independent of that. I don't know if this should be a showstopper for the release, given we have the exact same situation for the existing ControPlane field
There was a problem hiding this comment.
The optionalfields work for KAL I think is pretty close. Might be able to get that here next week, and at least it will highlight where we have marshalling issues. Fixes will still be partially manual/need to be discussed
There was a problem hiding this comment.
If it helps for the current PR. I think the PR doesn't make it worse as we had the same problem on this field before? (probably? :))
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 0b8d0aea0d269c2294f46226a302e6aeedfd7bbe
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini 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
the
approved
|
/lgtm |
6 participants
Signed-off-by: Stefan Büringer [email protected]
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Follow-up to #11671 (comment)
Part of #10852