From b9b436a01838b14377ead188e4f3735ab465a8ab Mon Sep 17 00:00:00 2001 From: Dejan Pejchev Date: Wed, 9 Aug 2023 17:09:31 +0200 Subject: [PATCH] cleanup: extract generateSelector from Validate method in job strategy --- pkg/registry/batch/job/strategy.go | 11 +++++++--- pkg/registry/batch/job/strategy_test.go | 28 ++++++++++++------------- 2 files changed, 21 insertions(+), 18 deletions(-) diff --git a/pkg/registry/batch/job/strategy.go b/pkg/registry/batch/job/strategy.go index 71582bed98c..cb4b2b3a5ff 100644 --- a/pkg/registry/batch/job/strategy.go +++ b/pkg/registry/batch/job/strategy.go @@ -92,6 +92,7 @@ func (jobStrategy) GetResetFields() map[fieldpath.APIVersion]*fieldpath.Set { // PrepareForCreate clears the status of a job before creation. func (jobStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) { job := obj.(*batch.Job) + generateSelectorIfNeeded(job) job.Status = batch.JobStatus{} job.Generation = 1 @@ -163,9 +164,6 @@ func (jobStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object // Validate validates a new job. func (jobStrategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList { job := obj.(*batch.Job) - if !*job.Spec.ManualSelector { - generateSelector(job) - } opts := validationOptionsForJob(job, nil) return batchvalidation.ValidateJob(job, opts) } @@ -212,6 +210,13 @@ func (jobStrategy) WarningsOnCreate(ctx context.Context, obj runtime.Object) []s return warnings } +// generateSelectorIfNeeded checks the job's manual selector flag and generates selector labels if the flag is true. +func generateSelectorIfNeeded(obj *batch.Job) { + if !*obj.Spec.ManualSelector { + generateSelector(obj) + } +} + // generateSelector adds a selector to a job and labels to its template // which can be used to uniquely identify the pods created by that job, // if the user has requested this behavior. diff --git a/pkg/registry/batch/job/strategy_test.go b/pkg/registry/batch/job/strategy_test.go index aa7abac20f2..04bd22f1224 100644 --- a/pkg/registry/batch/job/strategy_test.go +++ b/pkg/registry/batch/job/strategy_test.go @@ -1293,10 +1293,10 @@ func TestJobStrategy_Validate(t *testing.T) { theUID := types.UID("1a2b3c4d5e6f7g8h9i0k") validSelector := &metav1.LabelSelector{ - MatchLabels: map[string]string{"a": "b"}, + MatchLabels: map[string]string{"a": "b", batch.LegacyJobNameLabel: "myjob2", batch.JobNameLabel: "myjob2", batch.LegacyControllerUidLabel: string(theUID), batch.ControllerUidLabel: string(theUID)}, } - validLabels := map[string]string{batch.LegacyJobNameLabel: "myjob2", batch.JobNameLabel: "myjob2", batch.LegacyControllerUidLabel: string(theUID), batch.ControllerUidLabel: string(theUID)} labelsWithNonBatch := map[string]string{"a": "b", batch.LegacyJobNameLabel: "myjob2", batch.JobNameLabel: "myjob2", batch.LegacyControllerUidLabel: string(theUID), batch.ControllerUidLabel: string(theUID)} + defaultSelector := &metav1.LabelSelector{MatchLabels: map[string]string{batch.ControllerUidLabel: string(theUID)}} validPodSpec := api.PodSpec{ RestartPolicy: api.RestartPolicyOnFailure, DNSPolicy: api.DNSClusterFirst, @@ -1320,7 +1320,7 @@ func TestJobStrategy_Validate(t *testing.T) { job: &batch.Job{ ObjectMeta: validObjectMeta, Spec: batch.JobSpec{ - Selector: nil, + Selector: defaultSelector, ManualSelector: pointer.Bool(false), Template: api.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ @@ -1332,7 +1332,7 @@ func TestJobStrategy_Validate(t *testing.T) { wantJob: &batch.Job{ ObjectMeta: validObjectMeta, Spec: batch.JobSpec{ - Selector: &metav1.LabelSelector{MatchLabels: map[string]string{batch.ControllerUidLabel: string(theUID)}}, + Selector: defaultSelector, ManualSelector: pointer.Bool(false), Template: api.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ @@ -1346,7 +1346,7 @@ func TestJobStrategy_Validate(t *testing.T) { job: &batch.Job{ ObjectMeta: validObjectMeta, Spec: batch.JobSpec{ - Selector: nil, + Selector: defaultSelector, ManualSelector: pointer.Bool(false), Template: api.PodTemplateSpec{ Spec: validPodSpec, @@ -1355,21 +1355,19 @@ func TestJobStrategy_Validate(t *testing.T) { wantJob: &batch.Job{ ObjectMeta: validObjectMeta, Spec: batch.JobSpec{ - Selector: &metav1.LabelSelector{MatchLabels: map[string]string{batch.ControllerUidLabel: string(theUID)}}, + Selector: defaultSelector, ManualSelector: pointer.Bool(false), Template: api.PodTemplateSpec{ - ObjectMeta: metav1.ObjectMeta{ - Labels: validLabels, - }, Spec: validPodSpec, }}, }, + wantWarningCount: 5, }, "labels exist": { job: &batch.Job{ ObjectMeta: validObjectMeta, Spec: batch.JobSpec{ - Selector: nil, + Selector: defaultSelector, ManualSelector: pointer.Bool(false), Template: api.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ @@ -1381,7 +1379,7 @@ func TestJobStrategy_Validate(t *testing.T) { wantJob: &batch.Job{ ObjectMeta: validObjectMeta, Spec: batch.JobSpec{ - Selector: &metav1.LabelSelector{MatchLabels: map[string]string{batch.ControllerUidLabel: string(theUID)}}, + Selector: defaultSelector, ManualSelector: pointer.Bool(false), Template: api.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ @@ -1425,7 +1423,7 @@ func TestJobStrategy_Validate(t *testing.T) { job: &batch.Job{ ObjectMeta: validObjectMeta, Spec: batch.JobSpec{ - Selector: nil, + Selector: defaultSelector, ManualSelector: pointer.Bool(false), Template: api.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ @@ -1442,7 +1440,7 @@ func TestJobStrategy_Validate(t *testing.T) { wantJob: &batch.Job{ ObjectMeta: validObjectMeta, Spec: batch.JobSpec{ - Selector: &metav1.LabelSelector{MatchLabels: map[string]string{batch.ControllerUidLabel: string(theUID)}}, + Selector: defaultSelector, ManualSelector: pointer.Bool(false), Template: api.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ @@ -1461,7 +1459,7 @@ func TestJobStrategy_Validate(t *testing.T) { job: &batch.Job{ ObjectMeta: validObjectMeta, Spec: batch.JobSpec{ - Selector: nil, + Selector: defaultSelector, ManualSelector: pointer.Bool(false), Template: api.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ @@ -1479,7 +1477,7 @@ func TestJobStrategy_Validate(t *testing.T) { wantJob: &batch.Job{ ObjectMeta: validObjectMeta, Spec: batch.JobSpec{ - Selector: &metav1.LabelSelector{MatchLabels: map[string]string{batch.ControllerUidLabel: string(theUID)}}, + Selector: defaultSelector, ManualSelector: pointer.Bool(false), Template: api.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{