Fix path for the error when attempting to mutate managedBy #527
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
/cc @danielvegamyhre |
k8s-ci-robot
added
the
size/M
✅ Deploy Preview for kubernetes-sigs-jobset canceled.
|
|
/lgtm /assign @danielvegamyhre @ahg-g We should backport this to 0.5.0. |
k8s-ci-robot
added
the
lgtm
pkg/webhooks/jobset_webhook_test.go
Outdated
| assert.True(t, strings.Contains(err.Error(), tc.want.Error())) | ||
| } else { | ||
| assert.Nil(t, err) | ||
| if diff := cmp.Diff(tc.want, err, cmpopts.IgnoreFields(field.Error{}, "BadValue")); diff != "" { |
There was a problem hiding this comment.
Why do we need to ignore the BadValue field here
There was a problem hiding this comment.
We don't need to ignore, tentatively I ignored it because I got a bigger diff for the test case: "replicated_jobs_are_immutable", and it wasn't asserted before:
- BadValue: string(""),
+ BadValue: []v1alpha2.ReplicatedJob{
+ {
+ Name: "test-jobset-replicated-job-0",
+ Template: v1.JobTemplateSpec{Spec: v1.JobSpec{...}},
+ Replicas: 2,
+ },
+ {
+ Name: "test-jobset-replicated-job-1",
+ Template: v1.JobTemplateSpec{Spec: v1.JobSpec{...}},
+ Replicas: 1,
+ },
+ },I'm happy either way, WDYT?
There was a problem hiding this comment.
The last commit pushed contains validation of the BadValue. PTAL.
There was a problem hiding this comment.
Yeah I think the bad value can be helpful to see what value was mutated in the immutable construct, so it's easier to find where the mutation is coming from in the code and fix it.
mimowo
force-pushed
the
fix-managed-by-message
branch
from
5f843af
to
263ef24
Compare
k8s-ci-robot
removed
the
lgtm
| @@ -228,7 +228,7 @@ func (j *jobSetWebhook) ValidateUpdate(ctx context.Context, old, newObj runtime. | |||
| } | |||
| // Note that SucccessPolicy and failurePolicy are made immutable via CEL. | |||
| errs := apivalidation.ValidateImmutableField(mungedSpec.ReplicatedJobs, oldJS.Spec.ReplicatedJobs, field.NewPath("spec").Child("replicatedJobs")) | |||
| errs = append(errs, apivalidation.ValidateImmutableField(mungedSpec.ManagedBy, oldJS.Spec.ManagedBy, field.NewPath("spec").Child("labels").Key("managedBy"))...) | |||
| errs = append(errs, apivalidation.ValidateImmutableField(mungedSpec.ManagedBy, oldJS.Spec.ManagedBy, field.NewPath("spec").Child("managedBy"))...) | |||
There was a problem hiding this comment.
We can try cherry-picking this but this PR seems to be two in one.
We have a bug fix (which is useful!) and some test cleanup. I'll see if I can get both into but I would usually suggest leaving cleanups out of a bug fix.
There was a problem hiding this comment.
The test changes are in order to assert on the error path. Previously path was ignored in the test. I think this is a good practice to accompany each change with a test in the same PR.
One thing potentially optional is asserting on the "BadValue", but it raised a question, so I added: #527 (comment)
|
/lgtm Cherry-pick seems fine but just a suggestion for next time. |
k8s-ci-robot
added
the
lgtm
mimowo
changed the title
|
/approve Thanks! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danielvegamyhre, mimowo 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
…pstream-release-0.5 Automated cherry pick of #527: Fix path for the error when mutating managedBy
Fix for the path in error which I discovered wrong during manual e2e testing.