fix: add proper artifactOverrides->setValueTemplates conversion when …

…upgrading from v2beta29

integration/fix_test.go

@@ -51,6 +51,14 @@ func TestFixStdout(t *testing.T) {
 	skaffold.Run().WithConfig("-").InDir("testdata/fix").InNs(ns.Name).WithStdin(out).RunOrFail(t)
 }
 
+func TestFixV2Beta29ToV3Alpha1PatchProfiles(t *testing.T) {
+	MarkIntegrationTest(t, CanRunWithoutGcp)
+
+	out, err := skaffold.Fix().InDir("testdata/fix-v2beta29").RunWithCombinedOutput(t)
+	testutil.CheckError(t, false, err)
+	testutil.CheckContains(t, "/manifests/helm/releases/0/setValueTemplates/", string(out))
+}
+
 func TestFixOutputFile(t *testing.T) {
 	// TODO: Fix https://github.com/GoogleContainerTools/skaffold/issues/7033.
 	t.Skipf("Fix https://github.com/GoogleContainerTools/skaffold/issues/7033")

integration/testdata/fix-v2beta29/skaffold.yaml

@@ -0,0 +1,24 @@
+apiVersion: skaffold/v2beta29
+kind: Config
+build:
+  artifacts:
+  - image: skaffold-helm
+deploy:
+ helm:
+   releases:
+     - name: skaffold-helm
+       chartPath: charts
+       namespace: repro-ns
+       recreatePods: false
+       useHelmSecrets: false
+       wait: false
+       artifactOverrides:
+         image: skaffold-helm
+       imageStrategy:
+         helm: {}
+profiles:
+ - name: skaffold-helm-v2
+   patches:
+      - op: replace
+        path: /deploy/helm/releases/0/artifactOverrides/image
+        value: skaffold-helm-v2

pkg/skaffold/schema/v2beta29/upgrade.go

@@ -20,16 +20,21 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"regexp"
+	"strconv"
 	"strings"
 
 	"github.com/pkg/errors"
+	"gopkg.in/yaml.v2"
 
 	"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/output/log"
 	"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/schema/util"
 	next "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/schema/v3alpha1"
 	pkgutil "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/util"
 )
 
