ref(kind_sorter): use an in-place sort for sortManifestsByKind, reduce code duplication

Signed-off-by: Matthew Fisher <matt.fisher@microsoft.com>

pkg/releaseutil/kind_sorter.go

@@ -103,84 +103,42 @@ var UninstallOrder KindSortOrder = []string{
 	"Namespace",
 }
 
-// sort manifests by kind (out of place sort)
+// sort manifests by kind.
 //
 // Results are sorted by 'ordering', keeping order of items with equal kind/priority
-func manifestsSortedByKind(manifests []Manifest, ordering KindSortOrder) []Manifest {
+func sortManifestsByKind(manifests []Manifest, ordering KindSortOrder) []Manifest {
-	k := make([]string, len(manifests))
+	sort.SliceStable(manifests, func(i, j int) bool {
-	for i, h := range manifests {
+		return lessByKind(manifests[i], manifests[j], manifests[i].Head.Kind, manifests[j].Head.Kind, ordering)
-		k[i] = h.Head.Kind
+	})
-	}
-	ks := newKindSorter(k, ordering)
-	sort.Stable(ks)
 
-	// apply permutation
+	return manifests
-	sorted := make([]Manifest, len(manifests))
-	for i, p := range ks.permutation {
-		sorted[i] = manifests[p]
-	}
-	return sorted
 }
 
-// sort hooks by kind (out of place sort)
+// 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 hooksSortedByKind(hooks []*release.Hook, ordering KindSortOrder) []*release.Hook {
+func sortHooksByKind(hooks []*release.Hook, ordering KindSortOrder) []*release.Hook {
-	k := make([]string, len(hooks))
+	h := hooks
-	for i, h := range hooks {
+	sort.SliceStable(h, func(i, j int) bool {
-		k[i] = h.Kind
+		return lessByKind(h[i], h[j], h[i].Kind, h[j].Kind, ordering)
+	})
+
+	return h
 }
 
-	ks := newKindSorter(k, ordering)
+func lessByKind(a interface{}, b interface{}, kindA string, kindB string, o KindSortOrder) bool {
-	sort.Stable(ks)
+	ordering := make(map[string]int, len(o))
-
+	for v, k := range o {
-	// apply permutation
+		ordering[k] = v
-	sorted := make([]*release.Hook, len(hooks))
-	for i, p := range ks.permutation {
-		sorted[i] = hooks[p]
-	}
-	return sorted
 	}
 
-type kindSorter struct {
+	first, aok := ordering[kindA]
-	permutation []int
+	second, bok := ordering[kindB]
-	ordering    map[string]int
-	kinds       []string
-}
-
-func newKindSorter(kinds []string, s KindSortOrder) *kindSorter {
-	o := make(map[string]int, len(s))
-	for v, k := range s {
-		o[k] = v
-	}
-
-	p := make([]int, len(kinds))
-	for i := range p {
-		p[i] = i
-	}
-	return &kindSorter{
-		permutation: p,
-		kinds:       kinds,
-		ordering:    o,
-	}
-}
-
-func (k *kindSorter) Len() int { return len(k.kinds) }
-
-func (k *kindSorter) Swap(i, j int) {
-	k.permutation[i], k.permutation[j] = k.permutation[j], k.permutation[i]
-}
-
-func (k *kindSorter) Less(i, j int) bool {
-	a := k.kinds[k.permutation[i]]
-	b := k.kinds[k.permutation[j]]
-	first, aok := k.ordering[a]
-	second, bok := k.ordering[b]
 
 	if !aok && !bok {
 		// if both are unknown then sort alphabetically by kind, keep original order if same kind
-		if a != b {
+		if kindA != kindB {
-			return a < b
+			return kindA < kindB
 		}
 		return first < second
 	}

pkg/releaseutil/kind_sorter_test.go

@@ -177,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 manifestsSortedByKind(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")
+				}
+			}
 		})
 	}
 }
@@ -238,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 manifestsSortedByKind(manifests, test.order) {
+			for _, r := range sortManifestsByKind(manifests, test.order) {
 				buf.WriteString(r.Name)
 			}
 			if got := buf.String(); got != test.expected {
@@ -259,7 +265,7 @@ func TestKindSorterNamespaceAgainstUnknown(t *testing.T) {
 	}
 
 	manifests := []Manifest{unknown, namespace}
-	manifests = manifestsSortedByKind(manifests, InstallOrder)
+	manifests = sortManifestsByKind(manifests, InstallOrder)
 
 	expectedOrder := []Manifest{namespace, unknown}
 	for i, manifest := range manifests {
@@ -269,7 +275,7 @@ func TestKindSorterNamespaceAgainstUnknown(t *testing.T) {
 	}
 }
 
-// test hook sorting with a small subset of kinds, since it uses the same algorithm as manifestsSortedByKind
+// 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{
 		{
@@ -304,9 +310,15 @@ func TestKindSorterForHooks(t *testing.T) {
 				t.Fatalf("Expected %d names in order, got %d", want, got)
 			}
 			defer buf.Reset()
-			for _, r := range hooksSortedByKind(hooks, test.order) {
+			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 hooksSortedByKind(result.hooks, ordering), manifestsSortedByKind(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 = manifestsSortedByKind(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)