Should `skaffold fix -version` also upgrade the dependent skaffold.yaml?

[yuwenma]

Updated from the discussion with @gsquared94 (See below).

Information

• Skaffold version: v2beta15

  Actual behavior

  skaffolx fix --version VERSION only upgrades the root skaffold.yaml (or --filename if given). Should it also upgrade the local dependencies' skaffold.yaml?

Expected behavior

When users run skaffold fix [--version VERSION], they may not only want to upgrade the root skaffold.yaml, but the sub-directories' skaffold.yaml. Especially if the root skaffold.yaml is out of date, the sub-directories skaffold.yaml are more likely to be out of date as well.

Proposal

1. Change the skaffold fix behavior to upgrade both the root skaffold.yaml and local dependent skaffold.yaml files (which are defined in .requires.path). @gsquared94 @tejal29 do you think we shall make this as the default behavior? Gaurav: add flag --recursive default to true.
2. If any APIVersions are newer than the --version flag, do no-op for that skaffold.yaml rather than abort the whole skaffold fix.

Original question

Should skaffold fix -version abort or do no-op?

Description

In skaffolx fix --version VERSION, if VERSION is older than in use version, skaffold aborts the attempts with message. e.g.

skaffold fix --version v2beta12
config version v2beta14 is more recent than target version v2beta12: upgrade Skaffold

Here is the source code.

As a consequence, in multi-config mode, users have no chance to use skaffold fix --version to upgrade their older versions if they also have some newer versions in use. Is this expected? Should skaffold fix --version do no-op if the in-use versions is equal or newer than the VERSION?

[gsquared94]

Can you provide an example of how this fails in multi-config mode? I think skaffold fix only updates configs in the current skaffold.yaml file.

[yuwenma]

@gsquared94 You are right, I guess I'm confused by this code: The skaffold fix has a returned config slices which iterate the current skaffold.yaml and try to find all APIVersion from the skaffold.yaml. But it looks like the APISchema actually only have one single APIVersion field. Why does configFactoryFromAPIVersion assumes the parsing file have a list of APIVersions?

[yuwenma]

@gsquared94 Another question: Why does skaffold fix only upgrade the current skaffold.yaml, rather than upgrade all its dependency skaffold.yaml files? Seems nicer for users to have a single cmd upgrading all files (especially if the root skaffold.yaml is older than the expected, it is more likely its sub-dir skaffold.yaml files also need an update).

[gsquared94]

Why does configFactoryFromAPIVersion assumes the parsing file have a list of APIVersions?

There can be a slice of config definitions in a single file with different versions, like rewriting examples/microservices skaffold.yaml:

apiVersion: skaffold/v2beta12
kind: Config
build:
  artifacts:
    - image: base
      context: base
---
apiVersion: skaffold/v2beta13
kind: Config
requires:
  - configs: [base]
build:
  artifacts:
    - image: leeroy-web
      context: leeroy-web
      requires:
        - image: base
          alias: BASE
    - image: leeroy-app
      context: leeroy-app
      requires:
        - image: base
          alias: BASE

[gsquared94]

@gsquared94 Another question: Why does skaffold fix only upgrade the current skaffold.yaml, rather than upgrade all its dependency skaffold.yaml files? Seems nicer for users to have a single cmd upgrading all files (especially if the root skaffold.yaml is older than the expected, it is more likely its sub-dir skaffold.yaml files also need an update).

There's no reason why it can't do that. But your original question needs to be addressed then, whether we upgrade all files and skip (instead of failing) for files that are at a version higher than target. Also there should be a way to only upgrade single files, so maybe a --all or --recursive flag or something.

[gsquared94]

Updated proposal looks good to me. Please also add a flag --recursive with default value true that users can disable to go back to existing behavior.