upstream/helm
1
0
Fork 0
mirror of https://github.com/helm/helm.git synced 2026-05-28 04:35:48 -04:00
Code Issues Projects Releases Packages Wiki Activity Actions 17

feat(template): process manifests in file path order, then in order found in each file (before sorting manifests)

Browse source 
Signed-off-by: Andreas Sommer <andreas.sommer87@googlemail.com>
This commit is contained in:
Andreas Sommer 2019-10-30 22:22:42 +01:00
parent f4f5b2e46f
commit 4d8160eedf
11 changed files with 471 additions and 33 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

37
cmd/helm/helm_test.go
View file

@ -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()()
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)
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) {

8
cmd/helm/template_test.go
View file

@ -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)
}

191
cmd/helm/testdata/output/object-order.txt vendored Normal file
View file

@ -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

5
cmd/helm/testdata/testcharts/object-order/Chart.yaml vendored Normal file
View file

@ -0,0 +1,5 @@
apiVersion: v2
name: object-order
description: Test ordering of manifests in output
type: application
version: 0.1.0

57
cmd/helm/testdata/testcharts/object-order/templates/01-a.yml vendored Normal file
View file

@ -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

143
cmd/helm/testdata/testcharts/object-order/templates/02-b.yml vendored Normal file
View file

@ -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

0
cmd/helm/testdata/testcharts/object-order/values.yaml vendored Normal file
View file

8
pkg/releaseutil/kind_sorter.go
View file

@ -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
}

8
pkg/releaseutil/kind_sorter_test.go
View file

@ -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) {

18
pkg/releaseutil/manifest.go
View file

@ -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] }

29
pkg/releaseutil/manifest_sorter.go
View file

@ -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)