Support skaffold fix —version skaffold/v1 (#4016)

* Skaffold fix —version

Fixes #3613

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

* Fix doc

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

* feedback

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

Author: dgageot

cmd/skaffold/app/cmd/find_configs.go

@@ -89,7 +89,7 @@ func findConfigs(directory string) (map[string]string, error) {
 	}
 
 	err := walk.From(directory).When(isYaml).Do(func(path string, _ walk.Dirent) error {
-		if cfg, err := schema.ParseConfig(path, false); err == nil {
+		if cfg, err := schema.ParseConfig(path); err == nil {
 			pathToVersion[path] = cfg.GetVersion()
 		}
 		return nil

cmd/skaffold/app/cmd/fix.go

@@ -32,22 +32,27 @@ import (
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/validation"
 )
 
+var toVersion string
+
 func NewCmdFix() *cobra.Command {
 	return NewCmd("fix").
-		WithDescription("Update old configuration to newest schema version").
+		WithDescription("Update old configuration to a newer schema version").
+		WithExample("Update \"skaffold.yaml\" in the current folder to the latest version", "fix").
+		WithExample("Update \"skaffold.yaml\" in the current folder to version \"skaffold/v1\"", "fix --version skaffold/v1").
 		WithCommonFlags().
 		WithFlags(func(f *pflag.FlagSet) {
 			f.BoolVar(&overwrite, "overwrite", false, "Overwrite original config with fixed config")
+			f.StringVar(&toVersion, "version", latest.Version, "Target schema version to upgrade to")
 		}).
 		NoArgs(doFix)
 }
 
 func doFix(_ context.Context, out io.Writer) error {
-	return fix(out, opts.ConfigurationFile, overwrite)
+	return fix(out, opts.ConfigurationFile, toVersion, overwrite)
 }
 
-func fix(out io.Writer, configFile string, overwrite bool) error {
-	cfg, err := schema.ParseConfig(configFile, false)
+func fix(out io.Writer, configFile string, toVersion string, overwrite bool) error {
+	cfg, err := schema.ParseConfig(configFile)
 	if err != nil {
 		return err
 	}
@@ -57,7 +62,7 @@ func fix(out io.Writer, configFile string, overwrite bool) error {
 		return nil
 	}
 
-	cfg, err = schema.ParseConfig(configFile, true)
+	cfg, err = schema.ParseConfigAndUpgrade(configFile, toVersion)
 	if err != nil {
 		return err
 	}

cmd/skaffold/app/cmd/fix_test.go

@@ -113,7 +113,7 @@ kind: Config
 			cfgFile := t.TempFile("config", []byte(test.inputYaml))
 
 			var b bytes.Buffer
-			err := fix(&b, cfgFile, false)
+			err := fix(&b, cfgFile, latest.Version, false)
 
 			t.CheckErrorAndDeepEqual(test.shouldErr, err, test.output, b.String())
 		})
@@ -158,7 +158,7 @@ deploy:
 		cfgFile := t.TempFile("config", []byte(inputYaml))
 
 		var b bytes.Buffer
-		err := fix(&b, cfgFile, true)
+		err := fix(&b, cfgFile, latest.Version, true)
 
 		output, _ := ioutil.ReadFile(cfgFile)
 

cmd/skaffold/app/cmd/runner.go

@@ -51,7 +51,7 @@ func withRunner(ctx context.Context, action func(runner.Runner, *latest.Skaffold
 
 // createNewRunner creates a Runner and returns the SkaffoldConfig associated with it.
 func createNewRunner(opts config.SkaffoldOptions) (runner.Runner, *latest.SkaffoldConfig, error) {
-	parsed, err := schema.ParseConfig(opts.ConfigurationFile, true)
+	parsed, err := schema.ParseConfigAndUpgrade(opts.ConfigurationFile, latest.Version)
 	if err != nil {
 		if os.IsNotExist(errors.Unwrap(err)) {
 			return nil, nil, fmt.Errorf("[%s] not found. You might need to run `skaffold init`", opts.ConfigurationFile)

docs/content/en/docs/references/cli/_index.md

@@ -79,7 +79,7 @@ Pipeline building blocks for CI/CD:
 
 Getting started with a new project:
   init              [alpha] Generate configuration for deploying an application
-  fix               Update old configuration to newest schema version
+  fix               Update old configuration to a newer schema version
 
 Other Commands:
   completion        Output shell completion for the given shell (bash or zsh)
@@ -584,14 +584,22 @@ Env vars:
 
 ### skaffold fix
 
-Update old configuration to newest schema version
+Update old configuration to a newer schema version
 
 ```
 
 
+Examples:
+  # Update "skaffold.yaml" in the current folder to the latest version
+  skaffold fix
+
+  # Update "skaffold.yaml" in the current folder to version "skaffold/v1"
+  skaffold fix --version skaffold/v1
+
 Options:
   -f, --filename='skaffold.yaml': Path or URL to the Skaffold config file
       --overwrite=false: Overwrite original config with fixed config
+      --version='skaffold/v2beta2': Target schema version to upgrade to
 
 Usage:
   skaffold fix [options]
@@ -604,6 +612,7 @@ Env vars:
 
 * `SKAFFOLD_FILENAME` (same as `--filename`)
 * `SKAFFOLD_OVERWRITE` (same as `--overwrite`)
+* `SKAFFOLD_VERSION` (same as `--version`)
 
 ### skaffold init
 

pkg/skaffold/initializer/init_test.go

@@ -227,10 +227,10 @@ func strip(s string) string {
 }
 
 func checkGeneratedConfig(t *testutil.T, dir string) {
-	expectedOutput, err := schema.ParseConfig(filepath.Join(dir, "skaffold.yaml"), false)
+	expectedOutput, err := schema.ParseConfig(filepath.Join(dir, "skaffold.yaml"))
 	t.CheckNoError(err)
 
-	output, err := schema.ParseConfig(filepath.Join(dir, "skaffold.yaml.out"), false)
+	output, err := schema.ParseConfig(filepath.Join(dir, "skaffold.yaml.out"))
 	t.CheckNoError(err)
 	t.CheckDeepEqual(expectedOutput, output)
 }

pkg/skaffold/runner/generate_pipeline.go

@@ -71,7 +71,7 @@ func setupConfigFiles(configPaths []string) ([]*pipeline.ConfigFile, error) {
 	// Read all given config files to read contents into SkaffoldConfig
 	var configFiles []*pipeline.ConfigFile
 	for _, path := range configPaths {
-		parsed, err := schema.ParseConfig(path, true)
+		parsed, err := schema.ParseConfigAndUpgrade(path, latest.Version)
 		if err != nil {
 			return nil, fmt.Errorf("parsing config %q: %w", path, err)
 		}

pkg/skaffold/schema/latest/upgrade.go

@@ -24,5 +24,5 @@ import (
 
 // Upgrade upgrades a configuration to the next version.
 func (c *SkaffoldConfig) Upgrade() (util.VersionedConfig, error) {
-	return nil, errors.New("not implemented yet")
+	return nil, errors.New("there's no version to upgrade from \"latest\"")
 }

pkg/skaffold/schema/profiles_test.go

@@ -62,7 +62,7 @@ profiles:
 		tmpDir := t.NewTempDir().
 			Write("skaffold.yaml", addVersion(config))
 
-		parsed, err := ParseConfig(tmpDir.Path("skaffold.yaml"), false)
+		parsed, err := ParseConfig(tmpDir.Path("skaffold.yaml"))
 		t.CheckNoError(err)
 
 		skaffoldConfig := parsed.(*latest.SkaffoldConfig)
@@ -93,7 +93,7 @@ profiles:
 		tmp := t.NewTempDir().
 			Write("skaffold.yaml", addVersion(config))
 
-		parsed, err := ParseConfig(tmp.Path("skaffold.yaml"), false)
+		parsed, err := ParseConfig(tmp.Path("skaffold.yaml"))
 		t.CheckNoError(err)
 
 		skaffoldConfig := parsed.(*latest.SkaffoldConfig)
@@ -666,7 +666,7 @@ profiles:
 		tmpDir := t.NewTempDir().
 			Write("skaffold.yaml", addVersion(config))
 
-		parsed, err := ParseConfig(tmpDir.Path("skaffold.yaml"), false)
+		parsed, err := ParseConfig(tmpDir.Path("skaffold.yaml"))
 		t.RequireNoError(err)
 
 		skaffoldConfig := parsed.(*latest.SkaffoldConfig)

pkg/skaffold/schema/samples_test.go

@@ -75,7 +75,7 @@ func TestParseSamples(t *testing.T) {
 
 func checkSkaffoldConfig(t *testutil.T, yaml []byte) {
 	configFile := t.TempFile("skaffold.yaml", yaml)
-	cfg, err := ParseConfig(configFile, true)
+	cfg, err := ParseConfigAndUpgrade(configFile, latest.Version)
 	t.CheckNoError(err)
 
 	err = defaults.Set(cfg.(*latest.SkaffoldConfig))

pkg/skaffold/schema/versions.go

@@ -120,14 +120,14 @@ func IsSkaffoldConfig(file string) bool {
 		return false
 	}
 
-	if config, err := ParseConfig(file, false); err == nil && config != nil {
+	if config, err := ParseConfig(file); err == nil && config != nil {
 		return true
 	}
 	return false
 }
 
 // ParseConfig reads a configuration file.
-func ParseConfig(filename string, upgrade bool) (util.VersionedConfig, error) {
+func ParseConfig(filename string) (util.VersionedConfig, error) {
 	buf, err := misc.ReadConfiguration(filename)
 	if err != nil {
 		return nil, fmt.Errorf("read skaffold config: %w", err)
@@ -146,7 +146,7 @@ func ParseConfig(filename string, upgrade bool) (util.VersionedConfig, error) {
 
 	factory, present := SchemaVersions.Find(apiVersion.Version)
 	if !present {
-		return nil, fmt.Errorf("unknown api version: '%s'", apiVersion.Version)
+		return nil, fmt.Errorf("unknown api version: %q", apiVersion.Version)
 	}
 
 	// Remove all top-level keys starting with `.` so they can be used as YAML anchors
@@ -169,42 +169,46 @@ func ParseConfig(filename string, upgrade bool) (util.VersionedConfig, error) {
 		return nil, fmt.Errorf("unable to parse config: %w", err)
 	}
 
-	if upgrade && cfg.GetVersion() != latest.Version {
-		cfg, err = upgradeToLatest(cfg)
-		if err != nil {
-			return nil, err
-		}
-	}
-
 	return cfg, nil
 }
 
-// upgradeToLatest upgrades a configuration to the latest version.
-func upgradeToLatest(vc util.VersionedConfig) (util.VersionedConfig, error) {
-	var err error
+// ParseConfigAndUpgrade reads a configuration file and upgrades it to a given version.
+func ParseConfigAndUpgrade(filename, toVersion string) (util.VersionedConfig, error) {
+	cfg, err := ParseConfig(filename)
+	if err != nil {
+		return nil, err
+	}
+
+	// Check that the target version exists
+	if _, present := SchemaVersions.Find(toVersion); !present {
+		return nil, fmt.Errorf("unknown api version: %q", toVersion)
+	}
 
-	// first, check to make sure config version isn't too new
-	version, err := apiversion.Parse(vc.GetVersion())
+	// Check that the config's version is not newer than the target version
+	currentVersion, err := apiversion.Parse(cfg.GetVersion())
 	if err != nil {
-		return nil, fmt.Errorf("parsing api version: %w", err)
+		return nil, err
+	}
+	targetVersion, err := apiversion.Parse(toVersion)
+	if err != nil {
+		return nil, err
 	}
 
-	semver := apiversion.MustParse(latest.Version)
-	if version.EQ(semver) {
-		return vc, nil
+	if currentVersion.EQ(targetVersion) {
+		return cfg, nil
 	}
-	if version.GT(semver) {
-		return nil, fmt.Errorf("config version %s is too new for this version: upgrade Skaffold", vc.GetVersion())
+	if currentVersion.GT(targetVersion) {
+		return nil, fmt.Errorf("config version %q is more recent than target version %q: upgrade Skaffold", cfg.GetVersion(), toVersion)
 	}
 
-	logrus.Debugf("config version (%s) out of date: upgrading to latest (%s)", vc.GetVersion(), latest.Version)
+	logrus.Debugf("config version %q out of date: upgrading to latest %q", cfg.GetVersion(), toVersion)
 
-	for vc.GetVersion() != latest.Version {
-		vc, err = vc.Upgrade()
+	for cfg.GetVersion() != toVersion {
+		cfg, err = cfg.Upgrade()
 		if err != nil {
 			return nil, fmt.Errorf("transforming skaffold config: %w", err)
 		}
 	}
 
-	return vc, nil
+	return cfg, nil
 }

pkg/skaffold/schema/versions_test.go

@@ -149,7 +149,7 @@ deploy:
 	}
 }
 
-func TestParseConfig(t *testing.T) {
+func TestParseConfigAndUpgrade(t *testing.T) {
 	tests := []struct {
 		apiVersion  string
 		description string
@@ -273,7 +273,7 @@ func TestParseConfig(t *testing.T) {
 			tmpDir := t.NewTempDir().
 				Write("skaffold.yaml", fmt.Sprintf("apiVersion: %s\nkind: Config\n%s", test.apiVersion, test.config))
 
-			cfg, err := ParseConfig(tmpDir.Path("skaffold.yaml"), true)
+			cfg, err := ParseConfigAndUpgrade(tmpDir.Path("skaffold.yaml"), latest.Version)
 			if cfg != nil {
 				config := cfg.(*latest.SkaffoldConfig)
 				err := defaults.Set(config)
@@ -472,3 +472,27 @@ func TestCantUpgradeFromLatestVersion(t *testing.T) {
 	_, err := factory().Upgrade()
 	testutil.CheckError(t, true, err)
 }
+
+func TestParseConfigAndUpgradeToUnknownVersion(t *testing.T) {
+	testutil.Run(t, "", func(t *testutil.T) {
+		t.NewTempDir().
+			Write("skaffold.yaml", fmt.Sprintf("apiVersion: %s\nkind: Config\n%s", latest.Version, minimalConfig)).
+			Chdir()
+
+		_, err := ParseConfigAndUpgrade("skaffold.yaml", "unknown")
+
+		t.CheckErrorContains(`unknown api version: "unknown"`, err)
+	})
+}
+
+func TestParseConfigAndUpgradeToOlderVersion(t *testing.T) {
+	testutil.Run(t, "", func(t *testutil.T) {
+		t.NewTempDir().
+			Write("skaffold.yaml", fmt.Sprintf("apiVersion: %s\nkind: Config\n%s", latest.Version, minimalConfig)).
+			Chdir()
+
+		_, err := ParseConfigAndUpgrade("skaffold.yaml", "skaffold/v1alpha1")
+
+		t.CheckErrorContains(`is more recent than target version "skaffold/v1alpha1": upgrade Skaffold`, err)
+	})
+}