Merge pull request #7499 from bacongobbler/fix-7416

ref(helm): sort hooks by kind

pkg/action/hooks.go

@@ -38,7 +38,8 @@ func (cfg *Configuration) execHook(rl *release.Release, hook release.HookEvent,
 		}
 	}
 
-	sort.Sort(hookByWeight(executingHooks))
+	// hooke are pre-ordered by kind, so keep order stable
+	sort.Stable(hookByWeight(executingHooks))
 
 	for _, h := range executingHooks {
 		// Set default delete policy to before-hook-creation

pkg/releaseutil/kind_sorter.go

@@ -16,7 +16,11 @@ limitations under the License.
 
 package releaseutil
 
-import "sort"
+import (
+	"sort"
+
+	"helm.sh/helm/v3/pkg/release"
+)
 
 // KindSortOrder is an ordering of Kinds.
 type KindSortOrder []string
@@ -99,46 +103,42 @@ var UninstallOrder KindSortOrder = []string{
 	"Namespace",
 }
 
-// sortByKind does an in-place sort of manifests by Kind.
+// sort manifests by kind.
 //
 // Results are sorted by 'ordering', keeping order of items with equal kind/priority
-func sortByKind(manifests []Manifest, ordering KindSortOrder) []Manifest {
-	ks := newKindSorter(manifests, ordering)
-	sort.Stable(ks)
-	return ks.manifests
-}
+func sortManifestsByKind(manifests []Manifest, ordering KindSortOrder) []Manifest {
+	sort.SliceStable(manifests, func(i, j int) bool {
+		return lessByKind(manifests[i], manifests[j], manifests[i].Head.Kind, manifests[j].Head.Kind, ordering)
+	})
 
-type kindSorter struct {
-	ordering  map[string]int
-	manifests []Manifest
+	return manifests
 }
 
-func newKindSorter(m []Manifest, s KindSortOrder) *kindSorter {
-	o := make(map[string]int, len(s))
-	for v, k := range s {
-		o[k] = v
-	}
+// sort hooks by kind, using an out-of-place sort to preserve the input parameters.
+//
+// Results are sorted by 'ordering', keeping order of items with equal kind/priority
+func sortHooksByKind(hooks []*release.Hook, ordering KindSortOrder) []*release.Hook {
+	h := hooks
+	sort.SliceStable(h, func(i, j int) bool {
+		return lessByKind(h[i], h[j], h[i].Kind, h[j].Kind, ordering)
+	})
 
-	return &kindSorter{
-		manifests: m,
-		ordering:  o,
-	}
+	return h
 }
 
-func (k *kindSorter) Len() int { return len(k.manifests) }
-
-func (k *kindSorter) Swap(i, j int) { k.manifests[i], k.manifests[j] = k.manifests[j], k.manifests[i] }
+func lessByKind(a interface{}, b interface{}, kindA string, kindB string, o KindSortOrder) bool {
+	ordering := make(map[string]int, len(o))
+	for v, k := range o {
+		ordering[k] = v
+	}
 
-func (k *kindSorter) Less(i, j int) bool {
-	a := k.manifests[i]
-	b := k.manifests[j]
-	first, aok := k.ordering[a.Head.Kind]
-	second, bok := k.ordering[b.Head.Kind]
+	first, aok := ordering[kindA]
+	second, bok := ordering[kindB]
 
 	if !aok && !bok {
 		// if both are unknown then sort alphabetically by kind, keep original order if same kind
-		if a.Head.Kind != b.Head.Kind {
-			return a.Head.Kind < b.Head.Kind
+		if kindA != kindB {
+			return kindA < kindB
 		}
 		return first < second
 	}

pkg/releaseutil/kind_sorter_test.go

@@ -19,6 +19,8 @@ package releaseutil
 import (
 	"bytes"
 	"testing"
+
+	"helm.sh/helm/v3/pkg/release"
 )
 
 func TestKindSorter(t *testing.T) {
@@ -175,12 +177,18 @@ func TestKindSorter(t *testing.T) {
 				t.Fatalf("Expected %d names in order, got %d", want, got)
 			}
 			defer buf.Reset()
-			for _, r := range sortByKind(manifests, test.order) {
+			orig := manifests
+			for _, r := range sortManifestsByKind(manifests, test.order) {
 				buf.WriteString(r.Name)
 			}
 			if got := buf.String(); got != test.expected {
 				t.Errorf("Expected %q, got %q", test.expected, got)
 			}
+			for i, manifest := range orig {
+				if manifest != manifests[i] {
+					t.Fatal("Expected input to sortManifestsByKind to stay the same")
+				}
+			}
 		})
 	}
 }
@@ -236,7 +244,7 @@ func TestKindSorterKeepOriginalOrder(t *testing.T) {
 		var buf bytes.Buffer
 		t.Run(test.description, func(t *testing.T) {
 			defer buf.Reset()
-			for _, r := range sortByKind(manifests, test.order) {
+			for _, r := range sortManifestsByKind(manifests, test.order) {
 				buf.WriteString(r.Name)
 			}
 			if got := buf.String(); got != test.expected {
@@ -257,7 +265,7 @@ func TestKindSorterNamespaceAgainstUnknown(t *testing.T) {
 	}
 
 	manifests := []Manifest{unknown, namespace}
-	sortByKind(manifests, InstallOrder)
+	manifests = sortManifestsByKind(manifests, InstallOrder)
 
 	expectedOrder := []Manifest{namespace, unknown}
 	for i, manifest := range manifests {
@@ -266,3 +274,54 @@ func TestKindSorterNamespaceAgainstUnknown(t *testing.T) {
 		}
 	}
 }
+
+// test hook sorting with a small subset of kinds, since it uses the same algorithm as sortManifestsByKind
+func TestKindSorterForHooks(t *testing.T) {
+	hooks := []*release.Hook{
+		{
+			Name: "i",
+			Kind: "ClusterRole",
+		},
+		{
+			Name: "j",
+			Kind: "ClusterRoleBinding",
+		},
+		{
+			Name: "c",
+			Kind: "LimitRange",
+		},
+		{
+			Name: "a",
+			Kind: "Namespace",
+		},
+	}
+
+	for _, test := range []struct {
+		description string
+		order       KindSortOrder
+		expected    string
+	}{
+		{"install", InstallOrder, "acij"},
+		{"uninstall", UninstallOrder, "jica"},
+	} {
+		var buf bytes.Buffer
+		t.Run(test.description, func(t *testing.T) {
+			if got, want := len(test.expected), len(hooks); got != want {
+				t.Fatalf("Expected %d names in order, got %d", want, got)
+			}
+			defer buf.Reset()
+			orig := hooks
+			for _, r := range sortHooksByKind(hooks, test.order) {
+				buf.WriteString(r.Name)
+			}
+			for i, hook := range orig {
+				if hook != hooks[i] {
+					t.Fatal("Expected input to sortHooksByKind to stay the same")
+				}
+			}
+			if got := buf.String(); got != test.expected {
+				t.Errorf("Expected %q, got %q", test.expected, got)
+			}
+		})
+	}
+}

pkg/releaseutil/manifest_sorter.go

@@ -108,7 +108,7 @@ func SortManifests(files map[string]string, apis chartutil.VersionSet, ordering
 		}
 	}
 
-	return result.hooks, sortByKind(result.generic, ordering), nil
+	return sortHooksByKind(result.hooks, ordering), sortManifestsByKind(result.generic, ordering), nil
 }
 
 // sort takes a manifestFile object which may contain multiple resource definition

pkg/releaseutil/manifest_sorter_test.go

@@ -219,7 +219,7 @@ metadata:
 		}
 	}
 
-	sorted = sortByKind(sorted, InstallOrder)
+	sorted = sortManifestsByKind(sorted, InstallOrder)
 	for i, m := range generic {
 		if m.Content != sorted[i].Content {
 			t.Errorf("Expected %q, got %q", m.Content, sorted[i].Content)