From d158d7ca156b118848e52e83c57e8d19477abf0d Mon Sep 17 00:00:00 2001 From: Suleiman Dibirov <3595194+idsulik@users.noreply.github.com> Date: Fri, 31 Jan 2025 20:03:29 +0200 Subject: [PATCH 1/3] fix: (helm) Add expand env template for dependsOn, fix concurrent installation (#9689) * fix(helm): Add expand env template for dependsOn and fix concurrent installation * tests Signed-off-by: Suleiman Dibirov --------- Signed-off-by: Suleiman Dibirov --- pkg/skaffold/deploy/helm/helm.go | 31 +++++++++++++++---------- pkg/skaffold/deploy/helm/helm_test.go | 33 +++++++++++++++++++++++++++ pkg/skaffold/deploy/helm/util.go | 8 +------ pkg/skaffold/deploy/helm/util_test.go | 10 +++++--- 4 files changed, 60 insertions(+), 22 deletions(-) diff --git a/pkg/skaffold/deploy/helm/helm.go b/pkg/skaffold/deploy/helm/helm.go index 3102fd8d952..344477564e5 100644 --- 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 index 15034222854..248d0015261 100644 --- 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 index dbb8e599cbd..e225758179a 100644 --- 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 index 4336f24f30b..a7921f0a810 100644 --- 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, }, } From 4178090099f24172e88c74ae935f810a7202af4f Mon Sep 17 00:00:00 2001 From: Michael Plump Date: Tue, 4 Feb 2025 11:10:11 -0500 Subject: [PATCH 2/3] feat: revert "feat: transform imagePullPolicy when using local cluster (#9495)" (#9703) This backs out commit c1834b6296f5f8a19b64c3c97ade0c11e08e6958. --- pkg/skaffold/deploy/kubectl/kubectl.go | 11 --- .../kubernetes/manifest/image_pull_policy.go | 88 ------------------- .../manifest/image_pull_policy_test.go | 67 -------------- 3 files changed, 166 deletions(-) delete mode 100644 pkg/skaffold/kubernetes/manifest/image_pull_policy.go delete mode 100644 pkg/skaffold/kubernetes/manifest/image_pull_policy_test.go diff --git a/pkg/skaffold/deploy/kubectl/kubectl.go b/pkg/skaffold/deploy/kubectl/kubectl.go index 63faeb870eb..1b5e8fdef37 100644 --- a/pkg/skaffold/deploy/kubectl/kubectl.go +++ b/pkg/skaffold/deploy/kubectl/kubectl.go @@ -241,17 +241,6 @@ func (k *Deployer) Deploy(ctx context.Context, out io.Writer, builds []graph.Art return err } - cluster, err := config.GetCluster(ctx, config.GetClusterOpts{}) - if err != nil { - return err - } - if cluster.Local { - manifests, err = manifests.ReplaceImagePullPolicy(manifest.NewResourceSelectorImagePullPolicy()) - if err != nil { - return err - } - } - childCtx, endTrace = instrumentation.StartTrace(ctx, "Deploy_LoadImages") if err := k.imageLoader.LoadImages(childCtx, out, k.localImages, k.originalImages, builds); err != nil { endTrace(instrumentation.TraceEndError(err)) diff --git a/pkg/skaffold/kubernetes/manifest/image_pull_policy.go b/pkg/skaffold/kubernetes/manifest/image_pull_policy.go deleted file mode 100644 index e03999578b5..00000000000 --- a/pkg/skaffold/kubernetes/manifest/image_pull_policy.go +++ /dev/null @@ -1,88 +0,0 @@ -/* -Copyright 2019 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 manifest - -import apimachinery "k8s.io/apimachinery/pkg/runtime/schema" - -// resourceSelectorImagePullPolicy selects PodSpecs for transforming the imagePullPolicy field -// based on allowlist and denylist rules for their GroupKind and navigation path. -type resourceSelectorImagePullPolicy struct{} - -func NewResourceSelectorImagePullPolicy() ResourceSelector { - return &resourceSelectorImagePullPolicy{} -} - -// allowByGroupKind checks if a GroupKind is allowed for transformation. -// It allows GroupKinds in the allowlist unless they are in the denylist with ".*" in PodSpec paths, which blocks all PodSpecs. -func (rs *resourceSelectorImagePullPolicy) allowByGroupKind(gk apimachinery.GroupKind) bool { - if _, allowed := TransformAllowlist[gk]; allowed { - if rf, disallowed := TransformDenylist[gk]; disallowed { - for _, s := range rf.PodSpec { - if s == ".*" { - return false - } - } - } - return true - } - return false -} - -// allowByNavpath checks if a GroupKind's PodSpec path (navpath) allows transformation. -// It blocks transformation if the path matches a denylist entry or ".*". -// If not denied, it permits transformation if the path matches an allowlist entry or ".*". -func (rs *resourceSelectorImagePullPolicy) allowByNavpath(gk apimachinery.GroupKind, navpath string, k string) (string, bool) { - if rf, ok := TransformDenylist[gk]; ok { - for _, denypath := range rf.PodSpec { - if denypath == ".*" || navpath == denypath { - return "", false - } - } - } - if rf, ok := TransformAllowlist[gk]; ok { - for _, allowpath := range rf.PodSpec { - if allowpath == ".*" || navpath == allowpath { - return "", true - } - } - } - return "", false -} - -func (l *ManifestList) ReplaceImagePullPolicy(rs ResourceSelector) (ManifestList, error) { - r := &imagePullPolicyReplacer{} - return l.Visit(r, rs) -} - -// imagePullPolicyReplacer implements FieldVisitor and modifies the "imagePullPolicy" field in Kubernetes manifests. -type imagePullPolicyReplacer struct{} - -// Visit sets the value of the "imagePullPolicy" field in a Kubernetes manifest to "Never". -func (i *imagePullPolicyReplacer) Visit(gk apimachinery.GroupKind, navpath string, o map[string]interface{}, k string, v interface{}, rs ResourceSelector) bool { - const imagePullPolicyField = "imagePullPolicy" - if _, allowed := rs.allowByNavpath(gk, navpath, k); !allowed { - return true - } - if k != imagePullPolicyField { - return true - } - if _, ok := v.(string); !ok { - return true - } - o[imagePullPolicyField] = "Never" - return false -} diff --git a/pkg/skaffold/kubernetes/manifest/image_pull_policy_test.go b/pkg/skaffold/kubernetes/manifest/image_pull_policy_test.go deleted file mode 100644 index d1ff2a6bba4..00000000000 --- a/pkg/skaffold/kubernetes/manifest/image_pull_policy_test.go +++ /dev/null @@ -1,67 +0,0 @@ -/* -Copyright 2019 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 manifest - -import ( - "testing" - - "github.com/GoogleContainerTools/skaffold/v2/testutil" -) - -func TestReplaceImagePullPolicy(t *testing.T) { - manifests := ManifestList{[]byte(` -apiVersion: v1 -kind: Pod -metadata: - name: replace-imagePullPolicy -spec: - containers: - - image: gcr.io/k8s-skaffold/example@sha256:81daf011d63b68cfa514ddab7741a1adddd59d3264118dfb0fd9266328bb8883 - name: if-not-present - imagePullPolicy: IfNotPresent - - image: gcr.io/k8s-skaffold/example@sha256:81daf011d63b68cfa514ddab7741a1adddd59d3264118dfb0fd9266328bb8883 - name: always - imagePullPolicy: Always - - image: gcr.io/k8s-skaffold/example@sha256:81daf011d63b68cfa514ddab7741a1adddd59d3264118dfb0fd9266328bb8883 - name: never - imagePullPolicy: Never -`)} - - expected := ManifestList{[]byte(` -apiVersion: v1 -kind: Pod -metadata: - name: replace-imagePullPolicy -spec: - containers: - - image: gcr.io/k8s-skaffold/example@sha256:81daf011d63b68cfa514ddab7741a1adddd59d3264118dfb0fd9266328bb8883 - name: if-not-present - imagePullPolicy: Never - - image: gcr.io/k8s-skaffold/example@sha256:81daf011d63b68cfa514ddab7741a1adddd59d3264118dfb0fd9266328bb8883 - name: always - imagePullPolicy: Never - - image: gcr.io/k8s-skaffold/example@sha256:81daf011d63b68cfa514ddab7741a1adddd59d3264118dfb0fd9266328bb8883 - name: never - imagePullPolicy: Never -`)} - - testutil.Run(t, "", func(t *testutil.T) { - resultManifest, err := manifests.ReplaceImagePullPolicy(NewResourceSelectorImagePullPolicy()) - t.CheckNoError(err) - t.CheckDeepEqual(expected.String(), resultManifest.String(), testutil.YamlObj(t.T)) - }) -} From 83563a960989430eaa42ea2357766395701fb138 Mon Sep 17 00:00:00 2001 From: Suleiman Dibirov <3595194+idsulik@users.noreply.github.com> Date: Tue, 4 Feb 2025 18:35:53 +0200 Subject: [PATCH 3/3] fix(helm): Fix helm package installation order (#9693) * fix(helm): Fix helm package installation order * fix copyright Signed-off-by: Suleiman Dibirov * tests Signed-off-by: Suleiman Dibirov * lint Signed-off-by: Suleiman Dibirov * fixes Signed-off-by: Suleiman Dibirov * fix linters Signed-off-by: Suleiman Dibirov --------- Signed-off-by: Suleiman Dibirov --- pkg/skaffold/deploy/helm/dependencygraph.go | 220 +++++++++++ .../deploy/helm/dependencygraph_test.go | 320 +++++++++++++++ pkg/skaffold/deploy/helm/helm.go | 51 ++- pkg/skaffold/deploy/helm/helm_test.go | 86 ++++ pkg/skaffold/deploy/helm/util.go | 126 ------ pkg/skaffold/deploy/helm/util_test.go | 369 ------------------ pkg/skaffold/schema/defaults/defaults.go | 6 +- 7 files changed, 650 insertions(+), 528 deletions(-) create mode 100644 pkg/skaffold/deploy/helm/dependencygraph.go create mode 100644 pkg/skaffold/deploy/helm/dependencygraph_test.go delete mode 100644 pkg/skaffold/deploy/helm/util.go delete mode 100644 pkg/skaffold/deploy/helm/util_test.go diff --git a/pkg/skaffold/deploy/helm/dependencygraph.go b/pkg/skaffold/deploy/helm/dependencygraph.go new file mode 100644 index 00000000000..ed7c8f21607 --- /dev/null +++ b/pkg/skaffold/deploy/helm/dependencygraph.go @@ -0,0 +1,220 @@ +/* +Copyright 2025 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 helm + +import ( + "fmt" + + "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/schema/latest" +) + +// DependencyGraph represents a graph of helm release dependencies +type DependencyGraph struct { + graph map[string][]string + releases []latest.HelmRelease + hasDependencies bool +} + +// NewDependencyGraph creates a new DependencyGraph from a list of helm releases +func NewDependencyGraph(releases []latest.HelmRelease) (*DependencyGraph, error) { + graph := make(map[string][]string) + releaseNames := make(map[string]bool) + + for _, r := range releases { + if _, exists := releaseNames[r.Name]; exists { + return nil, fmt.Errorf("duplicate release name %s", r.Name) + } + releaseNames[r.Name] = true + } + + // Check for non-existent dependencies + hasDependencies := false + for _, r := range releases { + for _, dep := range r.DependsOn { + if !releaseNames[dep] { + return nil, fmt.Errorf("release %s depends on non-existent release %s", r.Name, dep) + } + hasDependencies = true + } + graph[r.Name] = r.DependsOn + } + + g := &DependencyGraph{ + graph: graph, + releases: releases, + hasDependencies: hasDependencies, + } + + if err := g.hasCycles(); err != nil { + return nil, err + } + + return g, nil +} + +// GetReleasesByLevel returns releases grouped by their dependency level while preserving +// the original order within each level. Level 0 contains releases with no dependencies, +// level 1 contains releases that depend only on level 0 releases, and so on. +func (g *DependencyGraph) GetReleasesByLevel() (map[int][]string, error) { + if len(g.releases) == 0 { + // For empty releases, return empty map to avoid nil + return map[int][]string{}, nil + } + + if !g.hasDependencies { + // Fast path: if no dependencies, all releases are at level 0 + // Preserve original order from releases slice + return map[int][]string{ + 0: g.getNames(), + }, nil + } + + order, err := g.calculateDeploymentOrder() + if err != nil { + return nil, err + } + + return g.groupReleasesByLevel(order), nil +} + +// hasCycles checks if there are any cycles in the dependency graph +func (g *DependencyGraph) hasCycles() error { + if !g.hasDependencies { + return nil + } + + visited := make(map[string]bool) + recStack := make(map[string]bool) + + var checkCycle func(node string) error + checkCycle = func(node string) error { + if !visited[node] { + visited[node] = true + recStack[node] = true + + for _, dep := range g.graph[node] { + if !visited[dep] { + if err := checkCycle(dep); err != nil { + return err + } + } else if recStack[dep] { + return fmt.Errorf("cycle detected involving release %q", node) + } + } + } + recStack[node] = false + return nil + } + + for node := range g.graph { + if !visited[node] { + if err := checkCycle(node); err != nil { + return err + } + } + } + return nil +} + +// getNames returns a slice of release names in their original order +func (g *DependencyGraph) getNames() []string { + names := make([]string, len(g.releases)) + for i, release := range g.releases { + names[i] = release.Name + } + return names +} + +// calculateDeploymentOrder returns a topologically sorted list of releases, +// ensuring that releases are deployed after their dependencies while maintaining +// the original order where possible +func (g *DependencyGraph) calculateDeploymentOrder() ([]string, error) { + visited := make(map[string]bool) + order := make([]string, 0, len(g.releases)) + + // Create a mapping of release name to its index in original order + originalOrder := make(map[string]int, len(g.releases)) + for i, release := range g.releases { + originalOrder[release.Name] = i + } + + var visit func(node string) error + visit = func(node string) error { + if visited[node] { + return nil + } + visited[node] = true + + // Sort dependencies based on original order + deps := make([]string, len(g.graph[node])) + copy(deps, g.graph[node]) + if len(deps) > 1 { + // Sort dependencies by their original position + for i := 0; i < len(deps)-1; i++ { + for j := i + 1; j < len(deps); j++ { + if originalOrder[deps[i]] > originalOrder[deps[j]] { + deps[i], deps[j] = deps[j], deps[i] + } + } + } + } + + // Visit dependencies in original order + for _, dep := range deps { + if err := visit(dep); err != nil { + return err + } + } + order = append(order, node) + return nil + } + + // Process releases in their original order + for _, release := range g.releases { + if err := visit(release.Name); err != nil { + return nil, err + } + } + + return order, nil +} + +// groupReleasesByLevel groups releases by their dependency level while preserving +// the original order within each level +func (g *DependencyGraph) groupReleasesByLevel(order []string) map[int][]string { + levels := make(map[int][]string) + releaseLevels := make(map[string]int) + + // Calculate level for each release + for _, release := range order { + level := 0 + for _, dep := range g.graph[release] { + if depLevel, exists := releaseLevels[dep]; exists { + if depLevel >= level { + level = depLevel + 1 + } + } + } + releaseLevels[release] = level + if levels[level] == nil { + levels[level] = make([]string, 0) + } + levels[level] = append(levels[level], release) + } + + return levels +} diff --git a/pkg/skaffold/deploy/helm/dependencygraph_test.go b/pkg/skaffold/deploy/helm/dependencygraph_test.go new file mode 100644 index 00000000000..ec48b267a19 --- /dev/null +++ b/pkg/skaffold/deploy/helm/dependencygraph_test.go @@ -0,0 +1,320 @@ +/* +Copyright 2025 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 helm + +import ( + "slices" + "testing" + + "github.com/google/go-cmp/cmp" + + "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/schema/latest" + "github.com/GoogleContainerTools/skaffold/v2/testutil" +) + +func TestNewDependencyGraph(t *testing.T) { + tests := []struct { + description string + releases []latest.HelmRelease + expected map[string][]string + shouldErr bool + errorMessage string + }{ + { + description: "simple dependency graph", + releases: []latest.HelmRelease{ + {Name: "release1", DependsOn: []string{"release2"}}, + {Name: "release2"}, + }, + expected: map[string][]string{ + "release1": {"release2"}, + "release2": {}, + }, + }, + { + description: "no dependencies", + releases: []latest.HelmRelease{ + {Name: "release1"}, + {Name: "release2"}, + }, + expected: map[string][]string{ + "release1": nil, + "release2": nil, + }, + }, + { + description: "multiple dependencies", + releases: []latest.HelmRelease{ + {Name: "release1", DependsOn: []string{"release2", "release3"}}, + {Name: "release2", DependsOn: []string{"release3"}}, + {Name: "release3"}, + }, + expected: map[string][]string{ + "release1": {"release2", "release3"}, + "release2": {"release3"}, + "release3": nil, + }, + }, + { + description: "non-existent dependency", + releases: []latest.HelmRelease{ + {Name: "release1", DependsOn: []string{"release3"}}, // release3 doesn't exist + {Name: "release2"}, + }, + shouldErr: true, + errorMessage: "release release1 depends on non-existent release release3", + }, + { + description: "duplicate release names", + releases: []latest.HelmRelease{ + {Name: "release1"}, + {Name: "release1"}, + }, + shouldErr: true, + errorMessage: "duplicate release name release1", + }, + { + description: "has cycle", + releases: []latest.HelmRelease{ + {Name: "a", DependsOn: []string{"b"}}, + {Name: "b", DependsOn: []string{"a"}}, + }, + shouldErr: true, + errorMessage: "cycle detected", + }, + } + + for _, test := range tests { + testutil.Run(t, test.description, func(t *testutil.T) { + graph, err := NewDependencyGraph(test.releases) + + if test.shouldErr { + t.CheckErrorContains(test.errorMessage, err) + return + } + t.CheckDeepEqual(len(test.expected), len(graph.graph)) + + //nolint:gocritic + opt := cmp.Comparer(func(x, y []string) bool { + return slices.Equal(x, y) + }) + if diff := cmp.Diff(test.expected, graph.graph, opt); diff != "" { + t.Errorf("%s:got unexpected diff: %s", test.description, diff) + } + }) + } +} + +func TestHasCycles(t *testing.T) { + tests := []struct { + description string + releases []latest.HelmRelease + shouldErr bool + }{ + { + description: "no cycles", + releases: []latest.HelmRelease{ + {Name: "a", DependsOn: []string{"b", "c"}}, + {Name: "b", DependsOn: []string{"c"}}, + {Name: "c"}, + }, + shouldErr: false, + }, + { + description: "simple cycle", + releases: []latest.HelmRelease{ + {Name: "a", DependsOn: []string{"b"}}, + {Name: "b", DependsOn: []string{"a"}}, + }, + shouldErr: true, + }, + { + description: "complex cycle", + releases: []latest.HelmRelease{ + {Name: "a", DependsOn: []string{"b"}}, + {Name: "b", DependsOn: []string{"c"}}, + {Name: "c", DependsOn: []string{"d"}}, + {Name: "d", DependsOn: []string{"b"}}, + }, + shouldErr: true, + }, + { + description: "empty graph", + releases: []latest.HelmRelease{}, + shouldErr: false, + }, + { + description: "disconnected components", + releases: []latest.HelmRelease{ + {Name: "a", DependsOn: []string{"b"}}, + {Name: "b"}, + {Name: "c", DependsOn: []string{"d"}}, + {Name: "d"}, + }, + shouldErr: false, + }, + } + + for _, test := range tests { + testutil.Run(t, test.description, func(t *testutil.T) { + _, err := NewDependencyGraph(test.releases) + + if test.shouldErr { + t.CheckErrorContains("cycle detected", err) + } else { + t.RequireNoError(err) + } + }) + } +} + +func TestGetReleasesByLevel(t *testing.T) { + tests := []struct { + description string + releases []latest.HelmRelease + expected map[int][]string + }{ + { + description: "linear chain", + releases: []latest.HelmRelease{ + {Name: "a", DependsOn: []string{"b"}}, + {Name: "b", DependsOn: []string{"c"}}, + {Name: "c"}, + }, + expected: map[int][]string{ + 0: {"c"}, + 1: {"b"}, + 2: {"a"}, + }, + }, + { + description: "multiple dependencies at same level", + releases: []latest.HelmRelease{ + {Name: "a", DependsOn: []string{"c", "b"}}, + {Name: "b", DependsOn: []string{"d"}}, + {Name: "c", DependsOn: []string{"d"}}, + {Name: "d"}, + }, + expected: map[int][]string{ + 0: {"d"}, + 1: {"b", "c"}, + 2: {"a"}, + }, + }, + { + description: "no dependencies", + releases: []latest.HelmRelease{ + {Name: "a"}, + {Name: "b"}, + {Name: "c"}, + }, + expected: map[int][]string{ + 0: {"a", "b", "c"}, + }, + }, + { + description: "empty graph", + releases: []latest.HelmRelease{}, + expected: map[int][]string{}, + }, + { + description: "mixed levels", + releases: []latest.HelmRelease{ + {Name: "a", DependsOn: []string{"b", "c"}}, + {Name: "b", DependsOn: []string{"d"}}, + {Name: "c", DependsOn: []string{"d", "e"}}, + {Name: "d"}, + {Name: "e"}, + }, + expected: map[int][]string{ + 0: {"d", "e"}, + 1: {"b", "c"}, + 2: {"a"}, + }, + }, + { + description: "preserve order within levels", + releases: []latest.HelmRelease{ + {Name: "a1", DependsOn: []string{"b1", "b2"}}, + {Name: "a2", DependsOn: []string{"b1", "b2"}}, + {Name: "b1", DependsOn: []string{"c1", "c2"}}, + {Name: "b2", DependsOn: []string{"c1", "c2"}}, + {Name: "c1"}, + {Name: "c2"}, + }, + expected: map[int][]string{ + 0: {"c1", "c2"}, + 1: {"b1", "b2"}, + 2: {"a1", "a2"}, + }, + }, + } + + for _, test := range tests { + testutil.Run(t, test.description, func(t *testutil.T) { + graph, _ := NewDependencyGraph(test.releases) + levels, err := graph.GetReleasesByLevel() + t.RequireNoError(err) + + t.CheckDeepEqual(len(test.expected), len(levels)) + + // Check that each level contains expected releases + //nolint:gocritic + opt := cmp.Comparer(func(x, y []string) bool { + return slices.Equal(x, y) + }) + if diff := cmp.Diff(test.expected, levels, opt); diff != "" { + t.Errorf("%s: got unexpected diff (-want +got):\n%s", test.description, diff) + } + + // Verify level assignments are correct + for level, releases := range levels { + for _, release := range releases { + // Check that all dependencies are at lower levels + for _, dep := range graph.graph[release] { + for depLevel, depReleases := range levels { + if slices.Contains(depReleases, dep) { + if depLevel >= level { + t.Errorf("dependency %s at level %d >= release %s at level %d", dep, depLevel, release, level) + } + } + } + } + } + } + + // Verify order preservation within each level + // Create a map of release name to its original position + originalOrder := make(map[string]int) + for i, release := range test.releases { + originalOrder[release.Name] = i + } + + // Check that relative order is preserved within each level + for _, releasesAtLevel := range levels { + for i := 1; i < len(releasesAtLevel); i++ { + prevRelease := releasesAtLevel[i-1] + currRelease := releasesAtLevel[i] + if originalOrder[prevRelease] > originalOrder[currRelease] { + t.Errorf("order not preserved within level: %s (original position %d) should come after %s (original position %d)", + prevRelease, originalOrder[prevRelease], currRelease, originalOrder[currRelease]) + } + } + } + }) + } +} diff --git a/pkg/skaffold/deploy/helm/helm.go b/pkg/skaffold/deploy/helm/helm.go index 344477564e5..6869c2a8a1b 100644 --- a/pkg/skaffold/deploy/helm/helm.go +++ b/pkg/skaffold/deploy/helm/helm.go @@ -261,40 +261,32 @@ func (h *Deployer) Deploy(ctx context.Context, out io.Writer, builds []graph.Art olog.Entry(ctx).Infof("Deploying with helm v%s ...", h.bV) - // Build dependency graph to determine the order of Helm release deployments. - dependencyGraph, err := BuildDependencyGraph(h.Releases) + dependencyGraph, err := NewDependencyGraph(h.Releases) if err != nil { - return fmt.Errorf("error building dependency graph: %w", err) + return fmt.Errorf("unable to create dependency graph: %w", err) } - // Verify no cycles in the dependency graph - if err := VerifyNoCycles(dependencyGraph); err != nil { - return fmt.Errorf("error verifying dependency graph: %w", err) - } - - // Calculate deployment order - deploymentOrder, err := calculateDeploymentOrder(dependencyGraph) + levelByLevelReleases, err := dependencyGraph.GetReleasesByLevel() if err != nil { - return fmt.Errorf("error calculating deployment order: %w", err) + return fmt.Errorf("unable to get releases by level: %w", err) } var mu sync2.Mutex nsMap := map[string]struct{}{} manifests := manifest.ManifestList{} - // Group releases by their dependency level to deploy them in the correct order. - levelGroups := groupReleasesByLevel(deploymentOrder, dependencyGraph) - - var concurrency int - if h.Concurrency == nil || *h.Concurrency == 1 { - concurrency = 1 - olog.Entry(ctx).Infof("Installing %d releases sequentially", len(h.Releases)) - } else { + concurrency := 1 + if h.Concurrency != nil { if *h.Concurrency == 0 { - concurrency = -1 + concurrency = -1 // unlimited } else { concurrency = *h.Concurrency } + } + + if concurrency == 1 { + olog.Entry(ctx).Infof("Installing %d releases sequentially", len(h.Releases)) + } else { olog.Entry(ctx).Infof("Installing %d releases concurrently", len(h.Releases)) } @@ -303,21 +295,24 @@ func (h *Deployer) Deploy(ctx context.Context, out io.Writer, builds []graph.Art releaseNameToRelease[r.Name] = r } + levels := make([]int, 0, len(levelByLevelReleases)) + for level := range levelByLevelReleases { + levels = append(levels, level) + } + // Sort levels in ascending order + sort.Ints(levels) + // 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+1, len(levelGroups), len(releases)) + for _, level := range levels { + releases := levelByLevelReleases[level] + if len(levelByLevelReleases) > 1 { + olog.Entry(ctx).Infof("Installing level %d/%d releases (%d releases)", level+1, len(levelByLevelReleases), 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 _, name := range releases { release := releaseNameToRelease[name] diff --git a/pkg/skaffold/deploy/helm/helm_test.go b/pkg/skaffold/deploy/helm/helm_test.go index 248d0015261..a2affa0f9a0 100644 --- a/pkg/skaffold/deploy/helm/helm_test.go +++ b/pkg/skaffold/deploy/helm/helm_test.go @@ -81,6 +81,48 @@ var testDeployWithDependsOnConfig = latest.LegacyHelmDeploy{ }}, } +var testDeployPreservingOrderWithDependsOnConfig = latest.LegacyHelmDeploy{ + /** + * Visual representation of the graph: + * A: B, C + * B: C + * C + * D: C + * E: D + * F + */ + + /** + * Expected order of deployment: + * level 0: C, F + * level 1: B, D + * level 2: A, E + */ + Releases: []latest.HelmRelease{{ + Name: "A", + ChartPath: "examples/test", + DependsOn: []string{"B", "C"}, + }, { + Name: "B", + ChartPath: "examples/test", + DependsOn: []string{"C"}, + }, { + Name: "C", + ChartPath: "examples/test", + }, { + Name: "D", + ChartPath: "examples/test", + DependsOn: []string{"C"}, + }, { + Name: "E", + ChartPath: "examples/test", + DependsOn: []string{"D"}, + }, { + Name: "F", + ChartPath: "examples/test", + }, + }} + var testDeployNamespacedConfig = latest.LegacyHelmDeploy{ Releases: []latest.HelmRelease{{ Name: "skaffold-helm", @@ -479,6 +521,50 @@ func TestHelmDeploy(t *testing.T) { builds: testBuilds, expectedNamespaces: []string{""}, }, + { + /* + expected order of deployment: + level 0: C, F + level 1: B, D + level 2: A, E + */ + description: "helm3.1 deploy in order on each level with dependsOn success", + commands: testutil. + CmdRunWithOutput("helm version --client", version31). + AndRun("helm --kube-context kubecontext get all C --kubeconfig kubeconfig"). + AndRun("helm --kube-context kubecontext dep build examples/test --kubeconfig kubeconfig"). + AndRunEnv("helm --kube-context kubecontext upgrade C 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 C --template {{.Release.Manifest}} --kubeconfig kubeconfig", validDeployYaml). + AndRun("helm --kube-context kubecontext get all F --kubeconfig kubeconfig"). + AndRun("helm --kube-context kubecontext dep build examples/test --kubeconfig kubeconfig"). + AndRunEnv("helm --kube-context kubecontext upgrade F 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 F --template {{.Release.Manifest}} --kubeconfig kubeconfig", validDeployYaml). + AndRun("helm --kube-context kubecontext get all B --kubeconfig kubeconfig"). + AndRun("helm --kube-context kubecontext dep build examples/test --kubeconfig kubeconfig"). + AndRunEnv("helm --kube-context kubecontext upgrade B 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 B --template {{.Release.Manifest}} --kubeconfig kubeconfig", validDeployYaml). + AndRun("helm --kube-context kubecontext get all D --kubeconfig kubeconfig"). + AndRun("helm --kube-context kubecontext dep build examples/test --kubeconfig kubeconfig"). + AndRunEnv("helm --kube-context kubecontext upgrade D 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 D --template {{.Release.Manifest}} --kubeconfig kubeconfig", validDeployYaml). + AndRun("helm --kube-context kubecontext get all A --kubeconfig kubeconfig"). + AndRun("helm --kube-context kubecontext dep build examples/test --kubeconfig kubeconfig"). + AndRunEnv("helm --kube-context kubecontext upgrade A 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 A --template {{.Release.Manifest}} --kubeconfig kubeconfig", validDeployYaml). + AndRun("helm --kube-context kubecontext get all E --kubeconfig kubeconfig"). + AndRun("helm --kube-context kubecontext dep build examples/test --kubeconfig kubeconfig"). + AndRunEnv("helm --kube-context kubecontext upgrade E 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 E --template {{.Release.Manifest}} --kubeconfig kubeconfig", validDeployYaml), + helm: testDeployPreservingOrderWithDependsOnConfig, + 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 deleted file mode 100644 index e225758179a..00000000000 --- a/pkg/skaffold/deploy/helm/util.go +++ /dev/null @@ -1,126 +0,0 @@ -/* -Copyright 2019 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 helm - -import ( - "fmt" - - "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/schema/latest" -) - -func BuildDependencyGraph(releases []latest.HelmRelease) (map[string][]string, error) { - dependencyGraph := make(map[string][]string) - for _, r := range releases { - dependencyGraph[r.Name] = r.DependsOn - } - - return dependencyGraph, nil -} - -// VerifyNoCycles checks if there are any cycles in the dependency graph -func VerifyNoCycles(graph map[string][]string) error { - visited := make(map[string]bool) - recStack := make(map[string]bool) - - var checkCycle func(node string) error - checkCycle = func(node string) error { - if !visited[node] { - visited[node] = true - recStack[node] = true - - for _, dep := range graph[node] { - if !visited[dep] { - if err := checkCycle(dep); err != nil { - return err - } - } else if recStack[dep] { - return fmt.Errorf("cycle detected involving release %s", node) - } - } - } - recStack[node] = false - return nil - } - - for node := range graph { - if !visited[node] { - if err := checkCycle(node); err != nil { - return err - } - } - } - return nil -} - -// calculateDeploymentOrder returns a topologically sorted list of releases, -// ensuring that releases are deployed after their dependencies. -func calculateDeploymentOrder(graph map[string][]string) ([]string, error) { - visited := make(map[string]bool) - order := make([]string, 0) - - var visit func(node string) error - visit = func(node string) error { - if visited[node] { - return nil - } - visited[node] = true - - for _, dep := range graph[node] { - if err := visit(dep); err != nil { - return err - } - } - order = append(order, node) - return nil - } - - for node := range graph { - if err := visit(node); err != nil { - return nil, err - } - } - - return order, nil -} - -// groupReleasesByLevel groups releases by their dependency level -// Level 0 contains releases with no dependencies -// Level 1 contains releases that depend only on level 0 releases -// And so on... -func groupReleasesByLevel(order []string, graph map[string][]string) map[int][]string { - levels := make(map[int][]string) - releaseLevels := make(map[string]int) - - // Calculate level for each release - for _, release := range order { - level := 0 - for _, dep := range graph[release] { - if depLevel, exists := releaseLevels[dep]; exists { - if depLevel >= level { - level = depLevel + 1 - } - } - } - releaseLevels[release] = level - if levels[level] == nil { - levels[level] = make([]string, 0) - } - levels[level] = append(levels[level], release) - } - - return levels -} diff --git a/pkg/skaffold/deploy/helm/util_test.go b/pkg/skaffold/deploy/helm/util_test.go deleted file mode 100644 index a7921f0a810..00000000000 --- a/pkg/skaffold/deploy/helm/util_test.go +++ /dev/null @@ -1,369 +0,0 @@ -/* -Copyright 2019 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 helm - -import ( - "slices" - "testing" - - "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/schema/latest" - "github.com/GoogleContainerTools/skaffold/v2/testutil" -) - -func TestBuildDependencyGraph(t *testing.T) { - tests := []struct { - description string - releases []latest.HelmRelease - expected map[string][]string - shouldErr bool - }{ - { - description: "simple dependency graph", - releases: []latest.HelmRelease{ - {Name: "release1", DependsOn: []string{"release2"}}, - {Name: "release2", DependsOn: []string{}}, - }, - expected: map[string][]string{ - "release1": {"release2"}, - "release2": {}, - }, - }, - { - description: "no dependencies", - releases: []latest.HelmRelease{ - {Name: "release1"}, - {Name: "release2"}, - }, - expected: map[string][]string{ - "release1": nil, - "release2": nil, - }, - }, - { - description: "multiple dependencies", - releases: []latest.HelmRelease{ - {Name: "release1", DependsOn: []string{"release2", "release3"}}, - {Name: "release2", DependsOn: []string{"release3"}}, - {Name: "release3"}, - }, - expected: map[string][]string{ - "release1": {"release2", "release3"}, - "release2": {"release3"}, - "release3": nil, - }, - }, - { - description: "simple dependency graph with placeholder in name", - releases: []latest.HelmRelease{ - {Name: "release1-{{.Service}}", DependsOn: []string{"release2-{{.Service}}"}}, - {Name: "release2-{{.Service}}", DependsOn: []string{}}, - }, - expected: map[string][]string{ - "release1-{{.Service}}": {"release2-{{.Service}}"}, - "release2-{{.Service}}": {}, - }, - }, - } - - for _, test := range tests { - testutil.Run(t, test.description, func(t *testutil.T) { - graph, err := BuildDependencyGraph(test.releases) - - if test.shouldErr { - t.CheckError(true, err) - return - } - - t.CheckError(false, err) - t.CheckDeepEqual(len(test.expected), len(graph)) - - for release, deps := range test.expected { - actualDeps, exists := graph[release] - if !exists { - t.Errorf("missing release %s in graph", release) - continue - } - - if len(deps) != len(actualDeps) { - t.Errorf("expected %d dependencies for %s, got %d", len(deps), release, len(actualDeps)) - continue - } - - // Check all expected dependencies exist - for _, dep := range deps { - if !slices.Contains(actualDeps, dep) { - t.Errorf("missing dependency %s for release %s", dep, release) - } - } - } - }) - } -} - -func TestVerifyNoCycles(t *testing.T) { - tests := []struct { - description string - graph map[string][]string - shouldErr bool - }{ - { - description: "no cycles", - graph: map[string][]string{ - "a": {"b", "c"}, - "b": {"c"}, - "c": {}, - }, - shouldErr: false, - }, - { - description: "simple cycle", - graph: map[string][]string{ - "a": {"b"}, - "b": {"a"}, - }, - shouldErr: true, - }, - { - description: "complex cycle", - graph: map[string][]string{ - "a": {"b"}, - "b": {"c"}, - "c": {"d"}, - "d": {"b"}, - }, - shouldErr: true, - }, - { - description: "self dependency", - graph: map[string][]string{ - "a": {"a"}, - }, - shouldErr: true, - }, - { - description: "empty graph", - graph: map[string][]string{}, - shouldErr: false, - }, - { - description: "disconnected components", - graph: map[string][]string{ - "a": {"b"}, - "c": {"d"}, - "b": {}, - "d": {}, - }, - shouldErr: false, - }, - } - - for _, test := range tests { - testutil.Run(t, test.description, func(t *testutil.T) { - err := VerifyNoCycles(test.graph) - - if test.shouldErr { - t.CheckErrorContains("cycle detected", err) - } else { - t.RequireNoError(err) - } - }) - } -} - -func TestCalculateDeploymentOrder(t *testing.T) { - tests := []struct { - description string - graph map[string][]string - expected []string - shouldErr bool - }{ - { - description: "linear dependency chain", - graph: map[string][]string{ - "a": {"b"}, - "b": {"c"}, - "c": {}, - }, - expected: []string{"c", "b", "a"}, - }, - { - description: "multiple dependencies", - graph: map[string][]string{ - "a": {"b", "c"}, - "b": {"d"}, - "c": {"d"}, - "d": {}, - }, - expected: []string{"d", "b", "c", "a"}, - }, - { - description: "no dependencies", - graph: map[string][]string{ - "a": {}, - "b": {}, - "c": {}, - }, - expected: []string{"a", "b", "c"}, - }, - { - description: "diamond dependency", - graph: map[string][]string{ - "a": {"b", "c"}, - "b": {"d"}, - "c": {"d"}, - "d": {}, - }, - expected: []string{"d", "b", "c", "a"}, - }, - { - description: "empty graph", - graph: map[string][]string{}, - expected: []string{}, - }, - } - - for _, test := range tests { - testutil.Run(t, test.description, func(t *testutil.T) { - order, err := calculateDeploymentOrder(test.graph) - - if test.shouldErr { - t.CheckError(true, err) - return - } - - t.CheckError(false, err) - - // Verify order satisfies dependencies - installed := make(map[string]bool) - for _, release := range order { - // Check all dependencies are installed - for _, dep := range test.graph[release] { - if !installed[dep] { - t.Errorf("dependency %s not installed before %s", dep, release) - } - } - installed[release] = true - } - - // Verify all nodes are present - if len(order) != len(test.graph) { - t.Errorf("expected %d nodes, got %d", len(test.graph), len(order)) - } - }) - } -} - -func TestGroupReleasesByLevel(t *testing.T) { - tests := []struct { - description string - order []string - graph map[string][]string - expected map[int][]string - }{ - { - description: "linear chain", - order: []string{"c", "b", "a"}, - graph: map[string][]string{ - "a": {"b"}, - "b": {"c"}, - "c": {}, - }, - expected: map[int][]string{ - 0: {"c"}, - 1: {"b"}, - 2: {"a"}, - }, - }, - { - description: "multiple dependencies at same level", - order: []string{"d", "b", "c", "a"}, - graph: map[string][]string{ - "a": {"b", "c"}, - "b": {"d"}, - "c": {"d"}, - "d": {}, - }, - expected: map[int][]string{ - 0: {"d"}, - 1: {"b", "c"}, - 2: {"a"}, - }, - }, - { - description: "no dependencies", - order: []string{"a", "b", "c"}, - graph: map[string][]string{ - "a": {}, - "b": {}, - "c": {}, - }, - expected: map[int][]string{ - 0: {"a", "b", "c"}, - }, - }, - { - description: "empty graph", - order: []string{}, - graph: map[string][]string{}, - expected: map[int][]string{}, - }, - { - description: "mixed levels", - order: []string{"d", "e", "b", "c", "a"}, - graph: map[string][]string{ - "a": {"b", "c"}, - "b": {"d"}, - "c": {"d", "e"}, - "d": {}, - "e": {}, - }, - expected: map[int][]string{ - 0: {"d", "e"}, - 1: {"b", "c"}, - 2: {"a"}, - }, - }, - } - - for _, test := range tests { - testutil.Run(t, test.description, func(t *testutil.T) { - levels := groupReleasesByLevel(test.order, test.graph) - - t.CheckDeepEqual(len(test.expected), len(levels)) - - for level, releases := range test.expected { - t.CheckDeepEqual(releases, levels[level]) - } - - // Verify level assignments are correct - for level, releases := range levels { - for _, release := range releases { - // Check that all dependencies are at lower levels - for _, dep := range test.graph[release] { - for depLevel, depReleases := range levels { - if slices.Contains(depReleases, dep) { - if depLevel >= level { - t.Errorf("dependency %s at level %d >= release %s at level %d", dep, depLevel, release, level) - } - } - } - } - } - } - }) - } -} diff --git a/pkg/skaffold/schema/defaults/defaults.go b/pkg/skaffold/schema/defaults/defaults.go index 596c3fdcaf2..98f0c98191e 100644 --- a/pkg/skaffold/schema/defaults/defaults.go +++ b/pkg/skaffold/schema/defaults/defaults.go @@ -129,14 +129,10 @@ func setHelmDefaults(c *latest.SkaffoldConfig) error { } if len(c.Deploy.LegacyHelmDeploy.Releases) > 1 { - graph, err := helm.BuildDependencyGraph(c.Deploy.LegacyHelmDeploy.Releases) + _, err := helm.NewDependencyGraph(c.Deploy.LegacyHelmDeploy.Releases) if err != nil { return err } - - if err := helm.VerifyNoCycles(graph); err != nil { - return err - } } return nil