Apply default values to profiles

Fixes #493

Signed-off-by: David Gageot <[email protected]>

Author: dgageot

pkg/skaffold/config/profile_test.go

@@ -25,11 +25,11 @@ import (
 
 func TestApplyProfiles(t *testing.T) {
 	tests := []struct {
-		description    string
-		config         SkaffoldConfig
-		profile        string
-		expectedConfig SkaffoldConfig
-		shouldErr      bool
+		description string
+		config      SkaffoldConfig
+		profile     string
+		expected    SkaffoldConfig
+		shouldErr   bool
 	}{
 		{
 			description: "unknown profile",
@@ -61,14 +61,25 @@ func TestApplyProfiles(t *testing.T) {
 					},
 				},
 			},
-			expectedConfig: SkaffoldConfig{
+			expected: SkaffoldConfig{
 				Build: v1alpha2.BuildConfig{
 					Artifacts: []*v1alpha2.Artifact{
-						{ImageName: "image"},
+						{
+							ImageName: "image",
+							Workspace: ".",
+							ArtifactType: v1alpha2.ArtifactType{
+								DockerArtifact: &v1alpha2.DockerArtifact{
+									DockerfilePath: "Dockerfile",
+								},
+							},
+						},
 					},
 					BuildType: v1alpha2.BuildType{
 						GoogleCloudBuild: &v1alpha2.GoogleCloudBuild{},
 					},
+					TagPolicy: v1alpha2.TagPolicy{
+						GitTagger: &v1alpha2.GitTagger{},
+					},
 				},
 				Deploy: v1alpha2.DeployConfig{},
 			},
@@ -93,12 +104,23 @@ func TestApplyProfiles(t *testing.T) {
 					},
 				},
 			},
-			expectedConfig: SkaffoldConfig{
+			expected: SkaffoldConfig{
 				Build: v1alpha2.BuildConfig{
 					Artifacts: []*v1alpha2.Artifact{
-						{ImageName: "image"},
+						{
+							ImageName: "image",
+							Workspace: ".",
+							ArtifactType: v1alpha2.ArtifactType{
+								DockerArtifact: &v1alpha2.DockerArtifact{
+									DockerfilePath: "Dockerfile",
+								},
+							},
+						},
 					},
 					TagPolicy: v1alpha2.TagPolicy{ShaTagger: &v1alpha2.ShaTagger{}},
+					BuildType: v1alpha2.BuildType{
+						LocalBuild: &v1alpha2.LocalBuild{},
+					},
 				},
 				Deploy: v1alpha2.DeployConfig{},
 			},
@@ -126,13 +148,32 @@ func TestApplyProfiles(t *testing.T) {
 					},
 				},
 			},
-			expectedConfig: SkaffoldConfig{
+			expected: SkaffoldConfig{
 				Build: v1alpha2.BuildConfig{
 					Artifacts: []*v1alpha2.Artifact{
-						{ImageName: "image"},
-						{ImageName: "imageProd"},
+						{
+							ImageName: "image",
+							Workspace: ".",
+							ArtifactType: v1alpha2.ArtifactType{
+								DockerArtifact: &v1alpha2.DockerArtifact{
+									DockerfilePath: "Dockerfile",
+								},
+							},
+						},
+						{
+							ImageName: "imageProd",
+							Workspace: ".",
+							ArtifactType: v1alpha2.ArtifactType{
+								DockerArtifact: &v1alpha2.DockerArtifact{
+									DockerfilePath: "Dockerfile",
+								},
+							},
+						},
 					},
 					TagPolicy: v1alpha2.TagPolicy{GitTagger: &v1alpha2.GitTagger{}},
+					BuildType: v1alpha2.BuildType{
+						LocalBuild: &v1alpha2.LocalBuild{},
+					},
 				},
 				Deploy: v1alpha2.DeployConfig{},
 			},
@@ -158,21 +199,29 @@ func TestApplyProfiles(t *testing.T) {
 					},
 				},
 			},
-			expectedConfig: SkaffoldConfig{
-				Build: v1alpha2.BuildConfig{},
+			expected: SkaffoldConfig{
+				Build: v1alpha2.BuildConfig{
+					TagPolicy: v1alpha2.TagPolicy{
+						GitTagger: &v1alpha2.GitTagger{},
+					},
+					BuildType: v1alpha2.BuildType{
+						LocalBuild: &v1alpha2.LocalBuild{},
+					},
+				},
 				Deploy: v1alpha2.DeployConfig{
 					DeployType: v1alpha2.DeployType{
 						HelmDeploy: &v1alpha2.HelmDeploy{},
 					},
 				},
 			},
-		}}
+		},
+	}
 
 	for _, test := range tests {
 		t.Run(test.description, func(t *testing.T) {
 			err := test.config.ApplyProfiles([]string{test.profile})
 
-			testutil.CheckErrorAndDeepEqual(t, test.shouldErr, err, test.expectedConfig, test.config)
+			testutil.CheckErrorAndDeepEqual(t, test.shouldErr, err, test.expected, test.config)
 		})
 	}
 }

