fix: (helm) Add expand env template for dependsOn, fix concurrent ins…

…tallation (#9689) * fix(helm): Add expand env template for dependsOn and fix concurrent installation * tests Signed-off-by: Suleiman Dibirov <idsulik@gmail.com> --------- Signed-off-by: Suleiman Dibirov <idsulik@gmail.com>
GoogleContainerTools · Jan 31, 2025 · dcc573f · dcc573f
1 parent acad129
commit dcc573f
Show file tree

Hide file tree

Showing 4 changed files with 60 additions and 22 deletions.
diff --git a/pkg/skaffold/deploy/helm/helm.go b/pkg/skaffold/deploy/helm/helm.go
@@ -285,40 +285,42 @@ func (h *Deployer) Deploy(ctx context.Context, out io.Writer, builds []graph.Art
 	// Group releases by their dependency level to deploy them in the correct order.
 	levelGroups := groupReleasesByLevel(deploymentOrder, dependencyGraph)
 
-	g, levelCtx := errgroup.WithContext(ctx)
-
+	var concurrency int
 	if h.Concurrency == nil || *h.Concurrency == 1 {
-		g.SetLimit(1)
+		concurrency = 1
 		olog.Entry(ctx).Infof("Installing %d releases sequentially", len(h.Releases))
 	} else {
-		g.SetLimit(*h.Concurrency)
+		if *h.Concurrency == 0 {
+			concurrency = -1
+		} else {
+			concurrency = *h.Concurrency
+		}
 		olog.Entry(ctx).Infof("Installing %d releases concurrently", len(h.Releases))
 	}
 
 	releaseNameToRelease := make(map[string]latest.HelmRelease)
 	for _, r := range h.Releases {
-		releaseName, err := util.ExpandEnvTemplateOrFail(r.Name, nil)
-		if err != nil {
-			return fmt.Errorf("cannot parse the release name template: %w", err)
-		}
-		releaseNameToRelease[releaseName] = r
+		releaseNameToRelease[r.Name] = r
 	}
 
 	// Process each level in order
 	for level, releases := range levelGroups {
 		if len(levelGroups) > 1 {
-			olog.Entry(ctx).Infof("Installing level %d/%d releases (%d releases)", level, len(levelGroups), len(releases))
+			olog.Entry(ctx).Infof("Installing level %d/%d releases (%d releases)", level+1, len(levelGroups), len(releases))
 		} else {
 			olog.Entry(ctx).Infof("Installing releases (%d releases)", len(releases))
 		}
 
+		g, levelCtx := errgroup.WithContext(ctx)
+		g.SetLimit(concurrency)
+
 		// sort releases in the same level, this is merely to ensure that the series of helm commands are in order for
 		// consistency in unit testing.
 		sort.Strings(releases)
 
 		// Deploy releases in current level
-		for _, releaseName := range releases {
-			release := releaseNameToRelease[releaseName]
+		for _, name := range releases {
+			release := releaseNameToRelease[name]
 
 			g.Go(func() error {
 				chartVersion, err := util.ExpandEnvTemplateOrFail(release.Version, nil)
@@ -336,6 +338,11 @@ func (h *Deployer) Deploy(ctx context.Context, out io.Writer, builds []graph.Art
 					return helm.UserErr(fmt.Sprintf("cannot expand chart path %q", release.ChartPath), err)
 				}
 
+				releaseName, err := util.ExpandEnvTemplateOrFail(release.Name, nil)
+				if err != nil {
+					return helm.UserErr(fmt.Sprintf("cannot expand release name %q", release.Name), err)
+				}
+
 				m, results, err := h.deployRelease(levelCtx, out, releaseName, release, builds, h.bV, chartVersion, repo)
 				if err != nil {
 					return helm.UserErr(fmt.Sprintf("deploying %q", releaseName), err)

diff --git a/pkg/skaffold/deploy/helm/helm_test.go b/pkg/skaffold/deploy/helm/helm_test.go
@@ -66,6 +66,21 @@ var testDeployConfig = latest.LegacyHelmDeploy{
 	}},
 }
 
+var testDeployWithDependsOnConfig = latest.LegacyHelmDeploy{
+	Releases: []latest.HelmRelease{{
+		Name:      "skaffold-helm-{{.FOO}}",
+		ChartPath: "examples/test",
+		Overrides: schemautil.HelmOverrides{Values: map[string]interface{}{"foo": "bar"}},
+		SetValues: map[string]string{
+			"some.key": "somevalue",
+		},
+		DependsOn: []string{"other-{{.FOO}}"},
+	}, {
+		Name:      "other-{{.FOO}}",
+		ChartPath: "examples/test",
+	}},
+}
+
 var testDeployNamespacedConfig = latest.LegacyHelmDeploy{
 	Releases: []latest.HelmRelease{{
 		Name:      "skaffold-helm",
@@ -446,6 +461,24 @@ func TestHelmDeploy(t *testing.T) {
 			builds:             testBuilds,
 			expectedNamespaces: []string{""},
 		},
+		{
+			description: "helm3.1 deploy with dependsOn success",
+			commands: testutil.
+				CmdRunWithOutput("helm version --client", version31).
+				AndRun("helm --kube-context kubecontext get all other-FOOBAR --kubeconfig kubeconfig").
+				AndRun("helm --kube-context kubecontext dep build examples/test --kubeconfig kubeconfig").
+				AndRunEnv("helm --kube-context kubecontext upgrade other-FOOBAR examples/test --post-renderer SKAFFOLD-BINARY --kubeconfig kubeconfig",
+					[]string{"SKAFFOLD_FILENAME=test.yaml", "SKAFFOLD_CMDLINE=filter --kube-context kubecontext --build-artifacts TMPFILE --kubeconfig kubeconfig"}).
+				AndRunWithOutput("helm --kube-context kubecontext get all other-FOOBAR --template {{.Release.Manifest}} --kubeconfig kubeconfig", validDeployYaml).
+				AndRun("helm --kube-context kubecontext get all skaffold-helm-FOOBAR --kubeconfig kubeconfig").
+				AndRun("helm --kube-context kubecontext dep build examples/test --kubeconfig kubeconfig").
+				AndRunEnv("helm --kube-context kubecontext upgrade skaffold-helm-FOOBAR examples/test --post-renderer SKAFFOLD-BINARY --set some.key=somevalue -f skaffold-overrides.yaml --kubeconfig kubeconfig",
+					[]string{"SKAFFOLD_FILENAME=test.yaml", "SKAFFOLD_CMDLINE=filter --kube-context kubecontext --build-artifacts TMPFILE --kubeconfig kubeconfig"}).
+				AndRunWithOutput("helm --kube-context kubecontext get all skaffold-helm-FOOBAR --template {{.Release.Manifest}} --kubeconfig kubeconfig", validDeployYaml),
+			helm:               testDeployWithDependsOnConfig,
+			builds:             testBuilds,
+			expectedNamespaces: []string{""},
+		},
 		{
 			description: "helm3.1 namespaced context deploy success",
 			commands: testutil.

diff --git a/pkg/skaffold/deploy/helm/util.go b/pkg/skaffold/deploy/helm/util.go
@@ -19,19 +19,13 @@ package helm
 import (
 	"fmt"
 
-	"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/helm"
 	"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/schema/latest"
-	"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/util"
 )
 
 func BuildDependencyGraph(releases []latest.HelmRelease) (map[string][]string, error) {
 	dependencyGraph := make(map[string][]string)
 	for _, r := range releases {
-		releaseName, err := util.ExpandEnvTemplateOrFail(r.Name, nil)
-		if err != nil {
-			return nil, helm.UserErr(fmt.Sprintf("cannot expand release name %q", r.Name), err)
-		}
-		dependencyGraph[releaseName] = r.DependsOn
+		dependencyGraph[r.Name] = r.DependsOn
 	}
 
 	return dependencyGraph, nil

diff --git a/pkg/skaffold/deploy/helm/util_test.go b/pkg/skaffold/deploy/helm/util_test.go
@@ -67,11 +67,15 @@ func TestBuildDependencyGraph(t *testing.T) {
 			},
 		},
 		{
-			description: "invalid release name template",
+			description: "simple dependency graph with placeholder in name",
 			releases: []latest.HelmRelease{
-				{Name: "{{.Invalid}}"},
+				{Name: "release1-{{.Service}}", DependsOn: []string{"release2-{{.Service}}"}},
+				{Name: "release2-{{.Service}}", DependsOn: []string{}},
+			},
+			expected: map[string][]string{
+				"release1-{{.Service}}": {"release2-{{.Service}}"},
+				"release2-{{.Service}}": {},
 			},
-			shouldErr: true,
 		},
 	}