From 10e32a205525b39fe3bdf9d27387ba872858c78c Mon Sep 17 00:00:00 2001 From: Suleiman Dibirov Date: Tue, 4 Feb 2025 08:42:08 +0200 Subject: [PATCH] fixes Signed-off-by: Suleiman Dibirov --- pkg/skaffold/deploy/helm/dependencygraph.go | 96 ++++++++++++------- .../deploy/helm/dependencygraph_test.go | 95 ++++-------------- pkg/skaffold/schema/defaults/defaults.go | 6 +- 3 files changed, 77 insertions(+), 120 deletions(-) diff --git a/pkg/skaffold/deploy/helm/dependencygraph.go b/pkg/skaffold/deploy/helm/dependencygraph.go index b819f18bb49..ed7c8f21607 100644 --- a/pkg/skaffold/deploy/helm/dependencygraph.go +++ b/pkg/skaffold/deploy/helm/dependencygraph.go @@ -53,15 +53,46 @@ func NewDependencyGraph(releases []latest.HelmRelease) (*DependencyGraph, error) graph[r.Name] = r.DependsOn } - return &DependencyGraph{ + g := &DependencyGraph{ graph: graph, releases: releases, hasDependencies: hasDependencies, - }, nil + } + + if err := g.hasCycles(); err != nil { + return nil, err + } + + return g, nil } -// HasCycles checks if there are any cycles in the dependency graph -func (g *DependencyGraph) HasCycles() error { +// 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 } @@ -81,7 +112,7 @@ func (g *DependencyGraph) HasCycles() error { return err } } else if recStack[dep] { - return fmt.Errorf("cycle detected involving release %s", node) + return fmt.Errorf("cycle detected involving release %q", node) } } } @@ -99,36 +130,6 @@ func (g *DependencyGraph) HasCycles() error { return 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 - } - - // Check for cycles before calculating deployment order - if err := g.HasCycles(); err != nil { - return nil, err - } - - order, err := g.calculateDeploymentOrder() - if err != nil { - return nil, err - } - - return g.groupReleasesByLevel(order), nil -} - // getNames returns a slice of release names in their original order func (g *DependencyGraph) getNames() []string { names := make([]string, len(g.releases)) @@ -143,7 +144,13 @@ func (g *DependencyGraph) getNames() []string { // the original order where possible func (g *DependencyGraph) calculateDeploymentOrder() ([]string, error) { visited := make(map[string]bool) - order := make([]string, 0) + 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 { @@ -152,7 +159,22 @@ func (g *DependencyGraph) calculateDeploymentOrder() ([]string, error) { } visited[node] = true - for _, dep := range g.graph[node] { + // 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 } diff --git a/pkg/skaffold/deploy/helm/dependencygraph_test.go b/pkg/skaffold/deploy/helm/dependencygraph_test.go index a4e128b0879..4086132cc32 100644 --- a/pkg/skaffold/deploy/helm/dependencygraph_test.go +++ b/pkg/skaffold/deploy/helm/dependencygraph_test.go @@ -22,6 +22,7 @@ import ( "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/schema/latest" "github.com/GoogleContainerTools/skaffold/v2/testutil" + "github.com/google/go-cmp/cmp" ) func TestNewDependencyGraph(t *testing.T) { @@ -85,6 +86,15 @@ func TestNewDependencyGraph(t *testing.T) { 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 { @@ -97,24 +107,11 @@ func TestNewDependencyGraph(t *testing.T) { } t.CheckDeepEqual(len(test.expected), len(graph.graph)) - for release, deps := range test.expected { - actualDeps, exists := graph.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) - } - } + 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) } }) } @@ -172,9 +169,7 @@ func TestHasCycles(t *testing.T) { for _, test := range tests { testutil.Run(t, test.description, func(t *testutil.T) { - graph, err := NewDependencyGraph(test.releases) - t.CheckError(false, err) - err = graph.HasCycles() + _, err := NewDependencyGraph(test.releases) if test.shouldErr { t.CheckErrorContains("cycle detected", err) @@ -207,7 +202,7 @@ func TestGetReleasesByLevel(t *testing.T) { { description: "multiple dependencies at same level", releases: []latest.HelmRelease{ - {Name: "a", DependsOn: []string{"b", "c"}}, + {Name: "a", DependsOn: []string{"c", "b"}}, {Name: "b", DependsOn: []string{"d"}}, {Name: "c", DependsOn: []string{"d"}}, {Name: "d"}, @@ -317,59 +312,3 @@ func TestGetReleasesByLevel(t *testing.T) { }) } } - -func TestOrderPreservationWithinLevels(t *testing.T) { - tests := []struct { - description string - releases []latest.HelmRelease - expected map[int][]string - }{ - { - description: "preserve order within same level", - releases: []latest.HelmRelease{ - {Name: "a3"}, - {Name: "a2"}, - {Name: "a1"}, - {Name: "b3", DependsOn: []string{"a3", "a2", "a1"}}, - {Name: "b2", DependsOn: []string{"a1", "a2", "a3"}}, - {Name: "b1", DependsOn: []string{"a1", "a2", "a3"}}, - }, - expected: map[int][]string{ - 0: {"a3", "a2", "a1"}, - 1: {"b3", "b2", "b1"}, - }, - }, - { - description: "preserve order with mixed dependencies", - releases: []latest.HelmRelease{ - {Name: "c2", DependsOn: []string{"a1"}}, - {Name: "a1"}, - {Name: "c1", DependsOn: []string{"a1"}}, - {Name: "c3", DependsOn: []string{"a1"}}, - {Name: "b1", DependsOn: []string{"c1", "c2", "c3"}}, - }, - expected: map[int][]string{ - 0: {"a1"}, - 1: {"c2", "c1", "c3"}, - 2: {"b1"}, - }, - }, - } - - for _, test := range tests { - testutil.Run(t, test.description, func(t *testutil.T) { - graph, err := NewDependencyGraph(test.releases) - t.CheckNoError(err) - - levels, err := graph.GetReleasesByLevel() - t.CheckNoError(err) - - // Verify exact ordering within each level - for level, expectedReleases := range test.expected { - actualReleases, exists := levels[level] - t.CheckTrue(exists) - t.CheckDeepEqual(expectedReleases, actualReleases) - } - }) - } -} diff --git a/pkg/skaffold/schema/defaults/defaults.go b/pkg/skaffold/schema/defaults/defaults.go index 59360101738..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 { - dependencyGraph, err := helm.NewDependencyGraph(c.Deploy.LegacyHelmDeploy.Releases) + _, err := helm.NewDependencyGraph(c.Deploy.LegacyHelmDeploy.Releases) if err != nil { return err } - - if err := dependencyGraph.HasCycles(); err != nil { - return fmt.Errorf("dependency cycle detected in helm releases: %w", err) - } } return nil