✨ KCP: Allow mutation of all fields that should be mutable #9871
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
/cherry-pick release-1.6 |
k8s-ci-robot
added
the
size/S
|
@sbueringer: once the present PR merges, I will cherry-pick it on top of release-1.6 in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/cherry-pick release-1.5 |
|
@sbueringer: once the present PR merges, I will cherry-pick it on top of release-1.5 in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/assign @fabriziopandini |
Signed-off-by: Stefan Büringer [email protected]
sbueringer
force-pushed
the
pr-kcp-mutable-fields
branch
from
06c2468
to
be30b77
Compare
k8s-ci-robot
added
size/M
| @@ -726,12 +718,6 @@ func TestKubeadmControlPlaneValidateUpdate(t *testing.T) { | |||
| before: before, | |||
| kcp: validUpdate, | |||
| }, | |||
| { | |||
| name: "should return error when trying to mutate the kubeadmconfigspec initconfiguration", | |||
There was a problem hiding this comment.
Now all fields in initconfiguration and joinconfiguration are mutable. This test effectively tested before that it wasn't possible to mutate some of the fields
| @@ -912,20 +892,20 @@ func TestKubeadmControlPlaneValidateUpdate(t *testing.T) { | |||
| kcp: featureGates, | |||
| }, | |||
| { | |||
There was a problem hiding this comment.
Happy to make some of these etcd fields immutable again. I just don't understand why it shouldn't be possible to mutate them.
Does anyone have more context? @fabriziopandini @vincepri maybe?
There was a problem hiding this comment.
no specific reason AFAIK. we just started with everything immutable and relaxed things slowly
There was a problem hiding this comment.
I understand that in general we added new mutable fields iteratively. I got confused that we explicitly had unit tests ensuring that these fields are immutable
| @@ -947,12 +927,6 @@ func TestKubeadmControlPlaneValidateUpdate(t *testing.T) { | |||
| before: etcdLocalImageTag, | |||
| kcp: unsetEtcd, | |||
| }, | |||
| { | |||
| name: "should fail when modifying a field that is not the local etcd image metadata", | |||
There was a problem hiding this comment.
This test is duplicate to the one above
fabriziopandini
changed the title
|
/area control-plane changes the PR type so it stands out in the release notes |
k8s-ci-robot
added
area/control-plane
|
LGTM label has been added. Git tree hash: 91fd99343e43152d237849031a93656e83b29552
|
|
/assign @vincepri @chrischdi @enxebre |
|
/approve |
|
[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
|
@sbueringer: new pull request created: #9884 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@sbueringer: new pull request created: #9885 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Signed-off-by: Stefan Büringer [email protected]
What this PR does / why we need it:
I went over the entire KCP struct and audited for fields that can be mutated.
The goal was to avoid further PRs to make more fields mutable only after someone hit a problem
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 #