Repository: furiko-io/furiko
Pull request #88 (2022-06-03 20:52:06)

fix(webhook): Add default values for parallelism.completionStrategy
## Bug Reproduction

Trying to apply the following YAML results in not being able to save:

```yaml
apiVersion: execution.furiko.io/v1alpha1
kind: JobConfig
metadata:
  name: jobconfig-parallel-sleep
spec:
  concurrency:
    policy: Forbid
  schedule:
    cron:
      expression: "H/15 * * * *"
      timezone: Asia/Singapore
    disabled: false
  template:
    spec:
      parallelism:
        withKeys:
          - "30"
          - "60"
          - "120"
      taskTemplate:
        pod:
          spec:
            containers:
              - name: job-container
                args:
                  - bash
                  - -c
                  - "echo Sleeping for ${task.index_key}; sleep ${task.index_key}"
                image: bash
```

The following error is displayed:

```
$ k apply -f jobconfig-parallel-sleep.yaml
The JobConfig "jobconfig-parallel-sleep" is invalid: spec.template.spec.parallelism.completionStrategy: Required value
```

## Bugfix

By right, we should also add default values for JobTemplate in JobConfigSpec. The default value should be set to `AllSuccessful`.



Patch:
diff --git a/pkg/execution/mutation/mutation.go b/pkg/execution/mutation/mutation.go
index d9bc7a8..55b1a50 100644
--- a/pkg/execution/mutation/mutation.go
+++ b/pkg/execution/mutation/mutation.go
@@ -67,6 +67,11 @@ func (m *Mutator) MutateJobConfig(rjc *v1alpha1.JobConfig) *webhook.Result {
 		rjc.Spec.Option = options.MutateDefaultingOptionSpec(spec)
 	}
 
+	// Mutate JobTemplate, but don't add taskTemplate fields, because they should be
+	// added only when the Job is created.
+	result.Merge(m.MutateJobTemplateSpec(&rjc.Spec.Template.Spec, false,
+		field.NewPath("spec", "template", "spec")))
+
 	return result
 }
 
@@ -129,7 +134,7 @@ func (m *Mutator) MutateJob(rj *v1alpha1.Job) *webhook.Result {
 	if rj.Spec.Template == nil {
 		rj.Spec.Template = &v1alpha1.JobTemplate{}
 	}
-	result.Merge(m.MutateJobTemplateSpec(rj.Spec.Template, field.NewPath("spec", "template")))
+	result.Merge(m.MutateJobTemplateSpec(rj.Spec.Template, true, field.NewPath("spec", "template")))
 
 	return result
 }
@@ -292,24 +297,35 @@ func (m *Mutator) evaluateOptionValues(rj *v1alpha1.Job, rjc *v1alpha1.JobConfig
 }
 
 // MutateJobTemplateSpec mutates a JobTemplate in-place.