pkg/skaffold/schema/v1alpha2/config.go

@@ -37,8 +37,8 @@ type SkaffoldConfig struct {
 	Profiles []Profile    `yaml:"profiles,omitempty"`
 }
 
-func (config *SkaffoldConfig) GetVersion() string {
-	return config.APIVersion
+func (c *SkaffoldConfig) GetVersion() string {
+	return c.APIVersion
 }
 
 // BuildConfig contains all the configuration for the build steps
@@ -156,17 +156,38 @@ type BazelArtifact struct {
 	BuildTarget string `yaml:"target"`
 }
 
-func defaultToLocalBuild(config *SkaffoldConfig) {
-	if config.Build.BuildType != (BuildType{}) {
+// Parse reads a SkaffoldConfig from yaml.
+func (c *SkaffoldConfig) Parse(contents []byte, useDefaults bool) error {
+	if err := yaml.Unmarshal(contents, c); err != nil {
+		return err
+	}
+
+	if useDefaults {
+		c.setDefaultValues()
+	}
+
+	return nil
+}
+
+func (c *SkaffoldConfig) setDefaultValues() {
+	c.defaultToLocalBuild()
+	c.defaultToDockerArtifacts()
+	c.setDefaultTagger()
+	c.setDefaultDockerfiles()
+	c.setDefaultWorkspaces()
+}
+
+func (c *SkaffoldConfig) defaultToLocalBuild() {
+	if c.Build.BuildType != (BuildType{}) {
 		return
 	}
 
 	logrus.Debugf("Defaulting build type to local build")
-	config.Build.BuildType.LocalBuild = &LocalBuild{}
+	c.Build.BuildType.LocalBuild = &LocalBuild{}
 }
 
-func defaultToDockerArtifacts(config *SkaffoldConfig) {
-	for _, artifact := range config.Build.Artifacts {
+func (c *SkaffoldConfig) defaultToDockerArtifacts() {
+	for _, artifact := range c.Build.Artifacts {
 		if artifact.ArtifactType != (ArtifactType{}) {
 			continue
 		}
@@ -177,24 +198,24 @@ func defaultToDockerArtifacts(config *SkaffoldConfig) {
 	}
 }
 
-func setDefaultTagger(cfg *SkaffoldConfig) {
-	if cfg.Build.TagPolicy != (TagPolicy{}) {
+func (c *SkaffoldConfig) setDefaultTagger() {
+	if c.Build.TagPolicy != (TagPolicy{}) {
 		return
 	}
 
-	cfg.Build.TagPolicy = TagPolicy{GitTagger: &GitTagger{}}
+	c.Build.TagPolicy = TagPolicy{GitTagger: &GitTagger{}}
 }
 
-func setDefaultDockerfiles(config *SkaffoldConfig) {
-	for _, artifact := range config.Build.Artifacts {
+func (c *SkaffoldConfig) setDefaultDockerfiles() {
+	for _, artifact := range c.Build.Artifacts {
 		if artifact.DockerArtifact != nil && artifact.DockerArtifact.DockerfilePath == "" {
 			artifact.DockerArtifact.DockerfilePath = constants.DefaultDockerfilePath
 		}
 	}
 }
 
-func setDefaultWorkspaces(config *SkaffoldConfig) {
-	for _, artifact := range config.Build.Artifacts {
+func (c *SkaffoldConfig) setDefaultWorkspaces() {
+	for _, artifact := range c.Build.Artifacts {
 		if artifact.Workspace == "" {
 			artifact.Workspace = "."
 		}
@@ -203,23 +224,24 @@ func setDefaultWorkspaces(config *SkaffoldConfig) {
 
 // ApplyProfiles returns configuration modified by the application
 // of a list of profiles.
-func (config *SkaffoldConfig) ApplyProfiles(profiles []string) error {
+func (c *SkaffoldConfig) ApplyProfiles(profiles []string) error {
 	var err error
 
-	byName := profilesByName(config.Profiles)
+	byName := profilesByName(c.Profiles)
 	for _, name := range profiles {
 		profile, present := byName[name]
 		if !present {
 			return fmt.Errorf("couldn't find profile %s", name)
 		}
 
-		err = applyProfile(config, profile)
+		err = applyProfile(c, profile)
 		if err != nil {
 			return errors.Wrapf(err, "applying profile %s", name)
 		}
 	}
 
-	config.Profiles = nil
+	c.setDefaultValues()
+	c.Profiles = nil
 
 	return nil
 }
@@ -242,17 +264,3 @@ func profilesByName(profiles []Profile) map[string]Profile {
 	}
 	return byName
 }
-
-func (config *SkaffoldConfig) Parse(contents []byte, useDefaults bool) error {
-	if err := yaml.Unmarshal(contents, config); err != nil {
-		return err
-	}
-	if useDefaults {
-		defaultToLocalBuild(config)
-		defaultToDockerArtifacts(config)
-		setDefaultTagger(config)
-		setDefaultDockerfiles(config)
-		setDefaultWorkspaces(config)
-	}
-	return nil
-}