From ac07fca85e97595007c1c3a8af5a666bcdfd806b Mon Sep 17 00:00:00 2001
From: Suleiman Dibirov <idsulik@gmail.com>
Date: Tue, 28 Jan 2025 11:26:34 +0200
Subject: [PATCH] fixes

Signed-off-by: Suleiman Dibirov <idsulik@gmail.com>
---
 pkg/skaffold/deploy/helm/dependencygraph.go   |  3 ++
 .../deploy/helm/dependencygraph_test.go       | 33 +++++++++++--------
 2 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/pkg/skaffold/deploy/helm/dependencygraph.go b/pkg/skaffold/deploy/helm/dependencygraph.go
index aa0e51fbae9..f32980c31ab 100644
--- a/pkg/skaffold/deploy/helm/dependencygraph.go
+++ b/pkg/skaffold/deploy/helm/dependencygraph.go
@@ -35,6 +35,9 @@ func NewDependencyGraph(releases []latest.HelmRelease) (*DependencyGraph, error)
 	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
 	}
 
diff --git a/pkg/skaffold/deploy/helm/dependencygraph_test.go b/pkg/skaffold/deploy/helm/dependencygraph_test.go
index 769806bc935..aabb8c2b72f 100644
--- a/pkg/skaffold/deploy/helm/dependencygraph_test.go
+++ b/pkg/skaffold/deploy/helm/dependencygraph_test.go
@@ -26,10 +26,11 @@ import (
 
 func TestNewDependencyGraph(t *testing.T) {
 	tests := []struct {
-		description string
-		releases    []latest.HelmRelease
-		expected    map[string][]string
-		shouldErr   bool
+		description  string
+		releases     []latest.HelmRelease
+		expected     map[string][]string
+		shouldErr    bool
+		errorMessage string
 	}{
 		{
 			description: "simple dependency graph",
@@ -73,6 +74,16 @@ func TestNewDependencyGraph(t *testing.T) {
 				{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",
 		},
 	}
 
@@ -81,7 +92,7 @@ func TestNewDependencyGraph(t *testing.T) {
 			graph, err := NewDependencyGraph(test.releases)
 
 			if test.shouldErr {
-				t.CheckError(test.shouldErr, err)
+				t.CheckErrorContains(test.errorMessage, err)
 				return
 			}
 			t.CheckDeepEqual(len(test.expected), len(graph.graph))
@@ -142,13 +153,6 @@ func TestHasCycles(t *testing.T) {
 			},
 			shouldErr: true,
 		},
-		{
-			description: "self dependency",
-			releases: []latest.HelmRelease{
-				{Name: "a", DependsOn: []string{"a"}},
-			},
-			shouldErr: true,
-		},
 		{
 			description: "empty graph",
 			releases:    []latest.HelmRelease{},
@@ -168,8 +172,9 @@ func TestHasCycles(t *testing.T) {
 
 	for _, test := range tests {
 		testutil.Run(t, test.description, func(t *testutil.T) {
-			graph, _ := NewDependencyGraph(test.releases)
-			err := graph.HasCycles()
+			graph, err := NewDependencyGraph(test.releases)
+			t.CheckError(false, err)
+			err = graph.HasCycles()
 
 			if test.shouldErr {
 				t.CheckErrorContains("cycle detected", err)