+var artifactOverridesRegexp = regexp.MustCompile("/deploy/helm/releases/[0-9]+/artifactOverrides/image")
+
 var migrations = map[string]string{
 	"/deploy/kubectl":             "/manifests/rawYaml",
 	"/deploy/kustomize/paths":     "/manifests/kustomize/paths",
@@ -45,28 +50,57 @@ func (c *SkaffoldConfig) Upgrade() (util.VersionedConfig, error) {
 	pkgutil.CloneThroughJSON(c, &newConfig)
 	newConfig.APIVersion = next.Version
 
-	err := util.UpgradePipelines(c, &newConfig, upgradeOnePipeline)
+	var nProfiles []next.Profile
+	// seed with existing Profiles
+	if c.Profiles != nil {
+		pkgutil.CloneThroughJSON(newConfig.Profiles, &nProfiles)
+	}
+
+	oldPatchesMap := map[string][]JSONPatch{}
+	newPatchesMap := map[string][]next.JSONPatch{}
+	for i, p := range c.Profiles {
+		oldPatchesMap[p.Name], newPatchesMap[p.Name] = p.Patches, nProfiles[i].Patches
+	}
+	onePipelineUpgrader := NewOnePipelineUpgrader(oldPatchesMap, newPatchesMap)
+
+	err := util.UpgradePipelines(c, &newConfig, onePipelineUpgrader.upgradeOnePipeline)
 	if err != nil {
 		return &newConfig, err
 	}
-
 	var newProfiles []next.Profile
 	// seed with existing Profiles
 	if c.Profiles != nil {
 		pkgutil.CloneThroughJSON(newConfig.Profiles, &newProfiles)
 	}
 
+	for i := range newProfiles {
+		if _, ok := newPatchesMap[newProfiles[i].Name]; ok {
+			newProfiles[i].Patches = newPatchesMap[newProfiles[i].Name]
+		}
+	}
+
 	// Update profiles patches
 	for i, p := range c.Profiles {
 		upgradePatches(p.Patches, newProfiles[i].Patches)
 	}
 
 	newConfig.Profiles = newProfiles
-
 	return &newConfig, nil
 }
 
-func upgradeOnePipeline(oldPipeline, newPipeline interface{}) error {
+type OnePipelineUpgrader struct {
+	oldPatchesMap map[string][]JSONPatch
+	newPatchesMap map[string][]next.JSONPatch
+}
+
+func NewOnePipelineUpgrader(oldPatchesMap map[string][]JSONPatch, newPatchesMap map[string][]next.JSONPatch) OnePipelineUpgrader {
+	return OnePipelineUpgrader{
+		oldPatchesMap: oldPatchesMap,
+		newPatchesMap: newPatchesMap,
+	}
+}
+
+func (opu *OnePipelineUpgrader) upgradeOnePipeline(oldPipeline, newPipeline interface{}) error {
 	oldPL := oldPipeline.(*Pipeline)
 	newPL := newPipeline.(*next.Pipeline)
 
@@ -151,12 +185,112 @@ func upgradeOnePipeline(oldPipeline, newPipeline interface{}) error {
 		newPL.Deploy.LegacyHelmDeploy = &next.LegacyHelmDeploy{}
 		newPL.Deploy.LegacyHelmDeploy.LifecycleHooks = newHelm.LifecycleHooks
 	}
+
+	err := upgradeArtifactOverridesPatches(opu.oldPatchesMap, opu.newPatchesMap, oldPL)
+	if err != nil {
+		return fmt.Errorf("error converting helm artifactOverrides during version upgrade: %w", err)
+	}
+
+	return nil
+}
+
+func upgradeArtifactOverridesPatches(oldsMap map[string][]JSONPatch, newsMap map[string][]next.JSONPatch, oldPL *Pipeline) error {
+	for k := range oldsMap {
+		for i, old := range oldsMap[k] {
+			if artifactOverridesRegexp.Match([]byte(old.Path)) {
+				idx, err := strconv.Atoi(strings.Split(old.Path, "/")[4])
+				if err != nil {
+					return err
+				}
+				newPathModified := strings.ReplaceAll(strings.ReplaceAll(old.Path, "/deploy/helm", "/manifests/helm"),
+					"artifactOverrides", "setValueTemplates")
+				if oldPL.Deploy.HelmDeploy != nil {
+					// replace commonly used image name chars that are illegal helm template chars "/" & "-" with "_"
+					validV := pkgutil.SanitizeHelmTemplateValue(old.Value.Node.Value().(string))
+					n := &util.YamlpatchNode{}
+
+					if oldPL.Deploy.HelmDeploy.Releases[idx].ImageStrategy.HelmConventionConfig != nil {
+						err = yaml.Unmarshal([]byte(fmt.Sprintf("\"{{.IMAGE_TAG_%s}}\"", validV)), &n)
+						if err != nil {
+							return err
+						}
+
+						newsMap[k] = append(newsMap[k], next.JSONPatch{
+							Op:    newsMap[k][i].Op,
+							Path:  newPathModified + ".tag",
+							From:  newsMap[k][i].From,
+							Value: n,
+						})
+
+						if oldPL.Deploy.HelmDeploy.Releases[i].ImageStrategy.HelmConventionConfig.ExplicitRegistry {
+							// is 'helm' imageStrategy + explicitRegistry
+							n := &util.YamlpatchNode{}
+							err = yaml.Unmarshal([]byte(fmt.Sprintf("\"{{.IMAGE_DOMAIN_%s}}\"", validV)), &n)
+							if err != nil {
+								return err
+							}
+
+							newsMap[k] = append(newsMap[k], next.JSONPatch{
+								Op:    newsMap[k][i].Op,
+								Path:  newPathModified + ".registry",
+								From:  newsMap[k][i].From,
+								Value: n,
+							})
+
+							n = &util.YamlpatchNode{}
+							err := yaml.Unmarshal([]byte(fmt.Sprintf("\"{{.IMAGE_REPO_NO_DOMAIN_%s}}\"", validV)), &n)
+							if err != nil {
+								return err
+							}
+
+							newsMap[k][i] = next.JSONPatch{
+								Op:    newsMap[k][i].Op,
+								Path:  newPathModified + ".repository",
+								From:  newsMap[k][i].From,
+								Value: n,
+							}
+						} else {
+							// is 'helm' imageStrategy
+							n = &util.YamlpatchNode{}
+							err := yaml.Unmarshal([]byte(fmt.Sprintf("\"{{.IMAGE_REPO_%s}}\"", validV)), &n)
+							if err != nil {
+								return err
+							}
+
+							newsMap[k][i] = next.JSONPatch{
+								Op:    newsMap[k][i].Op,
+								Path:  newPathModified + ".repository",
+								From:  newsMap[k][i].From,
+								Value: n,
+							}
+						}
+					} else {
+						// is 'fqn' imageStrategy
+						// replace commonly used image name chars that are illegal helm template chars "/" & "-" with "_"
+						newValue := fmt.Sprintf("\"{{.IMAGE_FULLY_QUALIFIED_%s}}\"", validV)
+
+						n := &util.YamlpatchNode{}
+						err := yaml.Unmarshal([]byte(newValue), &n)
+						if err != nil {
+							return err
+						}
+						newsMap[k][i].Value = n
+						newsMap[k][i].Path = newPathModified
+					}
+				}
+			}
+		}
+	}
 	return nil
 }
 
 func upgradePatches(olds []JSONPatch, news []next.JSONPatch) {
 	for i, old := range olds {
 		for str, repStr := range migrations {
+			if artifactOverridesRegexp.Match([]byte(old.Path)) {
+				// this is handled by upgradeArtifactOverridesPatches
+				continue
+			}
 			if strings.Contains(old.Path, str) {
 				news[i].Path = strings.ReplaceAll(old.Path, str, repStr)
 			}