Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix path for the error when attempting to mutate managedBy #527

Merged
merged 3 commits into from
Apr 19, 2024
Merged

Fix path for the error when attempting to mutate managedBy #527

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
263ef24
Fix path for the error when mutating managedBy
mimowo Apr 18, 2024
070467f
Do not ignore BadValue
mimowo Apr 19, 2024
749cbea
more intuitive namings
mimowo Apr 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/webhooks/jobset_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"))...)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@mimowo mimowo Apr 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

return nil, errs.ToAggregate()
}

Expand Down
49 changes: 44 additions & 5 deletions pkg/webhooks/jobset_webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1051,6 +1051,26 @@ func TestValidateUpdate(t *testing.T) {
},
},
},
{
name: "managedBy is immutable",
js: &jobset.JobSet{
ObjectMeta: validObjectMeta,
Spec: jobset.JobSetSpec{
ManagedBy: ptr.To("example.com/new"),
ReplicatedJobs: validReplicatedJobs,
},
},
oldJs: &jobset.JobSet{
ObjectMeta: validObjectMeta,
Spec: jobset.JobSetSpec{
ManagedBy: ptr.To("example.com/old"),
ReplicatedJobs: validReplicatedJobs,
},
},
want: field.ErrorList{
field.Invalid(field.NewPath("spec").Child("managedBy"), ptr.To("example.com/new"), "field is immutable"),
}.ToAggregate(),
},
{
name: "replicated jobs are immutable",
js: &jobset.JobSet{
Expand Down Expand Up @@ -1085,7 +1105,28 @@ func TestValidateUpdate(t *testing.T) {
ReplicatedJobs: validReplicatedJobs,
},
},
want: fmt.Errorf("field is immutable"),
want: field.ErrorList{
field.Invalid(field.NewPath("spec").Child("replicatedJobs"), []jobset.ReplicatedJob{
{
Name: "test-jobset-replicated-job-0",
Replicas: 2,
Template: batchv1.JobTemplateSpec{
Spec: batchv1.JobSpec{
Parallelism: ptr.To[int32](2),
},
},
},
{
Name: "test-jobset-replicated-job-1",
Replicas: 1,
Template: batchv1.JobTemplateSpec{
Spec: batchv1.JobSpec{
Parallelism: ptr.To[int32](1),
},
},
},
}, "field is immutable"),
}.ToAggregate(),
},
}

Expand All @@ -1097,10 +1138,8 @@ func TestValidateUpdate(t *testing.T) {
newObj := tc.js.DeepCopyObject()
oldObj := tc.oldJs.DeepCopyObject()
_, err = webhook.ValidateUpdate(context.TODO(), oldObj, newObj)
if tc.want != nil {
assert.True(t, strings.Contains(err.Error(), tc.want.Error()))
} else {
assert.Nil(t, err)
if diff := cmp.Diff(tc.want, err); diff != "" {
t.Errorf("ValidateResources() mismatch (-want +got):\n%s", diff)
}
})
}
Expand Down