fixes

Signed-off-by: Suleiman Dibirov <idsulik@gmail.com>
GoogleContainerTools · Feb 4, 2025 · 10e32a2 · 10e32a2
1 parent 6506f15
commit 10e32a2
Show file tree

Hide file tree

Showing 3 changed files with 77 additions and 120 deletions.
diff --git 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
@@ -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
@@ -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