kubernetes-sigs
🌱 Add NamingStrategy to KubeadmControlPlane
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):
Fixes part of #10577
Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all
right now it only has API changes, and the remaining work is in progress, I opened a draft PR so I can get some early inputs. cc @sbueringer
lgtm for the API from my side (+/- the godoc nit)
Before I start to review the implementation, @fabriziopandini @chrischdi @mdbooth @lentzi90 Are we fine with the API and does it cover your use cases?
lgtm for the API from my side (+/- the godoc nit)
Before I start to review the implementation, @fabriziopandini @chrischdi @mdbooth @lentzi90 Are we fine with the API and does it cover your use cases?
@adilGhaffarDev is representing the interests of @lentzi90 while he's away. From my POV, I'm happy if @adilGhaffarDev thinks it covers the use case.
Before I start to review the implementation, @fabriziopandini @chrischdi @mdbooth @lentzi90 Are we fine with the API and does it cover your use cases?
@lentzi90 flag was also for machineset and that is also going to be removed after 1.9, can I add same namingStrategy in MchineDeployments too in this same PR?
@lentzi90 flag was also for machineset and that is also going to be removed after 1.9, can I add same namingStrategy in MchineDeployments too in this same PR?
I will open separate PR for MDs, as discussed here: https://github.com/kubernetes-sigs/cluster-api/issues/10577#issuecomment-2333319671 , @sbueringer please review this one. Also, fuzzing tests are failing in v1alpha APIs, Do you have any idea why they are failing?
@sbueringer please review this one.
I would like to get reviews from other maintainers on the API first. It's not only my call.
LGTM label has been added.
@sbueringer please review this one.
I would like to get reviews from other maintainers on the API first. It's not only my call.
/hold for above
@adilGhaffarDev: The following test failed, say
/retestto rerun all failed tests or/retest-requiredto rerun all mandatory failed tests: Test name Commit Details Required Rerun command pull-cluster-api-test-main 94c96ca link true/test pull-cluster-api-test-mainFull PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.
@sbueringer fuzz tests for v1alpha are failing, what do I need to add to fix these tests?
@sbueringer fuzz tests for v1alpha are failing, what do I need to add to fix these tests?
You have to implement conversion. Basically when these objects are converted e.g. v1beta1 => v1alpha3 => v1beta1 the new field gets lost.
Conversion is implemented in ConvertTo & ConvertFrom functions of v1alpha3 & v1alpha4, e.g.: https://github.com/kubernetes-sigs/cluster-api/blob/4e151e2aa4cebf34efccc6c13410d8b35d1c4a64/internal/apis/controlplane/kubeadm/v1alpha3/conversion.go
To give you an example. For KubeadmControlPlane & v1alpha3 you have to set the new field in ConvertTo somewhere where we are setting (i.e. "restoring") all the other fields, e.g.
dst.Spec.RolloutBefore = restored.Spec.RolloutBefore
@sbueringer fuzz tests for v1alpha are failing, what do I need to add to fix these tests?
You have to implement conversion. Basically when these objects are converted e.g. v1beta1 => v1alpha3 => v1beta1 the new field gets lost.
Conversion is implemented in ConvertTo & ConvertFrom functions of v1alpha3 & v1alpha4, e.g.: https://github.com/kubernetes-sigs/cluster-api/blob/4e151e2aa4cebf34efccc6c13410d8b35d1c4a64/internal/apis/controlplane/kubeadm/v1alpha3/conversion.go
To give you an example. For KubeadmControlPlane & v1alpha3 you have to set the new field in ConvertTo somewhere where we are setting (i.e. "restoring") all the other fields, e.g.
dst.Spec.RolloutBefore = restored.Spec.RolloutBefore
conversions are added. now tests should pass.
@JoelSpeed @sbueringer @chrischdi @vincepri please take a look, all comments are addressed. Let me know if anything else is needed.
Thx!
/lgtm /approve
/hold I'll give other folks a few days time to also take another look, otherwise happy to merge
LGTM label has been added.
LGTM label has been added.
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: sbueringer
The full list of commands accepted by this bot can be found here.
The pull request process is described here
- ~~OWNERS~~ [sbueringer]
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment