fix(manifest): preserve default values on environment overrides for jobs

Fixes #2043, fixes #1736

internal/pkg/deploy/cloudformation/stack/scheduled_job.go

@@ -203,32 +203,33 @@ func (j *ScheduledJob) SerializedParameters() (string, error) {
 // Exception is made for strings of the form "rate( )" or "cron( )". These are accepted as-is and
 // validated server-side by CloudFormation.
 func (j *ScheduledJob) awsSchedule() (string, error) {
-	if j.manifest.On.Schedule == "" {
+	schedule := aws.StringValue(j.manifest.On.Schedule)
+	if schedule == "" {
 		return "", fmt.Errorf(`missing required field "schedule" in manifest for job %s`, j.name)
 	}
 	// If the schedule uses default CloudWatch Events syntax, pass it through for server-side validation.
-	if match := awsScheduleRegexp.FindStringSubmatch(j.manifest.On.Schedule); match != nil {
-		return j.manifest.On.Schedule, nil
+	if match := awsScheduleRegexp.FindStringSubmatch(schedule); match != nil {
+		return aws.StringValue(j.manifest.On.Schedule), nil
 	}
 	// Try parsing the string as a cron expression to validate it.
-	if _, err := cron.ParseStandard(j.manifest.On.Schedule); err != nil {
+	if _, err := cron.ParseStandard(schedule); err != nil {
 		return "", errScheduleInvalid{reason: err}
 	}
 	var scheduleExpression string
 	var err error
 	switch {
-	case strings.HasPrefix(j.manifest.On.Schedule, every):
-		scheduleExpression, err = toRate(j.manifest.On.Schedule[len(every):])
+	case strings.HasPrefix(schedule, every):
+		scheduleExpression, err = toRate(schedule[len(every):])
 		if err != nil {
 			return "", fmt.Errorf("parse fixed interval: %w", err)
 		}
-	case strings.HasPrefix(j.manifest.On.Schedule, "@"):
-		scheduleExpression, err = toFixedSchedule(j.manifest.On.Schedule)
+	case strings.HasPrefix(schedule, "@"):
+		scheduleExpression, err = toFixedSchedule(schedule)
 		if err != nil {
 			return "", fmt.Errorf("parse preset schedule: %w", err)
 		}
 	default:
-		scheduleExpression, err = toAWSCron(j.manifest.On.Schedule)
+		scheduleExpression, err = toAWSCron(schedule)
 		if err != nil {
 			return "", fmt.Errorf("parse cron schedule: %w", err)
 		}
@@ -357,8 +358,8 @@ func toAWSCron(schedule string) (string, error) {
 // It also performs basic validations to provide a fast feedback loop to the customer.
 func (j *ScheduledJob) stateMachineOpts() (*template.StateMachineOpts, error) {
 	var timeoutSeconds *int
-	if j.manifest.Timeout != "" {
-		parsedTimeout, err := time.ParseDuration(j.manifest.Timeout)
+	if inTimeout := aws.StringValue(j.manifest.Timeout); inTimeout != "" {
+		parsedTimeout, err := time.ParseDuration(inTimeout)
 		if err != nil {
 			return nil, errDurationInvalid{reason: err}
 		}
@@ -372,11 +373,11 @@ func (j *ScheduledJob) stateMachineOpts() (*template.StateMachineOpts, error) {
 	}
 
 	var retries *int
-	if j.manifest.Retries != 0 {
-		if j.manifest.Retries < 0 {
+	if inRetries := aws.IntValue(j.manifest.Retries); inRetries != 0 {
+		if inRetries < 0 {
 			return nil, errors.New("number of retries cannot be negative")
 		}
-		retries = aws.Int(j.manifest.Retries)
+		retries = aws.Int(inRetries)
 	}
 	return &template.StateMachineOpts{
 		Timeout: timeoutSeconds,

internal/pkg/deploy/cloudformation/stack/scheduled_job_test.go

@@ -38,11 +38,11 @@ func TestScheduledJob_Template(t *testing.T) {
 		Retries:  3,
 	})
 	testScheduledJobManifest.EntryPoint = manifest.EntryPointOverride{
-		String: nil,
+		String:      nil,
 		StringSlice: []string{"/bin/echo", "hello"},
 	}
 	testScheduledJobManifest.Command = manifest.CommandOverride{
-		String: nil,
+		String:      nil,
 		StringSlice: []string{"world"},
 	}
 
@@ -66,7 +66,7 @@ func TestScheduledJob_Template(t *testing.T) {
 						SubnetsType:    template.PublicSubnetsPlacement,
 					},
 					EntryPoint: []string{"/bin/echo", "hello"},
-					Command: []string{"world"},
+					Command:    []string{"world"},
 				})).Return(&template.Content{Buffer: bytes.NewBufferString("template")}, nil)
 				addons := mockTemplater{err: &addon.ErrAddonsDirNotExist{}}
 				j.parser = m
@@ -94,7 +94,7 @@ func TestScheduledJob_Template(t *testing.T) {
 						SubnetsType:    template.PublicSubnetsPlacement,
 					},
 					EntryPoint: []string{"/bin/echo", "hello"},
-					Command: []string{"world"},
+					Command:    []string{"world"},
 				})).Return(&template.Content{Buffer: bytes.NewBufferString("template")}, nil)
 				addons := mockTemplater{
 					tpl: `Resources:
@@ -305,7 +305,7 @@ func TestScheduledJob_awsSchedule(t *testing.T) {
 				manifest: &manifest.ScheduledJob{
 					ScheduledJobConfig: manifest.ScheduledJobConfig{
 						On: manifest.JobTriggerConfig{
-							Schedule: tc.inputSchedule,
+							Schedule: aws.String(tc.inputSchedule),
 						},
 					},
 				},
@@ -385,8 +385,8 @@ func TestScheduledJob_stateMachine(t *testing.T) {
 				manifest: &manifest.ScheduledJob{
 					ScheduledJobConfig: manifest.ScheduledJobConfig{
 						JobFailureHandlerConfig: manifest.JobFailureHandlerConfig{
-							Retries: tc.inputRetries,
-							Timeout: tc.inputTimeout,
+							Retries: aws.Int(tc.inputRetries),
+							Timeout: aws.String(tc.inputTimeout),
 						},
 					},
 				},

internal/pkg/manifest/job.go

@@ -48,13 +48,13 @@ type ScheduledJobConfig struct {
 
 // JobTriggerConfig represents the configuration for the event that triggers the job.
 type JobTriggerConfig struct {
-	Schedule string `yaml:"schedule"`
+	Schedule *string `yaml:"schedule"`
 }
 
 // JobFailureHandlerConfig represents the error handling configuration for the job.
 type JobFailureHandlerConfig struct {
-	Timeout string `yaml:"timeout"`
-	Retries int    `yaml:"retries"`
+	Timeout *string `yaml:"timeout"`
+	Retries *int    `yaml:"retries"`
 }
 
 // ScheduledJobProps contains properties for creating a new scheduled job manifest.
@@ -93,13 +93,24 @@ func newDefaultScheduledJob() *ScheduledJob {
 func NewScheduledJob(props *ScheduledJobProps) *ScheduledJob {
 	job := newDefaultScheduledJob()
 	// Apply overrides.
-	job.Name = aws.String(props.Name)
-	job.ScheduledJobConfig.ImageConfig.Build.BuildArgs.Dockerfile = stringP(props.Dockerfile)
-	job.ScheduledJobConfig.ImageConfig.Location = stringP(props.Image)
-	job.On.Schedule = props.Schedule
-	job.Retries = props.Retries
-	job.Timeout = props.Timeout
-
+	if props.Name != "" {
+		job.Name = aws.String(props.Name)
+	}
+	if props.Dockerfile != "" {
+		job.ScheduledJobConfig.ImageConfig.Build.BuildArgs.Dockerfile = aws.String(props.Dockerfile)
+	}
+	if props.Image != "" {
+		job.ScheduledJobConfig.ImageConfig.Location = aws.String(props.Image)
+	}
+	if props.Schedule != "" {
+		job.On.Schedule = aws.String(props.Schedule)
+	}
+	if props.Retries != 0 {
+		job.Retries = aws.Int(props.Retries)
+	}
+	if props.Timeout != "" {
+		job.Timeout = aws.String(props.Timeout)
+	}
 	job.parser = template.New()
 	return job
 }

internal/pkg/manifest/job_test.go

@@ -9,6 +9,7 @@ import (
 	"path/filepath"
 	"testing"
 
+	"github.com/aws/aws-sdk-go/aws"
 	"github.com/stretchr/testify/require"
 )
 
@@ -82,3 +83,112 @@ func TestScheduledJob_MarshalBinary(t *testing.T) {
 		})
 	}
 }
+
+func TestScheduledJob_ApplyEnv(t *testing.T) {
+	testCases := map[string]struct {
+		inputManifest *ScheduledJob
+		inputEnv      string
+
+		wantedManifest *ScheduledJob
+		wantedErr      error
+	}{
+		"should return the same scheduled job if the environment does not exist": {
+			inputManifest: newDefaultScheduledJob(),
+			inputEnv:      "test",
+
+			wantedManifest: newDefaultScheduledJob(),
+		},
+		"should preserve defaults and only override fields under 'environment'": {
+			inputManifest: &ScheduledJob{
+				Workload: Workload{
+					Name: aws.String("report-generator"),
+					Type: aws.String(ScheduledJobType),
+				},
+				ScheduledJobConfig: ScheduledJobConfig{
+					ImageConfig: Image{
+						Location: aws.String("nginx"),
+					},
+					On: JobTriggerConfig{
+						Schedule: aws.String("@hourly"),
+					},
+					JobFailureHandlerConfig: JobFailureHandlerConfig{
+						Timeout: aws.String("5m"),
+						Retries: aws.Int(1),
+					},
+					TaskConfig: TaskConfig{
+						CPU:    aws.Int(256),
+						Memory: aws.Int(512),
+						Count: Count{
+							Value: aws.Int(1),
+						},
+					},
+					Network: NetworkConfig{
+						VPC: vpcConfig{
+							Placement: stringP(PublicSubnetPlacement),
+						},
+					},
+				},
+				Environments: map[string]*ScheduledJobConfig{
+					"prod": {
+						TaskConfig: TaskConfig{
+							Variables: map[string]string{
+								"LOG_LEVEL": "prod",
+							},
+						},
+					},
+				},
+			},
+			inputEnv: "prod",
+
+			wantedManifest: &ScheduledJob{
+				Workload: Workload{
+					Name: aws.String("report-generator"),
+					Type: aws.String(ScheduledJobType),
+				},
+				ScheduledJobConfig: ScheduledJobConfig{
+					ImageConfig: Image{
+						Location: aws.String("nginx"),
+					},
+					On: JobTriggerConfig{
+						Schedule: aws.String("@hourly"),
+					},
+					JobFailureHandlerConfig: JobFailureHandlerConfig{
+						Timeout: aws.String("5m"),
+						Retries: aws.Int(1),
+					},
+					TaskConfig: TaskConfig{
+						CPU:    aws.Int(256),
+						Memory: aws.Int(512),
+						Count: Count{
+							Value: aws.Int(1),
+						},
+						Variables: map[string]string{
+							"LOG_LEVEL": "prod",
+						},
+					},
+					Network: NetworkConfig{
+						VPC: vpcConfig{
+							Placement: stringP(PublicSubnetPlacement),
+						},
+					},
+				},
+				Environments: nil,
+			},
+		},
+	}
+
+	for name, tc := range testCases {
+		t.Run(name, func(t *testing.T) {
+			// WHEN
+			actualManifest, actualErr := tc.inputManifest.ApplyEnv(tc.inputEnv)
+
+			// THEN
+			if tc.wantedErr != nil {
+				require.EqualError(t, actualErr, tc.wantedErr.Error())
+			} else {
+				require.NoError(t, actualErr)
+				require.Equal(t, tc.wantedManifest, actualManifest)
+			}
+		})
+	}
+}