From 4d8160eedf43db8ed137906a8af0c5d67e03e4fe Mon Sep 17 00:00:00 2001 From: Andreas Sommer Date: Wed, 30 Oct 2019 22:22:42 +0100 Subject: [PATCH] feat(template): process manifests in file path order, then in order found in each file (before sorting manifests) Signed-off-by: Andreas Sommer --- cmd/helm/helm_test.go | 39 ++-- cmd/helm/template_test.go | 8 + cmd/helm/testdata/output/object-order.txt | 191 ++++++++++++++++++ .../testcharts/object-order/Chart.yaml | 5 + .../object-order/templates/01-a.yml | 57 ++++++ .../object-order/templates/02-b.yml | 143 +++++++++++++ .../testcharts/object-order/values.yaml | 0 pkg/releaseutil/kind_sorter.go | 8 +- pkg/releaseutil/kind_sorter_test.go | 8 +- pkg/releaseutil/manifest.go | 18 +- pkg/releaseutil/manifest_sorter.go | 29 ++- 11 files changed, 472 insertions(+), 34 deletions(-) create mode 100644 cmd/helm/testdata/output/object-order.txt create mode 100644 cmd/helm/testdata/testcharts/object-order/Chart.yaml create mode 100644 cmd/helm/testdata/testcharts/object-order/templates/01-a.yml create mode 100644 cmd/helm/testdata/testcharts/object-order/templates/02-b.yml create mode 100644 cmd/helm/testdata/testcharts/object-order/values.yaml diff --git a/cmd/helm/helm_test.go b/cmd/helm/helm_test.go index 924e8e9d3..a08720e7a 100644 --- a/cmd/helm/helm_test.go +++ b/cmd/helm/helm_test.go @@ -47,24 +47,26 @@ func init() { func runTestCmd(t *testing.T, tests []cmdTestCase) { t.Helper() for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - defer resetEnv()() - - storage := storageFixture() - for _, rel := range tt.rels { - if err := storage.Create(rel); err != nil { - t.Fatal(err) + for i := 0; i <= tt.repeat; i++ { + t.Run(tt.name, func(t *testing.T) { + defer resetEnv()() + + storage := storageFixture() + for _, rel := range tt.rels { + if err := storage.Create(rel); err != nil { + t.Fatal(err) + } } - } - t.Log("running cmd: ", tt.cmd) - _, out, err := executeActionCommandC(storage, tt.cmd) - if (err != nil) != tt.wantError { - t.Errorf("expected error, got '%v'", err) - } - if tt.golden != "" { - test.AssertGoldenString(t, out, tt.golden) - } - }) + t.Logf("running cmd (attempt %d): %s", i+1, tt.cmd) + _, out, err := executeActionCommandC(storage, tt.cmd) + if (err != nil) != tt.wantError { + t.Errorf("expected error, got '%v'", err) + } + if tt.golden != "" { + test.AssertGoldenString(t, out, tt.golden) + } + }) + } } } @@ -124,6 +126,9 @@ type cmdTestCase struct { wantError bool // Rels are the available releases at the start of the test. rels []*release.Release + // Number of repeats (in case a feature was previously flaky and the test checks + // it's now stably producing identical results). 0 means test is run exactly once. + repeat int } func executeActionCommand(cmd string) (*cobra.Command, string, error) { diff --git a/cmd/helm/template_test.go b/cmd/helm/template_test.go index 735829432..35a8e996b 100644 --- a/cmd/helm/template_test.go +++ b/cmd/helm/template_test.go @@ -84,6 +84,14 @@ func TestTemplateCmd(t *testing.T) { cmd: fmt.Sprintf("template '%s' --include-crds", chartPath), golden: "output/template-with-crds.txt", }, + { + name: "sorted output of manifests (order of filenames, then order of objects within each YAML file)", + cmd: fmt.Sprintf("template '%s'", "testdata/testcharts/object-order"), + golden: "output/object-order.txt", + // Helm previously used random file order. Repeat the test so we + // don't accidentally get the expected result. + repeat: 10, + }, } runTestCmd(t, tests) } diff --git a/cmd/helm/testdata/output/object-order.txt b/cmd/helm/testdata/output/object-order.txt new file mode 100644 index 000000000..307f928f2 --- /dev/null +++ b/cmd/helm/testdata/output/object-order.txt @@ -0,0 +1,191 @@ +--- +# Source: object-order/templates/01-a.yml +# 1 +kind: NetworkPolicy +apiVersion: networking.k8s.io/v1 +metadata: + name: first +spec: + podSelector: {} + policyTypes: + - Egress + - Ingress +--- +# Source: object-order/templates/01-a.yml +# 2 +apiVersion: networking.k8s.io/v1 +kind: NetworkPolicy +metadata: + name: second +spec: + podSelector: {} + policyTypes: + - Egress + - Ingress +--- +# Source: object-order/templates/01-a.yml +# 3 +apiVersion: networking.k8s.io/v1 +kind: NetworkPolicy +metadata: + name: third +spec: + podSelector: {} + policyTypes: + - Egress + - Ingress +--- +# Source: object-order/templates/02-b.yml +# 5 +apiVersion: networking.k8s.io/v1 +kind: NetworkPolicy +metadata: + name: fifth +spec: + podSelector: {} + policyTypes: + - Egress + - Ingress +--- +# Source: object-order/templates/02-b.yml +# 7 +apiVersion: networking.k8s.io/v1 +kind: NetworkPolicy +metadata: + name: seventh +spec: + podSelector: {} + policyTypes: + - Egress + - Ingress +--- +# Source: object-order/templates/02-b.yml +# 8 +apiVersion: networking.k8s.io/v1 +kind: NetworkPolicy +metadata: + name: eighth +spec: + podSelector: {} + policyTypes: + - Egress + - Ingress +--- +# Source: object-order/templates/02-b.yml +# 9 +apiVersion: networking.k8s.io/v1 +kind: NetworkPolicy +metadata: + name: ninth +spec: + podSelector: {} + policyTypes: + - Egress + - Ingress +--- +# Source: object-order/templates/02-b.yml +# 10 +apiVersion: networking.k8s.io/v1 +kind: NetworkPolicy +metadata: + name: tenth +spec: + podSelector: {} + policyTypes: + - Egress + - Ingress +--- +# Source: object-order/templates/02-b.yml +# 11 +apiVersion: networking.k8s.io/v1 +kind: NetworkPolicy +metadata: + name: eleventh +spec: + podSelector: {} + policyTypes: + - Egress + - Ingress +--- +# Source: object-order/templates/02-b.yml +# 12 +apiVersion: networking.k8s.io/v1 +kind: NetworkPolicy +metadata: + name: twelfth +spec: + podSelector: {} + policyTypes: + - Egress + - Ingress +--- +# Source: object-order/templates/02-b.yml +# 13 +apiVersion: networking.k8s.io/v1 +kind: NetworkPolicy +metadata: + name: thirteenth +spec: + podSelector: {} + policyTypes: + - Egress + - Ingress +--- +# Source: object-order/templates/02-b.yml +# 14 +apiVersion: networking.k8s.io/v1 +kind: NetworkPolicy +metadata: + name: fourteenth +spec: + podSelector: {} + policyTypes: + - Egress + - Ingress +--- +# Source: object-order/templates/02-b.yml +# 15 (11th object within 02-b.yml, in order to test `SplitManifests` which assigns `manifest-10` +# to this object which should then come *after* `manifest-9`) +apiVersion: networking.k8s.io/v1 +kind: NetworkPolicy +metadata: + name: fifteenth +spec: + podSelector: {} + policyTypes: + - Egress + - Ingress +--- +# Source: object-order/templates/01-a.yml +# 4 (Deployment should come after all NetworkPolicy manifests, since 'helm template' outputs in install order) +apiVersion: apps/v1 +kind: Deployment +metadata: + name: fourth +spec: + selector: + matchLabels: + pod: fourth + replicas: 1 + template: + metadata: + labels: + pod: fourth + spec: + containers: + - name: hello-world + image: gcr.io/google-samples/node-hello:1.0 +--- +# Source: object-order/templates/02-b.yml +# 6 (implementation detail: currently, 'helm template' outputs hook manifests last; and yes, NetworkPolicy won't make a reasonable hook, this is just a dummy unit test manifest) +apiVersion: networking.k8s.io/v1 +kind: NetworkPolicy +metadata: + annotations: + "helm.sh/hook": pre-install + name: sixth +spec: + podSelector: {} + policyTypes: + - Egress + - Ingress diff --git a/cmd/helm/testdata/testcharts/object-order/Chart.yaml b/cmd/helm/testdata/testcharts/object-order/Chart.yaml new file mode 100644 index 000000000..d2eb42fd7 --- /dev/null +++ b/cmd/helm/testdata/testcharts/object-order/Chart.yaml @@ -0,0 +1,5 @@ +apiVersion: v2 +name: object-order +description: Test ordering of manifests in output +type: application +version: 0.1.0 diff --git a/cmd/helm/testdata/testcharts/object-order/templates/01-a.yml b/cmd/helm/testdata/testcharts/object-order/templates/01-a.yml new file mode 100644 index 000000000..32aa4a475 --- /dev/null +++ b/cmd/helm/testdata/testcharts/object-order/templates/01-a.yml @@ -0,0 +1,57 @@ +# 1 +kind: NetworkPolicy +apiVersion: networking.k8s.io/v1 +metadata: + name: first +spec: + podSelector: {} + policyTypes: + - Egress + - Ingress + +--- + +# 2 +apiVersion: networking.k8s.io/v1 +kind: NetworkPolicy +metadata: + name: second +spec: + podSelector: {} + policyTypes: + - Egress + - Ingress + +--- + +# 3 +apiVersion: networking.k8s.io/v1 +kind: NetworkPolicy +metadata: + name: third +spec: + podSelector: {} + policyTypes: + - Egress + - Ingress + +--- + +# 4 (Deployment should come after all NetworkPolicy manifests, since 'helm template' outputs in install order) +apiVersion: apps/v1 +kind: Deployment +metadata: + name: fourth +spec: + selector: + matchLabels: + pod: fourth + replicas: 1 + template: + metadata: + labels: + pod: fourth + spec: + containers: + - name: hello-world + image: gcr.io/google-samples/node-hello:1.0 diff --git a/cmd/helm/testdata/testcharts/object-order/templates/02-b.yml b/cmd/helm/testdata/testcharts/object-order/templates/02-b.yml new file mode 100644 index 000000000..895db8cf7 --- /dev/null +++ b/cmd/helm/testdata/testcharts/object-order/templates/02-b.yml @@ -0,0 +1,143 @@ +# 5 +apiVersion: networking.k8s.io/v1 +kind: NetworkPolicy +metadata: + name: fifth +spec: + podSelector: {} + policyTypes: + - Egress + - Ingress + +--- + +# 6 (implementation detail: currently, 'helm template' outputs hook manifests last; and yes, NetworkPolicy won't make a reasonable hook, this is just a dummy unit test manifest) +apiVersion: networking.k8s.io/v1 +kind: NetworkPolicy +metadata: + annotations: + "helm.sh/hook": pre-install + name: sixth +spec: + podSelector: {} + policyTypes: + - Egress + - Ingress + +--- + +# 7 +apiVersion: networking.k8s.io/v1 +kind: NetworkPolicy +metadata: + name: seventh +spec: + podSelector: {} + policyTypes: + - Egress + - Ingress + +--- + +# 8 +apiVersion: networking.k8s.io/v1 +kind: NetworkPolicy +metadata: + name: eighth +spec: + podSelector: {} + policyTypes: + - Egress + - Ingress + +--- + +# 9 +apiVersion: networking.k8s.io/v1 +kind: NetworkPolicy +metadata: + name: ninth +spec: + podSelector: {} + policyTypes: + - Egress + - Ingress + +--- + +# 10 +apiVersion: networking.k8s.io/v1 +kind: NetworkPolicy +metadata: + name: tenth +spec: + podSelector: {} + policyTypes: + - Egress + - Ingress + +--- + +# 11 +apiVersion: networking.k8s.io/v1 +kind: NetworkPolicy +metadata: + name: eleventh +spec: + podSelector: {} + policyTypes: + - Egress + - Ingress + +--- + +# 12 +apiVersion: networking.k8s.io/v1 +kind: NetworkPolicy +metadata: + name: twelfth +spec: + podSelector: {} + policyTypes: + - Egress + - Ingress + +--- + +# 13 +apiVersion: networking.k8s.io/v1 +kind: NetworkPolicy +metadata: + name: thirteenth +spec: + podSelector: {} + policyTypes: + - Egress + - Ingress + +--- + +# 14 +apiVersion: networking.k8s.io/v1 +kind: NetworkPolicy +metadata: + name: fourteenth +spec: + podSelector: {} + policyTypes: + - Egress + - Ingress + +--- + +# 15 (11th object within 02-b.yml, in order to test `SplitManifests` which assigns `manifest-10` +# to this object which should then come *after* `manifest-9`) +apiVersion: networking.k8s.io/v1 +kind: NetworkPolicy +metadata: + name: fifteenth +spec: + podSelector: {} + policyTypes: + - Egress + - Ingress diff --git a/cmd/helm/testdata/testcharts/object-order/values.yaml b/cmd/helm/testdata/testcharts/object-order/values.yaml new file mode 100644 index 000000000..e69de29bb diff --git a/pkg/releaseutil/kind_sorter.go b/pkg/releaseutil/kind_sorter.go index a5110a100..874a47c2a 100644 --- a/pkg/releaseutil/kind_sorter.go +++ b/pkg/releaseutil/kind_sorter.go @@ -101,10 +101,10 @@ var UninstallOrder KindSortOrder = []string{ // sortByKind does an in-place sort of manifests by Kind. // -// Results are sorted by 'ordering' +// 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.Sort(ks) + sort.Stable(ks) return ks.manifests } @@ -134,13 +134,11 @@ func (k *kindSorter) Less(i, j int) bool { b := k.manifests[j] first, aok := k.ordering[a.Head.Kind] second, bok := k.ordering[b.Head.Kind] - // if same kind (including unknown) sub sort alphanumeric if first == second { // if both are unknown and of different kind sort by kind alphabetically if !aok && !bok && a.Head.Kind != b.Head.Kind { return a.Head.Kind < b.Head.Kind } - return a.Name < b.Name } // unknown kind is last if !aok { @@ -149,6 +147,6 @@ func (k *kindSorter) Less(i, j int) bool { if !bok { return true } - // sort different kinds + // sort different kinds, keep original order if same priority return first < second } diff --git a/pkg/releaseutil/kind_sorter_test.go b/pkg/releaseutil/kind_sorter_test.go index 93d8ae782..e1dfc39cc 100644 --- a/pkg/releaseutil/kind_sorter_test.go +++ b/pkg/releaseutil/kind_sorter_test.go @@ -185,8 +185,8 @@ func TestKindSorter(t *testing.T) { } } -// TestKindSorterSubSort verifies manifests of same kind are also sorted alphanumeric -func TestKindSorterSubSort(t *testing.T) { +// TestKindSorterKeepOriginalOrder verifies manifests of same kind are kept in original order +func TestKindSorterKeepOriginalOrder(t *testing.T) { manifests := []Manifest{ { Name: "a", @@ -230,8 +230,8 @@ func TestKindSorterSubSort(t *testing.T) { order KindSortOrder expected string }{ - // expectation is sorted by kind (unknown is last) and then sub sorted alphabetically within each group - {"cm,clusterRole,clusterRoleBinding,Unknown,Unknown2", InstallOrder, "01Aa!zu1u2t3"}, + // expectation is sorted by kind (unknown is last) and within each group of same kind, the order is kept + {"cm,clusterRole,clusterRoleBinding,Unknown,Unknown2", InstallOrder, "01aAz!u2u1t3"}, } { var buf bytes.Buffer t.Run(test.description, func(t *testing.T) { diff --git a/pkg/releaseutil/manifest.go b/pkg/releaseutil/manifest.go index 37503b390..0b04a4599 100644 --- a/pkg/releaseutil/manifest.go +++ b/pkg/releaseutil/manifest.go @@ -19,6 +19,7 @@ package releaseutil import ( "fmt" "regexp" + "strconv" "strings" ) @@ -37,8 +38,9 @@ var sep = regexp.MustCompile("(?:^|\\s*\n)---\\s*") // SplitManifests takes a string of manifest and returns a map contains individual manifests func SplitManifests(bigFile string) map[string]string { // Basically, we're quickly splitting a stream of YAML documents into an - // array of YAML docs. In the current implementation, the file name is just - // a place holder, and doesn't have any further meaning. + // array of YAML docs. The file name is just a place holder, but should be + // integer-sortable so that manifests get output in the same order as the + // input (see `BySplitManifestsOrder`). tpl := "manifest-%d" res := map[string]string{} // Making sure that any extra whitespace in YAML stream doesn't interfere in splitting documents correctly. @@ -56,3 +58,15 @@ func SplitManifests(bigFile string) map[string]string { } return res } + +// BySplitManifestsOrder sorts by in-file manifest order, as provided in function `SplitManifests` +type BySplitManifestsOrder []string + +func (a BySplitManifestsOrder) Len() int { return len(a) } +func (a BySplitManifestsOrder) Less(i, j int) bool { + // Split `manifest-%d` + anum, _ := strconv.ParseInt(a[i][len("manifest-"):], 10, 0) + bnum, _ := strconv.ParseInt(a[j][len("manifest-"):], 10, 0) + return anum < bnum +} +func (a BySplitManifestsOrder) Swap(i, j int) { a[i], a[j] = a[j], a[i] } diff --git a/pkg/releaseutil/manifest_sorter.go b/pkg/releaseutil/manifest_sorter.go index 5f4b4e8d9..24b0c3c95 100644 --- a/pkg/releaseutil/manifest_sorter.go +++ b/pkg/releaseutil/manifest_sorter.go @@ -19,6 +19,7 @@ package releaseutil import ( "log" "path" + "sort" "strconv" "strings" @@ -74,10 +75,17 @@ var events = map[string]release.HookEvent{ // // Files that do not parse into the expected format are simply placed into a map and // returned. -func SortManifests(files map[string]string, apis chartutil.VersionSet, sort KindSortOrder) ([]*release.Hook, []Manifest, error) { +func SortManifests(files map[string]string, apis chartutil.VersionSet, ordering KindSortOrder) ([]*release.Hook, []Manifest, error) { result := &result{} - for filePath, c := range files { + var sortedFilePaths []string + for filePath := range files { + sortedFilePaths = append(sortedFilePaths, filePath) + } + sort.Strings(sortedFilePaths) + + for _, filePath := range sortedFilePaths { + content := files[filePath] // Skip partials. We could return these as a separate map, but there doesn't // seem to be any need for that at this time. @@ -85,12 +93,12 @@ func SortManifests(files map[string]string, apis chartutil.VersionSet, sort Kind continue } // Skip empty files and log this. - if strings.TrimSpace(c) == "" { + if strings.TrimSpace(content) == "" { continue } manifestFile := &manifestFile{ - entries: SplitManifests(c), + entries: SplitManifests(content), path: filePath, apis: apis, } @@ -100,7 +108,7 @@ func SortManifests(files map[string]string, apis chartutil.VersionSet, sort Kind } } - return result.hooks, sortByKind(result.generic, sort), nil + return result.hooks, sortByKind(result.generic, ordering), nil } // sort takes a manifestFile object which may contain multiple resource definition @@ -123,7 +131,16 @@ func SortManifests(files map[string]string, apis chartutil.VersionSet, sort Kind // annotations: // helm.sh/hook-delete-policy: hook-succeeded func (file *manifestFile) sort(result *result) error { - for _, m := range file.entries { + // Go through manifests in order found in file (function `SplitManifests` creates integer-sortable keys) + var sortedEntryKeys []string + for entryKey := range file.entries { + sortedEntryKeys = append(sortedEntryKeys, entryKey) + } + sort.Sort(BySplitManifestsOrder(sortedEntryKeys)) + + for _, entryKey := range sortedEntryKeys { + m := file.entries[entryKey] + var entry SimpleHead if err := yaml.Unmarshal([]byte(m), &entry); err != nil { return errors.Wrapf(err, "YAML parse error on %s", file.path)