Merge pull request #534 from kannon92/automated-cherry-pick-of-#527-u…

…pstream-release-0.5 Automated cherry pick of #527: Fix path for the error when mutating managedBy
kubernetes-sigs · Apr 21, 2024 · 3a2ae4d · 3a2ae4d
2 parents cb44e54 + b37189a
commit 3a2ae4d
Show file tree

Hide file tree

Showing 2 changed files with 45 additions and 6 deletions.
diff --git a/pkg/webhooks/jobset_webhook.go b/pkg/webhooks/jobset_webhook.go
@@ -225,7 +225,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"))...)
 	return nil, errs.ToAggregate()
 }
 

diff --git a/pkg/webhooks/jobset_webhook_test.go b/pkg/webhooks/jobset_webhook_test.go
@@ -972,6 +972,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{
@@ -1006,7 +1026,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(),
 		},
 	}
 
@@ -1018,10 +1059,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)
 			}
 		})
 	}