Merge pull request #1332 from dgageot/missing-defaults-during-upgrade

Apply default values upgraded configurations

cmd/skaffold/app/cmd/build.go

@@ -58,7 +58,7 @@ func runBuild(out io.Writer) error {
 	defer cancel()
 	catchCtrlC(cancel)
 
-	runner, config, err := newRunner(out, opts)
+	runner, config, err := newRunner(opts)
 	if err != nil {
 		return errors.Wrap(err, "creating runner")
 	}

cmd/skaffold/app/cmd/delete.go

@@ -43,7 +43,7 @@ func delete(out io.Writer) error {
 	defer cancel()
 	catchCtrlC(cancel)
 
-	runner, _, err := newRunner(out, opts)
+	runner, _, err := newRunner(opts)
 	if err != nil {
 		return errors.Wrap(err, "creating runner")
 	}

cmd/skaffold/app/cmd/dev.go

@@ -65,7 +65,7 @@ func dev(out io.Writer) error {
 		case <-ctx.Done():
 			return nil
 		default:
-			r, config, err := newRunner(out, opts)
+			r, config, err := newRunner(opts)
 			if err != nil {
 				return errors.Wrap(err, "creating runner")
 			}

cmd/skaffold/app/cmd/diagnose.go

@@ -49,7 +49,7 @@ func NewCmdDiagnose(out io.Writer) *cobra.Command {
 }
 
 func doDiagnose(out io.Writer) error {
-	_, config, err := newRunner(out, opts)
+	_, config, err := newRunner(opts)
 	if err != nil {
 		return errors.Wrap(err, "creating runner")
 	}

cmd/skaffold/app/cmd/fix.go

@@ -20,36 +20,21 @@ import (
 	"io"
 	"io/ioutil"
 
+	"github.com/pkg/errors"
+	"github.com/spf13/cobra"
+	"gopkg.in/yaml.v2"
+
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/color"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
-	schemautil "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/util"
-
-	"github.com/pkg/errors"
-	"github.com/sirupsen/logrus"
-	"github.com/spf13/cobra"
-	yaml "gopkg.in/yaml.v2"
 )
 
 func NewCmdFix(out io.Writer) *cobra.Command {
 	cmd := &cobra.Command{
 		Use:   "fix",
 		Short: "Converts old skaffold.yaml to newest schema version",
-		Run: func(cmd *cobra.Command, args []string) {
-			cfg, err := schema.ParseConfig(opts.ConfigurationFile, false)
-			if err != nil {
-				logrus.Error(err)
-				return
-			}
-
-			if cfg.GetVersion() == latest.Version {
-				color.Default.Fprintln(out, "config is already latest version")
-				return
-			}
-
-			if err := runFix(out, cfg); err != nil {
-				logrus.Errorf("fix: %s", err)
-			}
+		RunE: func(cmd *cobra.Command, args []string) error {
+			return runFix(out, opts.ConfigurationFile, overwrite)
 		},
 		Args: cobra.NoArgs,
 	}
@@ -58,8 +43,18 @@ func NewCmdFix(out io.Writer) *cobra.Command {
 	return cmd
 }
 
-func runFix(out io.Writer, cfg schemautil.VersionedConfig) error {
-	cfg, err := schema.UpgradeToLatest(out, cfg)
+func runFix(out io.Writer, configFile string, overwrite bool) error {
+	cfg, err := schema.ParseConfig(configFile, false)
+	if err != nil {
+		return err
+	}
+
+	if cfg.GetVersion() == latest.Version {
+		color.Default.Fprintln(out, "config is already latest version")
+		return nil
+	}
+
+	cfg, err = schema.ParseConfig(configFile, true)
 	if err != nil {
 		return err
 	}
@@ -70,7 +65,7 @@ func runFix(out io.Writer, cfg schemautil.VersionedConfig) error {
 	}
 
 	if overwrite {
-		if err := ioutil.WriteFile(opts.ConfigurationFile, newCfg, 0644); err != nil {
+		if err := ioutil.WriteFile(configFile, newCfg, 0644); err != nil {
 			return errors.Wrap(err, "writing config file")
 		}
 		color.Default.Fprintf(out, "New config at version %s generated and written to %s\n", cfg.GetVersion(), opts.ConfigurationFile)

cmd/skaffold/app/cmd/fix_test.go

@@ -19,22 +19,22 @@ package cmd
 import (
 	"bytes"
 	"fmt"
+	"io/ioutil"
 	"testing"
 
-	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
 	"github.com/GoogleContainerTools/skaffold/testutil"
 )
 
 func TestFix(t *testing.T) {
 	tests := []struct {
-		name       string
-		inputYaml  string
-		outputYaml string
-		shouldErr  bool
+		description string
+		inputYaml   string
+		output      string
+		shouldErr   bool
 	}{
 		{
-			name: "v1alpha4 to latest",
+			description: "v1alpha4 to latest",
 			inputYaml: `apiVersion: skaffold/v1alpha4
 kind: Config
 build:
@@ -51,7 +51,7 @@ deploy:
     manifests:
     - k8s/deployment.yaml
 `,
-			outputYaml: fmt.Sprintf(`apiVersion: %s
+			output: fmt.Sprintf(`apiVersion: %s
 kind: Config
 build:
   artifacts:
@@ -69,7 +69,7 @@ deploy:
 `, latest.Version),
 		},
 		{
-			name: "v1alpha1 to latest",
+			description: "v1alpha1 to latest",
 			inputYaml: `apiVersion: skaffold/v1alpha1
 kind: Config
 build:
@@ -82,7 +82,7 @@ deploy:
     - paths:
       - k8s/deployment.yaml
 `,
-			outputYaml: fmt.Sprintf(`apiVersion: %s
+			output: fmt.Sprintf(`apiVersion: %s
 kind: Config
 build:
   artifacts:
@@ -95,19 +95,74 @@ deploy:
     - k8s/deployment.yaml
 `, latest.Version),
 		},
+		{
+			description: "already latest version",
+			inputYaml: fmt.Sprintf(`apiVersion: %s
+kind: Config
+`, latest.Version),
+			output: "config is already latest version\n",
+		},
+		{
+			description: "invalid input",
+			inputYaml:   "invalid",
+			shouldErr:   true,
+		},
 	}
 
-	for _, tt := range tests {
-		cfgFile, teardown := testutil.TempFile(t, "config", []byte(tt.inputYaml))
-		defer teardown()
+	for _, test := range tests {
+		t.Run(test.description, func(t *testing.T) {
+			cfgFile, teardown := testutil.TempFile(t, "config", []byte(test.inputYaml))
+			defer teardown()
 
-		cfg, err := schema.ParseConfig(cfgFile, false)
-		if err != nil {
-			t.Fatalf(err.Error())
-		}
-		var b bytes.Buffer
-		err = runFix(&b, cfg)
+			var b bytes.Buffer
+			err := runFix(&b, cfgFile, false)
 
-		testutil.CheckErrorAndDeepEqual(t, tt.shouldErr, err, tt.outputYaml, b.String())
+			testutil.CheckErrorAndDeepEqual(t, test.shouldErr, err, test.output, b.String())
+		})
 	}
 }
+
+func TestFixOverwrite(t *testing.T) {
+	inputYaml := `apiVersion: skaffold/v1alpha4
+kind: Config
+build:
+  artifacts:
+  - image: docker/image
+    docker:
+      dockerfile: dockerfile.test
+test:
+- image: docker/image
+  structureTests:
+  - ./test/*
+deploy:
+  kubectl:
+    manifests:
+    - k8s/deployment.yaml
+`
+	expectedOutput := fmt.Sprintf(`apiVersion: %s
+kind: Config
+build:
+  artifacts:
+  - image: docker/image
+    docker:
+      dockerfile: dockerfile.test
+test:
+- image: docker/image
+  structureTests:
+  - ./test/*
+deploy:
+  kubectl:
+    manifests:
+    - k8s/deployment.yaml
+`, latest.Version)
+
+	cfgFile, teardown := testutil.TempFile(t, "config", []byte(inputYaml))
+	defer teardown()
+
+	var b bytes.Buffer
+	err := runFix(&b, cfgFile, true)
+
+	output, _ := ioutil.ReadFile(cfgFile)
+
+	testutil.CheckErrorAndDeepEqual(t, false, err, expectedOutput, string(output))
+}

cmd/skaffold/app/cmd/init.go

@@ -106,7 +106,7 @@ func doInit(out io.Writer) error {
 
 	for _, file := range potentialConfigs {
 		if !force {
-			config, err := schema.ParseConfig(file, true)
+			config, err := schema.ParseConfig(file, false)
 			if err == nil && config != nil {
 				return fmt.Errorf("pre-existing %s found", file)
 			}

cmd/skaffold/app/cmd/run.go

@@ -46,7 +46,7 @@ func run(out io.Writer) error {
 	defer cancel()
 	catchCtrlC(cancel)
 
-	runner, config, err := newRunner(out, opts)
+	runner, config, err := newRunner(opts)
 	if err != nil {
 		return errors.Wrap(err, "creating runner")
 	}

cmd/skaffold/app/cmd/runner.go

@@ -17,8 +17,6 @@ limitations under the License.
 package cmd
 
 import (
-	"io"
-
 	configutil "github.com/GoogleContainerTools/skaffold/cmd/skaffold/app/cmd/config"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/config"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner"
@@ -29,16 +27,14 @@ import (
 )
 
 // newRunner creates a SkaffoldRunner and returns the SkaffoldPipeline associated with it.
-func newRunner(out io.Writer, opts *config.SkaffoldOptions) (*runner.SkaffoldRunner, *latest.SkaffoldPipeline, error) {
+func newRunner(opts *config.SkaffoldOptions) (*runner.SkaffoldRunner, *latest.SkaffoldPipeline, error) {
 	parsed, err := schema.ParseConfig(opts.ConfigurationFile, true)
 	if err != nil {
 		return nil, nil, errors.Wrap(err, "parsing skaffold config")
 	}
 
-	// automatically upgrade older config
-	parsed, err = schema.UpgradeToLatest(out, parsed)
-	if err != nil {
-		return nil, nil, errors.Wrap(err, "invalid config")
+	if err := parsed.SetDefaultValues(); err != nil {
+		return nil, nil, errors.Wrap(err, "setting default values")
 	}
 
 	config := parsed.(*latest.SkaffoldPipeline)

pkg/skaffold/schema/latest/defaults_test.go

@@ -0,0 +1,37 @@
+/*
+Copyright 2018 The Skaffold Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package latest
+
+import (
+	"testing"
+
+	"github.com/GoogleContainerTools/skaffold/testutil"
+)
+
+func TestDetDefaults(t *testing.T) {
+	pipeline := SkaffoldPipeline{
+		Build: BuildConfig{
+			Artifacts: []*Artifact{
+				{ImageName: "image"},
+			},
+		},
+	}
+
+	err := pipeline.SetDefaultValues()
+
+	testutil.CheckError(t, false, err)
+}

pkg/skaffold/schema/util/util.go

@@ -18,6 +18,6 @@ package util
 
 type VersionedConfig interface {
 	GetVersion() string
-	Parse([]byte, bool) error
+	SetDefaultValues() error
 	Upgrade() (VersionedConfig, error)
 }

pkg/skaffold/schema/v1alpha1/config.go

@@ -17,10 +17,7 @@ limitations under the License.
 package v1alpha1
 
 import (
-	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/util"
-
-	yaml "gopkg.in/yaml.v2"
 )
 
 const Version string = "skaffold/v1alpha1"
@@ -112,41 +109,3 @@ type Artifact struct {
 	Workspace      string             `yaml:"workspace"`
 	BuildArgs      map[string]*string `yaml:"buildArgs,omitempty"`
 }
-
-// DefaultDevSkaffoldPipeline is a partial set of defaults for the SkaffoldPipeline
-// when dev mode is specified.
-// Each API is responsible for setting its own defaults that are not top level.
-var defaultDevSkaffoldPipeline = &SkaffoldPipeline{
-	Build: BuildConfig{
-		TagPolicy: constants.DefaultDevTagStrategy,
-	},
-}
-
-// DefaultRunSkaffoldPipeline is a partial set of defaults for the SkaffoldPipeline
-// when run mode is specified.
-// Each API is responsible for setting its own defaults that are not top level.
-var defaultRunSkaffoldPipeline = &SkaffoldPipeline{
-	Build: BuildConfig{
-		TagPolicy: constants.DefaultRunTagStrategy,
-	},
-}
-
-// Parse reads from an io.Reader and unmarshals the result into a SkaffoldPipeline.
-// The default config argument provides default values for the config,
-// which can be overridden if present in the config file.
-func (config *SkaffoldPipeline) Parse(contents []byte, useDefault bool) error {
-	if useDefault {
-		*config = *config.getDefaultForMode(false)
-	} else {
-		*config = SkaffoldPipeline{}
-	}
-
-	return yaml.UnmarshalStrict(contents, config)
-}
-
-func (config *SkaffoldPipeline) getDefaultForMode(dev bool) *SkaffoldPipeline {
-	if dev {
-		return defaultDevSkaffoldPipeline
-	}
-	return defaultRunSkaffoldPipeline
-}