fix: honor env-specific image override (#2415)

This PR:
1. implements custom`merge.Merge` logic for `image` field of workload manifest.
2. `ApplyEnv` override immediately after reading manifest, so that the image is built based on env-specific image override.

Fixes: #1702 and #2382.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Author: Lou1415926

internal/pkg/cli/job_deploy.go

@@ -38,7 +38,7 @@ type deployJobOpts struct {
 
 	store              store
 	ws                 wsJobDirReader
-	unmarshal          func(in []byte) (interface{}, error)
+	unmarshal          func(in []byte) (manifest.WorkloadManifest, error)
 	cmd                runner
 	addons             templater
 	appCFN             appResourcesGetter

internal/pkg/cli/svc_deploy.go

@@ -51,7 +51,7 @@ type deploySvcOpts struct {
 	store              store
 	ws                 wsSvcDirReader
 	imageBuilderPusher imageBuilderPusher
-	unmarshal          func([]byte) (interface{}, error)
+	unmarshal          func([]byte) (manifest.WorkloadManifest, error)
 	s3                 artifactUploader
 	cmd                runner
 	addons             templater
@@ -375,7 +375,11 @@ func (o *deploySvcOpts) manifest() (interface{}, error) {
 	if err != nil {
 		return nil, fmt.Errorf("unmarshal service %s manifest: %w", o.name, err)
 	}
-	return mft, nil
+	envMft, err := mft.ApplyEnv(o.envName)
+	if err != nil {
+		return nil, fmt.Errorf("apply environment %s override: %s", o.envName, err)
+	}
+	return envMft, nil
 }
 
 func (o *deploySvcOpts) runtimeConfig(addonsURL string) (*stack.RuntimeConfig, error) {

internal/pkg/cli/svc_deploy_test.go

@@ -651,7 +651,7 @@ func TestSvcDeployOpts_stackConfiguration(t *testing.T) {
 				appVersionGetter:  mockAppVersionGetter,
 				targetApp:         tc.inApp,
 				targetEnvironment: tc.inEnvironment,
-				unmarshal: func(b []byte) (interface{}, error) {
+				unmarshal: func(b []byte) (manifest.WorkloadManifest, error) {
 					return &manifest.LoadBalancedWebService{
 						Workload: manifest.Workload{
 							Name: aws.String(mockSvcName),

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

@@ -44,24 +44,20 @@ func NewBackendService(mft *manifest.BackendService, env, app string, rc Runtime
 	if err != nil {
 		return nil, fmt.Errorf("new addons: %w", err)
 	}
-	envManifest, err := mft.ApplyEnv(env) // Apply environment overrides to the manifest values.
-	if err != nil {
-		return nil, fmt.Errorf("apply environment %s override: %w", env, err)
-	}
 	return &BackendService{
 		ecsWkld: &ecsWkld{
 			wkld: &wkld{
 				name:   aws.StringValue(mft.Name),
 				env:    env,
 				app:    app,
 				rc:     rc,
-				image:  envManifest.ImageConfig,
+				image:  mft.ImageConfig,
 				parser: parser,
 				addons: addons,
 			},
-			tc: envManifest.TaskConfig,
+			tc: mft.TaskConfig,
 		},
-		manifest: envManifest,
+		manifest: mft,
 
 		parser: parser,
 	}, nil

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

@@ -25,79 +25,59 @@ import (
 
 const (
 	svcManifestPath = "svc-manifest.yml"
+	svcStackPath    = "svc-test.stack.yml"
+	svcParamsPath   = "svc-test.params.json"
 )
 
 func TestLoadBalancedWebService_Template(t *testing.T) {
-	testCases := map[string]struct {
-		envName       string
-		svcStackPath  string
-		svcParamsPath string
-	}{
-		"default env": {
-			envName:       "test",
-			svcStackPath:  "svc-test.stack.yml",
-			svcParamsPath: "svc-test.params.json",
-		},
-		"staging env": {
-			envName:       "staging",
-			svcStackPath:  "svc-staging.stack.yml",
-			svcParamsPath: "svc-staging.params.json",
-		},
-		"prod env": {
-			envName:       "prod",
-			svcStackPath:  "svc-prod.stack.yml",
-			svcParamsPath: "svc-prod.params.json",
-		},
-	}
 	path := filepath.Join("testdata", "workloads", svcManifestPath)
 	manifestBytes, err := ioutil.ReadFile(path)
 	require.NoError(t, err)
 	mft, err := manifest.UnmarshalWorkload(manifestBytes)
 	require.NoError(t, err)
-	v, ok := mft.(*manifest.LoadBalancedWebService)
+	envMft, err := mft.ApplyEnv(envName)
+	require.NoError(t, err)
+	v, ok := envMft.(*manifest.LoadBalancedWebService)
 	require.True(t, ok)
 
-	for name, tc := range testCases {
-
-		serializer, err := stack.NewHTTPSLoadBalancedWebService(v, tc.envName, appName, stack.RuntimeConfig{})
+	serializer, err := stack.NewHTTPSLoadBalancedWebService(v, envName, appName, stack.RuntimeConfig{})
 
-		tpl, err := serializer.Template()
-		require.NoError(t, err, "template should render")
-		regExpGUID := regexp.MustCompile(`([a-f\d]{8}-)([a-f\d]{4}-){3}([a-f\d]{12})`) // Matches random guids
-		testName := fmt.Sprintf("CF Template should be equal/%s", name)
-		parser := template.New()
-		envController, err := parser.Read(envControllerPath)
+	tpl, err := serializer.Template()
+	require.NoError(t, err, "template should render")
+	regExpGUID := regexp.MustCompile(`([a-f\d]{8}-)([a-f\d]{4}-){3}([a-f\d]{12})`) // Matches random guids
+	testName := fmt.Sprintf("CF Template should be equal")
+	parser := template.New()
+	envController, err := parser.Read(envControllerPath)
+	require.NoError(t, err)
+	zipFile := envController.String()
+	t.Run(testName, func(t *testing.T) {
+		actualBytes := []byte(tpl)
+		// Cut random GUID from template.
+		actualBytes = regExpGUID.ReplaceAll(actualBytes, []byte("RandomGUID"))
+		actualString := string(actualBytes)
+		// Cut out zip file from EnvControllerAction for more readable output
+		actualString = strings.ReplaceAll(actualString, zipFile, "Abracadabra")
+		actualBytes = []byte(actualString)
+		mActual := make(map[interface{}]interface{})
+		require.NoError(t, yaml.Unmarshal(actualBytes, mActual))
+
+		expected, err := ioutil.ReadFile(filepath.Join("testdata", "workloads", svcStackPath))
+		require.NoError(t, err, "should be able to read expected bytes")
+		expectedBytes := []byte(expected)
+		mExpected := make(map[interface{}]interface{})
+		require.NoError(t, yaml.Unmarshal(expectedBytes, mExpected))
+		require.Equal(t, mExpected, mActual)
+	})
+
+	testName = fmt.Sprintf("Parameter values should render properly")
+	t.Run(testName, func(t *testing.T) {
+		actualParams, err := serializer.SerializedParameters()
 		require.NoError(t, err)
-		zipFile := envController.String()
-		t.Run(testName, func(t *testing.T) {
-			actualBytes := []byte(tpl)
-			// Cut random GUID from template.
-			actualBytes = regExpGUID.ReplaceAll(actualBytes, []byte("RandomGUID"))
-			actualString := string(actualBytes)
-			// Cut out zip file from EnvControllerAction for more readable output
-			actualString = strings.ReplaceAll(actualString, zipFile, "Abracadabra")
-			actualBytes = []byte(actualString)
-			mActual := make(map[interface{}]interface{})
-			require.NoError(t, yaml.Unmarshal(actualBytes, mActual))
 
-			expected, err := ioutil.ReadFile(filepath.Join("testdata", "workloads", tc.svcStackPath))
-			require.NoError(t, err, "should be able to read expected bytes")
-			expectedBytes := []byte(expected)
-			mExpected := make(map[interface{}]interface{})
-			require.NoError(t, yaml.Unmarshal(expectedBytes, mExpected))
-			require.Equal(t, mExpected, mActual)
-		})
-
-		testName = fmt.Sprintf("Parameter values should render properly/%s", name)
-		t.Run(testName, func(t *testing.T) {
-			actualParams, err := serializer.SerializedParameters()
-			require.NoError(t, err)
-
-			path := filepath.Join("testdata", "workloads", tc.svcParamsPath)
-			wantedCFNParamsBytes, err := ioutil.ReadFile(path)
-			require.NoError(t, err)
+		path := filepath.Join("testdata", "workloads", svcParamsPath)
+		wantedCFNParamsBytes, err := ioutil.ReadFile(path)
+		require.NoError(t, err)
 
-			require.Equal(t, string(wantedCFNParamsBytes), actualParams)
-		})
-	}
+		require.Equal(t, string(wantedCFNParamsBytes), actualParams)
+	})
 }

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

@@ -52,24 +52,20 @@ func NewLoadBalancedWebService(mft *manifest.LoadBalancedWebService, env, app st
 	if err != nil {
 		return nil, fmt.Errorf("new addons: %w", err)
 	}
-	envManifest, err := mft.ApplyEnv(env) // Apply environment overrides to the manifest values.
-	if err != nil {
-		return nil, fmt.Errorf("apply environment %s override: %s", env, err)
-	}
 	return &LoadBalancedWebService{
 		ecsWkld: &ecsWkld{
 			wkld: &wkld{
 				name:   aws.StringValue(mft.Name),
 				env:    env,
 				app:    app,
 				rc:     rc,
-				image:  envManifest.ImageConfig,
+				image:  mft.ImageConfig,
 				parser: parser,
 				addons: addons,
 			},
-			tc: envManifest.TaskConfig,
+			tc: mft.TaskConfig,
 		},
-		manifest:     envManifest,
+		manifest:     mft,
 		httpsEnabled: false,
 
 		parser: parser,

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

@@ -32,26 +32,22 @@ func NewRequestDrivenWebService(mft *manifest.RequestDrivenWebService, env, app
 	if err != nil {
 		return nil, fmt.Errorf("new addons: %w", err)
 	}
-	envManifest, err := mft.ApplyEnv(env) // Apply environment overrides to the manifest values.
-	if err != nil {
-		return nil, fmt.Errorf("apply environment %s override: %s", env, err)
-	}
 	return &RequestDrivenWebService{
 		appRunnerWkld: &appRunnerWkld{
 			wkld: &wkld{
 				name:   aws.StringValue(mft.Name),
 				env:    env,
 				app:    app,
 				rc:     rc,
-				image:  envManifest.ImageConfig,
+				image:  mft.ImageConfig,
 				addons: addons,
 				parser: parser,
 			},
-			instanceConfig:    envManifest.InstanceConfig,
-			imageConfig:       envManifest.ImageConfig,
-			healthCheckConfig: envManifest.HealthCheckConfiguration,
+			instanceConfig:    mft.InstanceConfig,
+			imageConfig:       mft.ImageConfig,
+			healthCheckConfig: mft.HealthCheckConfiguration,
 		},
-		manifest: envManifest,
+		manifest: mft,
 		parser:   parser,
 	}, nil
 }

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

@@ -95,24 +95,20 @@ func NewScheduledJob(mft *manifest.ScheduledJob, env, app string, rc RuntimeConf
 	if err != nil {
 		return nil, fmt.Errorf("new addons: %w", err)
 	}
-	envManifest, err := mft.ApplyEnv(env)
-	if err != nil {
-		return nil, fmt.Errorf("apply environment %s override: %w", env, err)
-	}
 	return &ScheduledJob{
 		ecsWkld: &ecsWkld{
 			wkld: &wkld{
 				name:   aws.StringValue(mft.Name),
 				env:    env,
 				app:    app,
 				rc:     rc,
-				image:  envManifest.ImageConfig,
+				image:  mft.ImageConfig,
 				parser: parser,
 				addons: addons,
 			},
-			tc: envManifest.TaskConfig,
+			tc: mft.TaskConfig,
 		},
-		manifest: envManifest,
+		manifest: mft,
 
 		parser: parser,
 	}, nil

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

@@ -34,8 +34,11 @@ func TestScheduledJob_Template(t *testing.T) {
 	require.NoError(t, err)
 	mft, err := manifest.UnmarshalWorkload(manifestBytes)
 	require.NoError(t, err)
-	v, ok := mft.(*manifest.ScheduledJob)
+	envMft, err := mft.ApplyEnv(envName)
+	require.NoError(t, err)
+	v, ok := envMft.(*manifest.ScheduledJob)
 	require.True(t, ok)
+
 	serializer, err := stack.NewScheduledJob(v, envName, appName, stack.RuntimeConfig{})
 
 	tpl, err := serializer.Template()

internal/pkg/manifest/backend_svc.go

@@ -104,7 +104,7 @@ func (s *BackendService) BuildArgs(wsRoot string) *DockerBuildArgs {
 
 // ApplyEnv returns the service manifest with environment overrides.
 // If the environment passed in does not have any overrides then it returns itself.
-func (s BackendService) ApplyEnv(envName string) (*BackendService, error) {
+func (s BackendService) ApplyEnv(envName string) (WorkloadManifest, error) {
 	overrideConfig, ok := s.Environments[envName]
 	if !ok {
 		return &s, nil
@@ -122,7 +122,8 @@ func (s BackendService) ApplyEnv(envName string) (*BackendService, error) {
 	// Apply overrides to the original service s.
 	err := mergo.Merge(&s, BackendService{
 		BackendServiceConfig: *overrideConfig,
-	}, mergo.WithOverride, mergo.WithOverwriteWithEmptyValue)
+	}, mergo.WithOverride, mergo.WithOverwriteWithEmptyValue, mergo.WithTransformers(workloadTransformer{}))
+
 	if err != nil {
 		return nil, err
 	}