From 5c8843950763dec9ac04eb4cba9a300d08f9bdc8 Mon Sep 17 00:00:00 2001 From: Roberto Santalla Date: Thu, 17 Mar 2022 14:59:59 +0100 Subject: [PATCH] releaseutil/kind_sorter: sort hooks by weight, then kind, then name. Co-authored-by: Alvaro Cabanas Co-authored-by: Paolo Gallina Signed-off-by: Roberto Santalla --- pkg/action/hooks.go | 1 + pkg/releaseutil/kind_sorter.go | 19 +++++-- pkg/releaseutil/kind_sorter_test.go | 88 ++++++++++++++++++++++------- pkg/releaseutil/manifest_sorter.go | 2 +- 4 files changed, 85 insertions(+), 25 deletions(-) diff --git a/pkg/action/hooks.go b/pkg/action/hooks.go index fde37b825..90f9945c1 100644 --- a/pkg/action/hooks.go +++ b/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" ) diff --git a/pkg/releaseutil/kind_sorter.go b/pkg/releaseutil/kind_sorter.go index 1d1874cfa..af21475e3 100644 --- a/pkg/releaseutil/kind_sorter.go +++ b/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 } diff --git a/pkg/releaseutil/kind_sorter_test.go b/pkg/releaseutil/kind_sorter_test.go index afcae6d16..1e93be314 100644 --- a/pkg/releaseutil/kind_sorter_test.go +++ b/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) } diff --git a/pkg/releaseutil/manifest_sorter.go b/pkg/releaseutil/manifest_sorter.go index e83414500..9e7d82eee 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 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