-func (m *Mutator) MutateJobTemplateSpec(spec *v1alpha1.JobTemplate, fldPath *field.Path) *webhook.Result {
+func (m *Mutator) MutateJobTemplateSpec(
+	spec *v1alpha1.JobTemplate,
+	taskTemplate bool,
+	fldPath *field.Path,
+) *webhook.Result {
 	result := webhook.NewResult()
 
-	// Add MaxAttempts if not specified.
 	if spec.MaxAttempts == nil {
 		spec.MaxAttempts = pointer.Int64(1)
 	}
-
 	if spec.Parallelism != nil {
 		result.Merge(m.MutateParallelismSpec(spec.Parallelism, fldPath.Child("parallelism")))
 	}
-	if spec.TaskTemplate.Pod != nil {
-		result.Merge(m.MutatePodTemplateSpec(spec.TaskTemplate.Pod, fldPath.Child("taskTemplate", "pod")))
+	if taskTemplate {
+		result.Merge(m.MutateTaskTemplate(&spec.TaskTemplate, fldPath.Child("taskTemplate")))
 	}
 
 	return result
 }
 
+// MutateTaskTemplate mutates a TaskTemplate in-place.
+func (m *Mutator) MutateTaskTemplate(spec *v1alpha1.TaskTemplate, fldPath *field.Path) *webhook.Result {
+	result := webhook.NewResult()
+	if spec.Pod != nil {
+		result.Merge(m.MutatePodTemplateSpec(spec.Pod, fldPath.Child("pod")))
+	}
+	return result
+}
+
 // MutateParallelismSpec mutates ParallelismSpec in-place.
 func (m *Mutator) MutateParallelismSpec(spec *v1alpha1.ParallelismSpec, fldPath *field.Path) *webhook.Result {
 	result := webhook.NewResult()


Test patch:
diff --git a/pkg/execution/mutation/mutation_test.go b/pkg/execution/mutation/mutation_test.go
index 4a130dd..b289130 100644
--- a/pkg/execution/mutation/mutation_test.go
+++ b/pkg/execution/mutation/mutation_test.go
@@ -163,6 +163,15 @@ var (
 		},
 	}
 
+	jobConfigJobTemplateSpecBasic = v1alpha1.JobTemplateSpec{
+		Spec: v1alpha1.JobTemplate{
+			TaskTemplate: v1alpha1.TaskTemplate{
+				Pod: &podTemplateSpecBasic,
+			},
+			MaxAttempts: pointer.Int64(1),
+		},
+	}
+
 	jobTemplateSpecBasic2 = v1alpha1.JobTemplateSpec{
 		Spec: v1alpha1.JobTemplate{
 			TaskTemplate: v1alpha1.TaskTemplate{
@@ -223,7 +232,7 @@ func TestMutator_MutateJobConfig(t *testing.T) {
 			rjc: &v1alpha1.JobConfig{
 				ObjectMeta: objectMetaJobConfig,
 				Spec: v1alpha1.JobConfigSpec{
-					Template: jobTemplateSpecBasic,
+					Template: jobConfigJobTemplateSpecBasic,
 				},
 			},
 		},
@@ -237,6 +246,42 @@ func TestMutator_MutateJobConfig(t *testing.T) {
 							TaskTemplate: v1alpha1.TaskTemplate{
 								Pod: &podTemplateSpecBare,
 							},
+							MaxAttempts: pointer.Int64(1),
+						},
+					},
+				},
+			},
+		},
+		{
+			name: "add default JobTemplate values",
+			rjc: &v1alpha1.JobConfig{
+				ObjectMeta: objectMetaJobConfig,
+				Spec: v1alpha1.JobConfigSpec{
+					Template: v1alpha1.JobTemplateSpec{
+						Spec: v1alpha1.JobTemplate{
+							TaskTemplate: v1alpha1.TaskTemplate{
+								Pod: &podTemplateSpecBare,
+							},
+							Parallelism: &v1alpha1.ParallelismSpec{
+								WithCount: pointer.Int64(3),
+							},
+						},
+					},
+				},
+			},
+			want: &v1alpha1.JobConfig{
+				ObjectMeta: objectMetaJobConfig,
+				Spec: v1alpha1.JobConfigSpec{
+					Template: v1alpha1.JobTemplateSpec{
+						Spec: v1alpha1.JobTemplate{
+							TaskTemplate: v1alpha1.TaskTemplate{
+								Pod: &podTemplateSpecBare,
+							},
+							MaxAttempts: pointer.Int64(1),
+							Parallelism: &v1alpha1.ParallelismSpec{
+								WithCount:          pointer.Int64(3),
+								CompletionStrategy: v1alpha1.AllSuccessful,
+							},
 						},
 					},
 				},
@@ -247,7 +292,7 @@ func TestMutator_MutateJobConfig(t *testing.T) {
 			rjc: &v1alpha1.JobConfig{
 				ObjectMeta: objectMetaJobConfig,
 				Spec: v1alpha1.JobConfigSpec{
-					Template: jobTemplateSpecBasic,
+					Template: jobConfigJobTemplateSpecBasic,
 					Option: &v1alpha1.OptionSpec{
 						Options: []v1alpha1.Option{
 							{
@@ -261,7 +306,7 @@ func TestMutator_MutateJobConfig(t *testing.T) {
 			want: &v1alpha1.JobConfig{
 				ObjectMeta: objectMetaJobConfig,
 				Spec: v1alpha1.JobConfigSpec{
-					Template: jobTemplateSpecBasic,
+					Template: jobConfigJobTemplateSpecBasic,
 					Option: &v1alpha1.OptionSpec{
 						Options: []v1alpha1.Option{
 							{