releaseutil/kind_sorter: sort hooks by weight, then kind, then name.

Co-authored-by: Alvaro Cabanas <acabanas@newrelic.com>
Co-authored-by: Paolo Gallina <paologallina1992@gmail.com>
Signed-off-by: Roberto Santalla <roobre@roobre.es>

pkg/action/hooks.go

@@ -20,6 +20,7 @@ import (
 	"time"
 
 	"github.com/pkg/errors"
+
 	"helm.sh/helm/v3/pkg/release"
 	helmtime "helm.sh/helm/v3/pkg/time"
 )

pkg/releaseutil/kind_sorter.go

@@ -118,15 +118,26 @@ func sortManifestsByKind(manifests []Manifest, ordering KindSortOrder) []Manifes
 	return manifests
 }
 
-// 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 {
+// sortHooks sorts hooks by weight, kind, and finally by name.
+// Kind order is defined by ordering.
+func sortHooks(hooks []*release.Hook, ordering KindSortOrder) []*release.Hook {
 	h := hooks
+
+	// Sort first by name, the least important ordering.
+	sort.Slice(h, func(i, j int) bool {
+		return h[i].Name < h[j].Name
+	})
+
+	// Then sort by kind, keeping equal elements in their original order (Stable).
 	sort.SliceStable(h, func(i, j int) bool {
 		return lessByKind(h[i], h[j], h[i].Kind, h[j].Kind, ordering)
 	})
 
+	// Finally, sort by weight, again keeping equal elements in their original order.
+	sort.SliceStable(h, func(i, j int) bool {
+		return h[i].Weight < h[j].Weight
+	})
+
 	return h
 }
 

pkg/releaseutil/kind_sorter_test.go

@@ -284,8 +284,17 @@ 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{
+func TestSortHooks(t *testing.T) {
+	type testCase struct {
+		description string
+		hooks       []*release.Hook
+		order       KindSortOrder
+		expected    string
+	}
+
+	// hooksInstallUninstall is a list of hooks with Kinds that need to be installed in one order and uninstalled
+	// in the reverse order.
+	hooksInstallUninstall := []*release.Hook{
 		{
 			Name: "i",
 			Kind: "ClusterRole",
@@ -304,29 +313,68 @@ func TestKindSorterForHooks(t *testing.T) {
 		},
 	}
 
-	for _, test := range []struct {
-		description string
-		order       KindSortOrder
-		expected    string
-	}{
-		{"install", InstallOrder, "acij"},
-		{"uninstall", UninstallOrder, "jica"},
+	installCase := testCase{
+		description: "install_order_wihtout_weight",
+		order:       InstallOrder,
+		expected:    "acij",
+		hooks:       hooksInstallUninstall,
+	}
+	uninstallCase := testCase{
+		description: "uninstall_order_wihtout_weight",
+		order:       UninstallOrder,
+		expected:    "jica",
+		hooks:       hooksInstallUninstall,
+	}
+
+	// compositeOrderCase is a test case to check that order priorities is applied correctly:
+	// First by weight, then by kind, finally by name.
+	compositeOrderCase := testCase{
+		description: "install_order_composite",
+		order:       InstallOrder,
+		expected:    "pod_00_sa_01_pod_02_job_03_job_04_",
+		hooks: []*release.Hook{
+			{
+				// Pod goes third by Kind order.
+				Name: "pod_02_",
+				Kind: "Pod",
+			},
+			{
+				// ServiceAccount goes second, as per Kind InstallOrder.
+				Name: "sa_01_",
+				Kind: "ServiceAccount",
+			},
+			{
+				// Job goes fourth by Kind order.
+				Name: "job_03_",
+				Kind: "Job",
+			},
+			{
+				// This pod will go first as it has -1 weight.
+				Name:   "pod_00_",
+				Kind:   "Pod",
+				Weight: -1,
+			},
+			{
+				// This job goes fifth as their name comes later than the other Job.
+				Name: "job_04_",
+				Kind: "Job",
+			},
+		},
+	}
+
+	for _, test := range []testCase{
+		installCase,
+		uninstallCase,
+		compositeOrderCase,
 	} {
-		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)
-			}
+			buf := bytes.Buffer{}
 			defer buf.Reset()
-			orig := hooks
-			for _, r := range sortHooksByKind(hooks, test.order) {
+
+			for _, r := range sortHooks(test.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 sortHooksByKind(result.hooks, ordering), sortManifestsByKind(result.generic, ordering), nil
+	return sortHooks(result.hooks, ordering), sortManifestsByKind(result.generic, ordering), nil
 }
 
 // sort takes a manifestFile object which may contain multiple resource definition