From 5f28d63c578e495930265edfa8b155cf77a3705c Mon Sep 17 00:00:00 2001 From: Matthew Fisher Date: Thu, 30 Jan 2020 15:12:16 -0800 Subject: [PATCH] ref(kind_sorter): use an in-place sort for sortManifestsByKind, reduce code duplication Signed-off-by: Matthew Fisher Signed-off-by: Matheus Hunsche --- pkg/releaseutil/kind_sorter.go | 84 +++++++------------------ pkg/releaseutil/kind_sorter_test.go | 22 +++++-- pkg/releaseutil/manifest_sorter.go | 2 +- pkg/releaseutil/manifest_sorter_test.go | 2 +- 4 files changed, 40 insertions(+), 70 deletions(-) diff --git a/pkg/releaseutil/kind_sorter.go b/pkg/releaseutil/kind_sorter.go index 3f47ea55d..5b131b3b0 100644 --- a/pkg/releaseutil/kind_sorter.go +++ b/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 { - k := make([]string, len(manifests)) - for i, h := range manifests { - k[i] = h.Head.Kind - } - ks := newKindSorter(k, ordering) - sort.Stable(ks) +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) + }) - // apply permutation - sorted := make([]Manifest, len(manifests)) - for i, p := range ks.permutation { - sorted[i] = manifests[p] - } - return sorted + return manifests } -// 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 { - k := make([]string, len(hooks)) - for i, h := range hooks { - k[i] = h.Kind - } - - ks := newKindSorter(k, ordering) - sort.Stable(ks) - - // apply permutation - sorted := make([]*release.Hook, len(hooks)) - for i, p := range ks.permutation { - sorted[i] = hooks[p] - } - return sorted -} +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) + }) -type kindSorter struct { - permutation []int - ordering map[string]int - kinds []string + return h } -func newKindSorter(kinds []string, s KindSortOrder) *kindSorter { - o := make(map[string]int, len(s)) - for v, k := range s { - o[k] = v +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 } - 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] + 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 != b { - return a < b + if kindA != kindB { + return kindA < kindB } return first < second } diff --git a/pkg/releaseutil/kind_sorter_test.go b/pkg/releaseutil/kind_sorter_test.go index 777813667..341f528a0 100644 --- a/pkg/releaseutil/kind_sorter_test.go +++ b/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) } diff --git a/pkg/releaseutil/manifest_sorter.go b/pkg/releaseutil/manifest_sorter.go index 454977b00..e83414500 100644 --- a/pkg/releaseutil/manifest_sorter.go +++ b/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 diff --git a/pkg/releaseutil/manifest_sorter_test.go b/pkg/releaseutil/manifest_sorter_test.go index 29b0d85cf..20d809317 100644 --- a/pkg/releaseutil/manifest_sorter_test.go +++ b/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)