From 0e661611615de09ff826e1fd4e7a5a6bf747fc2e Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Thu, 19 May 2022 10:32:52 +0200 Subject: [PATCH 01/52] Podman build --- hack/build-podman.sh | 5 +++++ 1 file changed, 5 insertions(+) create mode 100755 hack/build-podman.sh diff --git a/hack/build-podman.sh b/hack/build-podman.sh new file mode 100755 index 000000000..e322a7a2b --- /dev/null +++ b/hack/build-podman.sh @@ -0,0 +1,5 @@ +#!/usr/bin/env bash + +cd $(dirname ${BASH_SOURCE[0]}) + +podman run --rm -v gopath:/go -v gobuild:/root/.cache/go-build -v $(realpath $PWD/..):/mnt -w /mnt docker.io/golang hack/build.sh From 1289a919a550d6346c0b4e92b9acd8b04ba239d7 Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Thu, 19 May 2022 11:35:22 +0200 Subject: [PATCH 02/52] Refactor podman script to run tests and other scripts --- hack/build-podman.sh | 2 +- hack/podman.sh | 5 +++++ hack/test-podman.sh | 5 +++++ 3 files changed, 11 insertions(+), 1 deletion(-) create mode 100755 hack/podman.sh create mode 100755 hack/test-podman.sh diff --git a/hack/build-podman.sh b/hack/build-podman.sh index e322a7a2b..a5675b51f 100755 --- a/hack/build-podman.sh +++ b/hack/build-podman.sh @@ -2,4 +2,4 @@ cd $(dirname ${BASH_SOURCE[0]}) -podman run --rm -v gopath:/go -v gobuild:/root/.cache/go-build -v $(realpath $PWD/..):/mnt -w /mnt docker.io/golang hack/build.sh +./podman.sh hack/build.sh diff --git a/hack/podman.sh b/hack/podman.sh new file mode 100755 index 000000000..d5211c71f --- /dev/null +++ b/hack/podman.sh @@ -0,0 +1,5 @@ +#!/usr/bin/env bash + +cd $(dirname ${BASH_SOURCE[0]}) + +podman run --rm -v gopath:/go -v gobuild:/root/.cache/go-build -v $(realpath $PWD/..):/mnt -w /mnt docker.io/golang $1 diff --git a/hack/test-podman.sh b/hack/test-podman.sh new file mode 100755 index 000000000..0acf44690 --- /dev/null +++ b/hack/test-podman.sh @@ -0,0 +1,5 @@ +#!/usr/bin/env bash + +cd $(dirname ${BASH_SOURCE[0]}) + +./podman.sh hack/test.sh From 7768d601172a5e47c0171f789bd9ad7190099bef Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Fri, 20 May 2022 11:08:03 +0200 Subject: [PATCH 03/52] Add script shortcuts to podman scripts --- hack/build-podman.sh | 2 +- hack/podman.sh | 6 +++++- hack/test-podman.sh | 2 +- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/hack/build-podman.sh b/hack/build-podman.sh index a5675b51f..6a244a1d2 100755 --- a/hack/build-podman.sh +++ b/hack/build-podman.sh @@ -2,4 +2,4 @@ cd $(dirname ${BASH_SOURCE[0]}) -./podman.sh hack/build.sh +./podman.sh build diff --git a/hack/podman.sh b/hack/podman.sh index d5211c71f..a0d065e78 100755 --- a/hack/podman.sh +++ b/hack/podman.sh @@ -2,4 +2,8 @@ cd $(dirname ${BASH_SOURCE[0]}) -podman run --rm -v gopath:/go -v gobuild:/root/.cache/go-build -v $(realpath $PWD/..):/mnt -w /mnt docker.io/golang $1 +action=$1 + +[ -x "$action.sh" ] && action=hack/$action.sh + +podman run --rm -v gopath:/go -v gobuild:/root/.cache/go-build -v $(realpath $PWD/..):/mnt -w /mnt docker.io/golang $action diff --git a/hack/test-podman.sh b/hack/test-podman.sh index 0acf44690..86da65e9e 100755 --- a/hack/test-podman.sh +++ b/hack/test-podman.sh @@ -2,4 +2,4 @@ cd $(dirname ${BASH_SOURCE[0]}) -./podman.sh hack/test.sh +./podman.sh test From 5eadc822a074be22659c55cbbb14facf47606b29 Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Sun, 29 May 2022 10:48:03 +0200 Subject: [PATCH 04/52] Demonstrate nameHashSuffix strip --- .../diff/change_set_with_versioned_rs_test.go | 49 +++++++++++++++++++ pkg/kapp/diff/versioned_resource.go | 17 ++++++- 2 files changed, 65 insertions(+), 1 deletion(-) diff --git a/pkg/kapp/diff/change_set_with_versioned_rs_test.go b/pkg/kapp/diff/change_set_with_versioned_rs_test.go index 12774d3db..e4deea7f6 100644 --- a/pkg/kapp/diff/change_set_with_versioned_rs_test.go +++ b/pkg/kapp/diff/change_set_with_versioned_rs_test.go @@ -157,6 +157,55 @@ metadata: 4, 7 ` checkChangeDiff(t, changes[1], expectedDiff2) + +} + +func TestChangeSet_StripKustomizeSuffix(t *testing.T){ + stripNameHashSuffix = true + newRs := ctlres.MustNewResourceFromBytes([]byte(` +apiVersion: v1 +kind: ConfigMap +metadata: + annotations: + kapp.k14s.io/versioned: "" + name: configmap-abc +data: + foo: bar +`)) + + existingRs := ctlres.MustNewResourceFromBytes([]byte(` +apiVersion: v1 +kind: ConfigMap +metadata: + annotations: + kapp.k14s.io/versioned: "" + name: configmap-ver-1 +data: + foo: foo +`)) + + changeSetWithVerRes := NewChangeSetWithVersionedRs([]ctlres.Resource{existingRs}, []ctlres.Resource{newRs}, nil, ChangeSetOpts{}, ChangeFactory{}) + + changes, err := changeSetWithVerRes.Calculate() + require.NoError(t, err) + + //require.Equal(t, ChangeOpUpdate, changes[0].Op(), "Expect to get updated") + + expectedDiff := ` 0, 0 apiVersion: v1 + 1, 1 data: + 2, 2 - foo: foo + 3, 2 + foo: bar + 3, 3 kind: ConfigMap + 4, 4 metadata: + 5, 5 annotations: + 6, 6 kapp.k14s.io/versioned: "" + 7, 7 - name: configmap-ver-1 + 8, 7 + name: configmap-ver-2 + 8, 8 +` + + checkChangeDiff(t, changes[0], expectedDiff) + } func checkChangeDiff(t *testing.T, change Change, expectedDiff string) { diff --git a/pkg/kapp/diff/versioned_resource.go b/pkg/kapp/diff/versioned_resource.go index ba1cd0dbb..8455a7aab 100644 --- a/pkg/kapp/diff/versioned_resource.go +++ b/pkg/kapp/diff/versioned_resource.go @@ -18,13 +18,22 @@ const ( nameSuffixSep = "-ver-" ) +var ( + stripNameHashSuffix = false +) + type VersionedResource struct { res ctlres.Resource allRules []ctlconf.TemplateRule } +func (d VersionedResource) StripNameHashSuffix() (bool) { + return stripNameHashSuffix +} + func (d VersionedResource) SetBaseName(ver int) { - name := fmt.Sprintf("%s%s%d", d.res.Name(), nameSuffixSep, ver) + currentName, _ := d.BaseNameAndVersion() + name := fmt.Sprintf("%s%s%d", currentName, nameSuffixSep, ver) d.res.SetName(name) } @@ -34,6 +43,12 @@ func (d VersionedResource) BaseNameAndVersion() (string, string) { if len(pieces) > 1 { return strings.Join(pieces[0:len(pieces)-1], nameSuffixSep), pieces[len(pieces)-1] } + if d.StripNameHashSuffix() { + pieces = strings.Split(name, "-") + if len(pieces) > 1 { + return strings.Join(pieces[0:len(pieces)-1], "-"), "" + } + } return name, "" } From 3376aff823eef3d82179436ad849a6298f5e09e2 Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Sun, 29 May 2022 10:49:11 +0200 Subject: [PATCH 05/52] Podman interactive e.g. for dlv debugging --- hack/podman.sh | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/hack/podman.sh b/hack/podman.sh index a0d065e78..809ed84cf 100755 --- a/hack/podman.sh +++ b/hack/podman.sh @@ -4,6 +4,10 @@ cd $(dirname ${BASH_SOURCE[0]}) action=$1 -[ -x "$action.sh" ] && action=hack/$action.sh +if [ "$action" ]; then + [ -x "$action.sh" ] && action=hack/$action.sh +else + interactive="-it" +fi -podman run --rm -v gopath:/go -v gobuild:/root/.cache/go-build -v $(realpath $PWD/..):/mnt -w /mnt docker.io/golang $action +podman run --rm -v gopath:/go -v gobuild:/root/.cache/go-build -v $(realpath $PWD/..):/mnt -w /mnt $interactive --cap-add SYS_PTRACE docker.io/golang $action From 12c3862a74b0bcd84308d96495625f022e9d08dc Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Sun, 29 May 2022 13:16:10 +0200 Subject: [PATCH 06/52] Move suffix-strip flag to change set type --- pkg/kapp/diff/change_set_with_versioned_rs.go | 27 +++++++++++-------- .../diff/change_set_with_versioned_rs_test.go | 4 +-- pkg/kapp/diff/versioned_resource.go | 13 ++++----- 3 files changed, 23 insertions(+), 21 deletions(-) diff --git a/pkg/kapp/diff/change_set_with_versioned_rs.go b/pkg/kapp/diff/change_set_with_versioned_rs.go index 804d49946..c7d7c5d72 100644 --- a/pkg/kapp/diff/change_set_with_versioned_rs.go +++ b/pkg/kapp/diff/change_set_with_versioned_rs.go @@ -19,16 +19,17 @@ const ( ) type ChangeSetWithVersionedRs struct { - existingRs, newRs []ctlres.Resource - rules []ctlconf.TemplateRule - opts ChangeSetOpts - changeFactory ChangeFactory + existingRs, newRs []ctlres.Resource + rules []ctlconf.TemplateRule + opts ChangeSetOpts + changeFactory ChangeFactory + stripNameHashSuffix bool } func NewChangeSetWithVersionedRs(existingRs, newRs []ctlres.Resource, rules []ctlconf.TemplateRule, opts ChangeSetOpts, changeFactory ChangeFactory) *ChangeSetWithVersionedRs { - return &ChangeSetWithVersionedRs{existingRs, newRs, rules, opts, changeFactory} + return &ChangeSetWithVersionedRs{existingRs, newRs, rules, opts, changeFactory, false /* TODO */} } func (d ChangeSetWithVersionedRs) Calculate() ([]Change, error) { @@ -76,19 +77,23 @@ func (d ChangeSetWithVersionedRs) Calculate() ([]Change, error) { return allChanges, nil } +func (d ChangeSetWithVersionedRs) versionedResourceName(res ctlres.Resource) VersionedResource { + return VersionedResource{res, nil, d.stripNameHashSuffix} +} + func (d ChangeSetWithVersionedRs) groupResources(rs []ctlres.Resource) map[string][]ctlres.Resource { result := map[string][]ctlres.Resource{} groupByFunc := func(res ctlres.Resource) string { if _, found := res.Annotations()[versionedResAnnKey]; found { - return VersionedResource{res, nil}.UniqVersionedKey().String() + return d.versionedResourceName(res).UniqVersionedKey().String() } panic("Expected to find versioned annotation on resource") } for resKey, subRs := range (GroupResources{rs, groupByFunc}).Resources() { sort.Slice(subRs, func(i, j int) bool { - return VersionedResource{subRs[i], nil}.Version() < VersionedResource{subRs[j], nil}.Version() + return d.versionedResourceName(subRs[i]).Version() < d.versionedResourceName(subRs[j]).Version() }) result[resKey] = subRs } @@ -101,12 +106,12 @@ func (d ChangeSetWithVersionedRs) assignNewNames( // TODO name isnt used during diffing, should it? for _, newRes := range newRs.Versioned { - newVerRes := VersionedResource{newRes, nil} + newVerRes := d.versionedResourceName(newRes) newResKey := newVerRes.UniqVersionedKey().String() if existingRs, found := existingRsGrouped[newResKey]; found { existingRes := existingRs[len(existingRs)-1] - newVerRes.SetBaseName(VersionedResource{existingRes, nil}.Version() + 1) + newVerRes.SetBaseName(d.versionedResourceName(existingRes).Version() + 1) } else { newVerRes.SetBaseName(1) } @@ -121,7 +126,7 @@ func (d ChangeSetWithVersionedRs) addAndKeepChanges( alreadyAdded := map[string]ctlres.Resource{} for _, newRes := range newRs.Versioned { - newResKey := VersionedResource{newRes, nil}.UniqVersionedKey().String() + newResKey := d.versionedResourceName(newRes).UniqVersionedKey().String() usedRes := newRes if existingRs, found := existingRsGrouped[newResKey]; found { @@ -153,7 +158,7 @@ func (d ChangeSetWithVersionedRs) addAndKeepChanges( } // Update both versioned and non-versioned - verRes := VersionedResource{usedRes, d.rules} + verRes := VersionedResource{usedRes, d.rules, d.stripNameHashSuffix} err := verRes.UpdateAffected(newRs.NonVersioned) if err != nil { diff --git a/pkg/kapp/diff/change_set_with_versioned_rs_test.go b/pkg/kapp/diff/change_set_with_versioned_rs_test.go index e4deea7f6..a5a637184 100644 --- a/pkg/kapp/diff/change_set_with_versioned_rs_test.go +++ b/pkg/kapp/diff/change_set_with_versioned_rs_test.go @@ -160,8 +160,7 @@ metadata: } -func TestChangeSet_StripKustomizeSuffix(t *testing.T){ - stripNameHashSuffix = true +func TestChangeSet_StripKustomizeSuffix(t *testing.T) { newRs := ctlres.MustNewResourceFromBytes([]byte(` apiVersion: v1 kind: ConfigMap @@ -185,6 +184,7 @@ data: `)) changeSetWithVerRes := NewChangeSetWithVersionedRs([]ctlres.Resource{existingRs}, []ctlres.Resource{newRs}, nil, ChangeSetOpts{}, ChangeFactory{}) + changeSetWithVerRes.stripNameHashSuffix = true changes, err := changeSetWithVerRes.Calculate() require.NoError(t, err) diff --git a/pkg/kapp/diff/versioned_resource.go b/pkg/kapp/diff/versioned_resource.go index 8455a7aab..b614e8772 100644 --- a/pkg/kapp/diff/versioned_resource.go +++ b/pkg/kapp/diff/versioned_resource.go @@ -18,17 +18,14 @@ const ( nameSuffixSep = "-ver-" ) -var ( - stripNameHashSuffix = false -) - type VersionedResource struct { - res ctlres.Resource - allRules []ctlconf.TemplateRule + res ctlres.Resource + allRules []ctlconf.TemplateRule + stripNameHashSuffix bool } -func (d VersionedResource) StripNameHashSuffix() (bool) { - return stripNameHashSuffix +func (d VersionedResource) StripNameHashSuffix() bool { + return d.stripNameHashSuffix } func (d VersionedResource) SetBaseName(ver int) { From 0099750981de750a7f3add975664c6be1ff41fd9 Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Sun, 29 May 2022 13:17:28 +0200 Subject: [PATCH 07/52] Add note about SYS_PTRACE --- hack/podman.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/hack/podman.sh b/hack/podman.sh index 809ed84cf..ed803a12d 100755 --- a/hack/podman.sh +++ b/hack/podman.sh @@ -10,4 +10,5 @@ else interactive="-it" fi +# SYS_PTRACE for dlv podman run --rm -v gopath:/go -v gobuild:/root/.cache/go-build -v $(realpath $PWD/..):/mnt -w /mnt $interactive --cap-add SYS_PTRACE docker.io/golang $action From 9b32450645ad5e2e453547fccba3a5df81fd2234 Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Sun, 29 May 2022 13:28:34 +0200 Subject: [PATCH 08/52] Add test against suffix-stripping unsupported resources --- .../diff/change_set_with_versioned_rs_test.go | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/pkg/kapp/diff/change_set_with_versioned_rs_test.go b/pkg/kapp/diff/change_set_with_versioned_rs_test.go index a5a637184..b84a537ea 100644 --- a/pkg/kapp/diff/change_set_with_versioned_rs_test.go +++ b/pkg/kapp/diff/change_set_with_versioned_rs_test.go @@ -208,6 +208,47 @@ data: } +func TestChangeSet_StripKustomizeSuffix_CMonly(t *testing.T) { + newRs := ctlres.MustNewResourceFromBytes([]byte(` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: my-deployment +spec: + replicas: 2 +`)) + + existingRs := ctlres.MustNewResourceFromBytes([]byte(` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: my-deployment +spec: + replicas: 1 +`)) + + changeSetWithVerRes := NewChangeSetWithVersionedRs([]ctlres.Resource{existingRs}, []ctlres.Resource{newRs}, nil, ChangeSetOpts{}, ChangeFactory{}) + changeSetWithVerRes.stripNameHashSuffix = true + + changes, err := changeSetWithVerRes.Calculate() + require.NoError(t, err) + + require.Equal(t, ChangeOpUpdate, changes[0].Op(), "Expect to get updated") + + expectedDiff := ` 0, 0 apiVersion: apps/v1 + 1, 1 kind: Deployment + 2, 2 metadata: + 3, 3 name: my-deployment + 4, 4 spec: + 5, 5 - replicas: 1 + 6, 5 + replicas: 2 + 6, 6 +` + + checkChangeDiff(t, changes[0], expectedDiff) + +} + func checkChangeDiff(t *testing.T, change Change, expectedDiff string) { actualDiffString := change.ConfigurableTextDiff().Full().FullString() From eed12ec21bfa16dc30d9bd32325c2a92d886cb5d Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Sun, 29 May 2022 13:28:49 +0200 Subject: [PATCH 09/52] Add note about disabled check --- pkg/kapp/diff/change_set_with_versioned_rs_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/kapp/diff/change_set_with_versioned_rs_test.go b/pkg/kapp/diff/change_set_with_versioned_rs_test.go index b84a537ea..6d268eda5 100644 --- a/pkg/kapp/diff/change_set_with_versioned_rs_test.go +++ b/pkg/kapp/diff/change_set_with_versioned_rs_test.go @@ -189,7 +189,7 @@ data: changes, err := changeSetWithVerRes.Calculate() require.NoError(t, err) - //require.Equal(t, ChangeOpUpdate, changes[0].Op(), "Expect to get updated") + //require.Equal(t, ChangeOpUpdate, changes[0].Op(), "Expect to get updated") // TODO: check why this is add, not update expectedDiff := ` 0, 0 apiVersion: v1 1, 1 data: From 5b4255cf8455f91b9f4905b56a4e5a08f0a68f76 Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Sun, 29 May 2022 13:55:04 +0200 Subject: [PATCH 10/52] Fix ChangeSetWithVersionedRs tests --- .../diff/change_set_with_versioned_rs_test.go | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/pkg/kapp/diff/change_set_with_versioned_rs_test.go b/pkg/kapp/diff/change_set_with_versioned_rs_test.go index 6d268eda5..467d9492a 100644 --- a/pkg/kapp/diff/change_set_with_versioned_rs_test.go +++ b/pkg/kapp/diff/change_set_with_versioned_rs_test.go @@ -27,9 +27,7 @@ metadata: kapp.k14s.io/versioned: "" `)) - changeSetWithVerRes := NewChangeSetWithVersionedRs([]ctlres.Resource{existingRes}, []ctlres.Resource{newRs}, nil, - ChangeSetOpts{}, ChangeFactory{}) - + changeSetWithVerRes := newChangeSet(existingRes, newRs) changes, err := changeSetWithVerRes.Calculate() require.NoError(t, err) @@ -75,8 +73,7 @@ metadata: name: secret `)) - changeSetWithVerRes := NewChangeSetWithVersionedRs([]ctlres.Resource{existingRes}, []ctlres.Resource{newRs}, nil, - ChangeSetOpts{}, ChangeFactory{}) + changeSetWithVerRes := newChangeSet(existingRes, newRs) changes, err := changeSetWithVerRes.Calculate() require.NoError(t, err) @@ -124,8 +121,7 @@ metadata: name: secret `)) - changeSetWithVerRes := NewChangeSetWithVersionedRs([]ctlres.Resource{existingRes}, []ctlres.Resource{newRs}, nil, - ChangeSetOpts{}, ChangeFactory{}) + changeSetWithVerRes := newChangeSet(existingRes, newRs) changes, err := changeSetWithVerRes.Calculate() require.NoError(t, err) @@ -183,8 +179,8 @@ data: foo: foo `)) - changeSetWithVerRes := NewChangeSetWithVersionedRs([]ctlres.Resource{existingRs}, []ctlres.Resource{newRs}, nil, ChangeSetOpts{}, ChangeFactory{}) - changeSetWithVerRes.stripNameHashSuffix = true + stripNameHashSuffix := true + changeSetWithVerRes := NewChangeSetWithVersionedRs([]ctlres.Resource{existingRs}, []ctlres.Resource{newRs}, nil, ChangeSetOpts{}, ChangeFactory{}, stripNameHashSuffix) changes, err := changeSetWithVerRes.Calculate() require.NoError(t, err) @@ -227,7 +223,7 @@ spec: replicas: 1 `)) - changeSetWithVerRes := NewChangeSetWithVersionedRs([]ctlres.Resource{existingRs}, []ctlres.Resource{newRs}, nil, ChangeSetOpts{}, ChangeFactory{}) + changeSetWithVerRes := newChangeSet(existingRs, newRs) changeSetWithVerRes.stripNameHashSuffix = true changes, err := changeSetWithVerRes.Calculate() @@ -249,6 +245,11 @@ spec: } +func newChangeSet(existingRes, newRes ctlres.Resource) *ChangeSetWithVersionedRs { + stripNameHashSuffix := false + return NewChangeSetWithVersionedRs([]ctlres.Resource{existingRes}, []ctlres.Resource{newRes}, nil, ChangeSetOpts{}, ChangeFactory{}, stripNameHashSuffix) +} + func checkChangeDiff(t *testing.T, change Change, expectedDiff string) { actualDiffString := change.ConfigurableTextDiff().Full().FullString() From a396c465901de528b3101696a8ccd9c941d8af4b Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Sun, 29 May 2022 13:57:37 +0200 Subject: [PATCH 11/52] Add StripNameHashSuffix to config --- pkg/kapp/cmd/app/deploy.go | 2 +- pkg/kapp/config/conf.go | 8 ++++++++ pkg/kapp/config/config.go | 2 ++ pkg/kapp/config/default.go | 2 ++ pkg/kapp/diff/change_set_with_versioned_rs.go | 4 ++-- 5 files changed, 15 insertions(+), 3 deletions(-) diff --git a/pkg/kapp/cmd/app/deploy.go b/pkg/kapp/cmd/app/deploy.go index 9f9a60f77..d426fcede 100644 --- a/pkg/kapp/cmd/app/deploy.go +++ b/pkg/kapp/cmd/app/deploy.go @@ -402,7 +402,7 @@ func (o *DeployOptions) calculateAndPresentChanges(existingResources, changes, err := ctldiff.NewChangeSetWithVersionedRs( existingResources, newResources, conf.TemplateRules(), - o.DiffFlags.ChangeSetOpts, changeFactory).Calculate() + o.DiffFlags.ChangeSetOpts, changeFactory, conf.StripNameHashSuffix()).Calculate() if err != nil { return clusterChangeSet, nil, false, "", err } diff --git a/pkg/kapp/config/conf.go b/pkg/kapp/config/conf.go index 50a1be572..2754d9c7f 100644 --- a/pkg/kapp/config/conf.go +++ b/pkg/kapp/config/conf.go @@ -175,3 +175,11 @@ func (c Conf) ChangeRuleBindings() []ChangeRuleBinding { } return result } + +func (c Conf) StripNameHashSuffix() bool { + result := false + for _, config := range c.configs { + result = result || config.StripNameHashSuffix + } + return result +} diff --git a/pkg/kapp/config/config.go b/pkg/kapp/config/config.go index 72807c60a..2958d4793 100644 --- a/pkg/kapp/config/config.go +++ b/pkg/kapp/config/config.go @@ -38,6 +38,8 @@ type Config struct { // TODO validations ChangeGroupBindings []ChangeGroupBinding ChangeRuleBindings []ChangeRuleBinding + + StripNameHashSuffix bool } type WaitRule struct { diff --git a/pkg/kapp/config/default.go b/pkg/kapp/config/default.go index ddbd4f01d..78b4b5a05 100644 --- a/pkg/kapp/config/default.go +++ b/pkg/kapp/config/default.go @@ -613,6 +613,8 @@ changeRuleBindings: - anyMatcher: {matchers: *rbacMatchers} - anyMatcher: {matchers: *podRelatedMatchers} - hasNamespaceMatcher: {} + +StripNameHashSuffix: false ` var defaultConfigRes = ctlres.MustNewResourceFromBytes([]byte(defaultConfigYAML)) diff --git a/pkg/kapp/diff/change_set_with_versioned_rs.go b/pkg/kapp/diff/change_set_with_versioned_rs.go index c7d7c5d72..ad088006e 100644 --- a/pkg/kapp/diff/change_set_with_versioned_rs.go +++ b/pkg/kapp/diff/change_set_with_versioned_rs.go @@ -27,9 +27,9 @@ type ChangeSetWithVersionedRs struct { } func NewChangeSetWithVersionedRs(existingRs, newRs []ctlres.Resource, - rules []ctlconf.TemplateRule, opts ChangeSetOpts, changeFactory ChangeFactory) *ChangeSetWithVersionedRs { + rules []ctlconf.TemplateRule, opts ChangeSetOpts, changeFactory ChangeFactory, stripNameHashSuffix bool) *ChangeSetWithVersionedRs { - return &ChangeSetWithVersionedRs{existingRs, newRs, rules, opts, changeFactory, false /* TODO */} + return &ChangeSetWithVersionedRs{existingRs, newRs, rules, opts, changeFactory, stripNameHashSuffix} } func (d ChangeSetWithVersionedRs) Calculate() ([]Change, error) { From 795bee49a83ba06ec38a3989a3eda4fc78a37460 Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Sat, 11 Jun 2022 18:51:47 +0200 Subject: [PATCH 12/52] podman histfile --- hack/.gitignore | 1 + hack/podman.sh | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-) create mode 100644 hack/.gitignore diff --git a/hack/.gitignore b/hack/.gitignore new file mode 100644 index 000000000..c40b48396 --- /dev/null +++ b/hack/.gitignore @@ -0,0 +1 @@ +.bash_history diff --git a/hack/podman.sh b/hack/podman.sh index ed803a12d..0f79ae237 100755 --- a/hack/podman.sh +++ b/hack/podman.sh @@ -7,7 +7,10 @@ action=$1 if [ "$action" ]; then [ -x "$action.sh" ] && action=hack/$action.sh else - interactive="-it" + interactive="-it -v $PWD/$histfile:/root/$histfile" + + histfile=.bash_history + [ -f $histfile ] || >$histfile fi # SYS_PTRACE for dlv From a728a67380b530e2050fbbe39cbd75a0a4117c7c Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Sat, 11 Jun 2022 19:19:30 +0200 Subject: [PATCH 13/52] adjust test to match (new) expectations --- .../diff/change_set_with_versioned_rs_test.go | 23 ++++++++----------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/pkg/kapp/diff/change_set_with_versioned_rs_test.go b/pkg/kapp/diff/change_set_with_versioned_rs_test.go index 467d9492a..4ff6f0088 100644 --- a/pkg/kapp/diff/change_set_with_versioned_rs_test.go +++ b/pkg/kapp/diff/change_set_with_versioned_rs_test.go @@ -157,26 +157,23 @@ metadata: } func TestChangeSet_StripKustomizeSuffix(t *testing.T) { - newRs := ctlres.MustNewResourceFromBytes([]byte(` + + existingRs := ctlres.MustNewResourceFromBytes([]byte(` apiVersion: v1 kind: ConfigMap metadata: - annotations: - kapp.k14s.io/versioned: "" name: configmap-abc data: - foo: bar + foo: foo `)) - existingRs := ctlres.MustNewResourceFromBytes([]byte(` + newRs := ctlres.MustNewResourceFromBytes([]byte(` apiVersion: v1 kind: ConfigMap metadata: - annotations: - kapp.k14s.io/versioned: "" - name: configmap-ver-1 + name: configmap-cdf data: - foo: foo + foo: bar `)) stripNameHashSuffix := true @@ -193,11 +190,9 @@ data: 3, 2 + foo: bar 3, 3 kind: ConfigMap 4, 4 metadata: - 5, 5 annotations: - 6, 6 kapp.k14s.io/versioned: "" - 7, 7 - name: configmap-ver-1 - 8, 7 + name: configmap-ver-2 - 8, 8 + 5, 5 - name: configmap-abc + 6, 5 + name: configmap-cdf + 6, 6 ` checkChangeDiff(t, changes[0], expectedDiff) From 8d94e34c0524513e9f43ae7b092781c8d25fbf7e Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Sat, 11 Jun 2022 20:40:25 +0200 Subject: [PATCH 14/52] Move version checks and version updates --- pkg/kapp/diff/change_set_with_versioned_rs.go | 32 +++++++----------- pkg/kapp/diff/versioned_resource.go | 33 +++++++++++++++++++ 2 files changed, 45 insertions(+), 20 deletions(-) diff --git a/pkg/kapp/diff/change_set_with_versioned_rs.go b/pkg/kapp/diff/change_set_with_versioned_rs.go index ad088006e..d32595b8c 100644 --- a/pkg/kapp/diff/change_set_with_versioned_rs.go +++ b/pkg/kapp/diff/change_set_with_versioned_rs.go @@ -33,10 +33,10 @@ func NewChangeSetWithVersionedRs(existingRs, newRs []ctlres.Resource, } func (d ChangeSetWithVersionedRs) Calculate() ([]Change, error) { - existingRs := existingVersionedResources(d.existingRs) + existingRs := d.existingVersionedResources(d.existingRs) existingRsGrouped := d.groupResources(existingRs.Versioned) - newRs := newVersionedResources(d.newRs) + newRs := d.newVersionedResources(d.newRs) allChanges := []Change{} d.assignNewNames(newRs, existingRsGrouped) @@ -85,8 +85,9 @@ func (d ChangeSetWithVersionedRs) groupResources(rs []ctlres.Resource) map[strin result := map[string][]ctlres.Resource{} groupByFunc := func(res ctlres.Resource) string { - if _, found := res.Annotations()[versionedResAnnKey]; found { - return d.versionedResourceName(res).UniqVersionedKey().String() + versionedRes := d.versionedResourceName(res) + if versionedRes.IsVersioned() { + return versionedRes.UniqVersionedKey().String() } panic("Expected to find versioned annotation on resource") } @@ -110,10 +111,10 @@ func (d ChangeSetWithVersionedRs) assignNewNames( newResKey := newVerRes.UniqVersionedKey().String() if existingRs, found := existingRsGrouped[newResKey]; found { - existingRes := existingRs[len(existingRs)-1] - newVerRes.SetBaseName(d.versionedResourceName(existingRes).Version() + 1) + existingRes := d.versionedResourceName(existingRs[len(existingRs)-1]) + newVerRes.AssignNextVersion(existingRes) } else { - newVerRes.SetBaseName(1) + newVerRes.AssignNewVersion() } } } @@ -263,13 +264,12 @@ type versionedResources struct { NonVersioned []ctlres.Resource } -func newVersionedResources(rs []ctlres.Resource) versionedResources { +func (d ChangeSetWithVersionedRs) newVersionedResources(rs []ctlres.Resource) versionedResources { var result versionedResources for _, res := range rs { - _, hasVersionedAnn := res.Annotations()[versionedResAnnKey] _, hasVersionedOrigAnn := res.Annotations()[versionedResOrigAnnKey] - if hasVersionedAnn { + if d.versionedResourceName(res).IsVersioned() { result.Versioned = append(result.Versioned, res) if hasVersionedOrigAnn { result.NonVersioned = append(result.NonVersioned, res.DeepCopy()) @@ -281,18 +281,10 @@ func newVersionedResources(rs []ctlres.Resource) versionedResources { return result } -func existingVersionedResources(rs []ctlres.Resource) versionedResources { +func (d ChangeSetWithVersionedRs) existingVersionedResources(rs []ctlres.Resource) versionedResources { var result versionedResources for _, res := range rs { - // Expect that versioned resources should not be transient - // (Annotations may have been copied from versioned resources - // onto transient resources for non-versioning related purposes). - _, hasVersionedAnn := res.Annotations()[versionedResAnnKey] - - versionedRs := VersionedResource{res: res} - _, version := versionedRs.BaseNameAndVersion() - - if hasVersionedAnn && !res.Transient() && version != "" { + if d.versionedResourceName(res).IsExistingVersioned() { result.Versioned = append(result.Versioned, res) } else { result.NonVersioned = append(result.NonVersioned, res) diff --git a/pkg/kapp/diff/versioned_resource.go b/pkg/kapp/diff/versioned_resource.go index b614e8772..e22a3c6da 100644 --- a/pkg/kapp/diff/versioned_resource.go +++ b/pkg/kapp/diff/versioned_resource.go @@ -68,6 +68,39 @@ func (d VersionedResource) UniqVersionedKey() ctlres.UniqueResourceKey { return ctlres.NewUniqueResourceKeyWithCustomName(d.res, baseName) } +func (d VersionedResource) TrackVersions() bool { + _, hasVersionedAnn := d.res.Annotations()[versionedResAnnKey] + return hasVersionedAnn +} + +func (d VersionedResource) IsVersioned() bool { + return d.TrackVersions() +} + +func (d VersionedResource) IsExistingVersioned() bool { + + // Expect that versioned resources should not be transient + // (Annotations may have been copied from versioned resources + // onto transient resources for non-versioning related purposes). + notTransient := !d.res.Transient() + + + // TODO check moved during refactoring, but not sure why it is required + _, version := d.BaseNameAndVersion() + hasVersion := version != "" + + return d.IsVersioned() && notTransient && hasVersion + +} + +func (d VersionedResource) AssignNextVersion(old VersionedResource) { + d.SetBaseName(old.Version() + 1) +} + +func (d VersionedResource) AssignNewVersion() { + d.SetBaseName(1) +} + func (d VersionedResource) UpdateAffected(rs []ctlres.Resource) error { rules, err := d.matchingRules() if err != nil { From 44c51d9e35ac9900b8b7d91daf2a785f8240753b Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Sat, 11 Jun 2022 20:45:42 +0200 Subject: [PATCH 15/52] Implement ignoring name-hash-suffix in diff --- pkg/kapp/diff/versioned_resource.go | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/pkg/kapp/diff/versioned_resource.go b/pkg/kapp/diff/versioned_resource.go index e22a3c6da..ea416ba7a 100644 --- a/pkg/kapp/diff/versioned_resource.go +++ b/pkg/kapp/diff/versioned_resource.go @@ -29,9 +29,11 @@ func (d VersionedResource) StripNameHashSuffix() bool { } func (d VersionedResource) SetBaseName(ver int) { - currentName, _ := d.BaseNameAndVersion() - name := fmt.Sprintf("%s%s%d", currentName, nameSuffixSep, ver) - d.res.SetName(name) + if d.TrackVersions() { + currentName, _ := d.BaseNameAndVersion() + name := fmt.Sprintf("%s%s%d", currentName, nameSuffixSep, ver) + d.res.SetName(name) + } } func (d VersionedResource) BaseNameAndVersion() (string, string) { @@ -52,7 +54,11 @@ func (d VersionedResource) BaseNameAndVersion() (string, string) { func (d VersionedResource) Version() int { _, ver := d.BaseNameAndVersion() if len(ver) == 0 { - panic(fmt.Sprintf("Missing version in versioned resource '%s'", d.res.Description())) + if d.StripNameHashSuffix() { + return -1 + } else { + panic(fmt.Sprintf("Missing version in versioned resource '%s'", d.res.Description())) + } } verInt, err1 := strconv.Atoi(ver) @@ -74,6 +80,9 @@ func (d VersionedResource) TrackVersions() bool { } func (d VersionedResource) IsVersioned() bool { + if d.StripNameHashSuffix() { + return true + } return d.TrackVersions() } @@ -89,7 +98,9 @@ func (d VersionedResource) IsExistingVersioned() bool { _, version := d.BaseNameAndVersion() hasVersion := version != "" - return d.IsVersioned() && notTransient && hasVersion + versionUnnecessary := !d.TrackVersions() + + return d.IsVersioned() && notTransient && ( hasVersion || versionUnnecessary ) } @@ -102,6 +113,13 @@ func (d VersionedResource) AssignNewVersion() { } func (d VersionedResource) UpdateAffected(rs []ctlres.Resource) error { + + if !d.TrackVersions() { + // if we're not tracking versions we don't update any names and thus + // don't need to update any references. + return nil + } + rules, err := d.matchingRules() if err != nil { return err From 1f8d0bc5084be040bf93669ebe002209922428f7 Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Sat, 11 Jun 2022 20:46:24 +0200 Subject: [PATCH 16/52] Fix striping suffix from unrelated resources --- pkg/kapp/diff/versioned_resource.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/pkg/kapp/diff/versioned_resource.go b/pkg/kapp/diff/versioned_resource.go index ea416ba7a..31f75537b 100644 --- a/pkg/kapp/diff/versioned_resource.go +++ b/pkg/kapp/diff/versioned_resource.go @@ -25,7 +25,14 @@ type VersionedResource struct { } func (d VersionedResource) StripNameHashSuffix() bool { - return d.stripNameHashSuffix + + stripEnabled := d.stripNameHashSuffix + + // TODO should properly check using a ResourceMatcher + matchesRequiredKind := d.res.Kind() == "ConfigMap" || d.res.Kind() == "Secret" + + return stripEnabled && matchesRequiredKind + } func (d VersionedResource) SetBaseName(ver int) { From da8749a7d73ff5d6f9047cf3b1709cd4d899ed0f Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Sat, 11 Jun 2022 21:33:40 +0200 Subject: [PATCH 17/52] podman e2e --- hack/podman.sh | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/hack/podman.sh b/hack/podman.sh index 0f79ae237..4d7019027 100755 --- a/hack/podman.sh +++ b/hack/podman.sh @@ -14,4 +14,13 @@ else fi # SYS_PTRACE for dlv -podman run --rm -v gopath:/go -v gobuild:/root/.cache/go-build -v $(realpath $PWD/..):/mnt -w /mnt $interactive --cap-add SYS_PTRACE docker.io/golang $action +# host network for minikube +podman run --rm \ + -v gopath:/go \ + -v gobuild:/root/.cache/go-build \ + -v $HOME/.kube:/root/.kube \ + -v $HOME/.minikube:$HOME/.minikube \ + -v $(realpath $PWD/..):/mnt -w /mnt $interactive \ + --cap-add SYS_PTRACE \ + --network host \ + docker.io/golang $action From f3d8e74886cee1d9bbf06085be8968a4f3d6956c Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Sat, 11 Jun 2022 21:33:56 +0200 Subject: [PATCH 18/52] gofmt changes --- pkg/kapp/diff/versioned_resource.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/kapp/diff/versioned_resource.go b/pkg/kapp/diff/versioned_resource.go index 31f75537b..b77d981ea 100644 --- a/pkg/kapp/diff/versioned_resource.go +++ b/pkg/kapp/diff/versioned_resource.go @@ -100,14 +100,13 @@ func (d VersionedResource) IsExistingVersioned() bool { // onto transient resources for non-versioning related purposes). notTransient := !d.res.Transient() - // TODO check moved during refactoring, but not sure why it is required _, version := d.BaseNameAndVersion() hasVersion := version != "" versionUnnecessary := !d.TrackVersions() - return d.IsVersioned() && notTransient && ( hasVersion || versionUnnecessary ) + return d.IsVersioned() && notTransient && (hasVersion || versionUnnecessary) } From 481545220e8967e4d8c5c69cd19a0da389855dbb Mon Sep 17 00:00:00 2001 From: Christoph Schulz Date: Sun, 12 Jun 2022 06:33:13 +0000 Subject: [PATCH 19/52] golint changes --- pkg/kapp/diff/versioned_resource.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/kapp/diff/versioned_resource.go b/pkg/kapp/diff/versioned_resource.go index b77d981ea..f812de4d0 100644 --- a/pkg/kapp/diff/versioned_resource.go +++ b/pkg/kapp/diff/versioned_resource.go @@ -63,9 +63,8 @@ func (d VersionedResource) Version() int { if len(ver) == 0 { if d.StripNameHashSuffix() { return -1 - } else { - panic(fmt.Sprintf("Missing version in versioned resource '%s'", d.res.Description())) } + panic(fmt.Sprintf("Missing version in versioned resource '%s'", d.res.Description())) } verInt, err1 := strconv.Atoi(ver) From 25e6c49193c7908867ba640fd6ff647c59a18916 Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Mon, 27 Jun 2022 09:13:50 +0200 Subject: [PATCH 20/52] Add files for e2e kapp.yml has been generated by `kustomize build . -o kapp.yml` --- .../res/kustomize/components/kapp-config/kapp-cm.yml | 6 ++++++ .../components/kapp-config/kustomization.yaml | 5 +++++ test/e2e/res/kustomize/overlays/versioned1/kapp.yml | 12 ++++++++++++ .../kustomize/overlays/versioned1/kustomization.yaml | 10 ++++++++++ test/e2e/res/kustomize/overlays/versioned2/kapp.yml | 12 ++++++++++++ .../kustomize/overlays/versioned2/kustomization.yaml | 10 ++++++++++ 6 files changed, 55 insertions(+) create mode 100644 test/e2e/res/kustomize/components/kapp-config/kapp-cm.yml create mode 100644 test/e2e/res/kustomize/components/kapp-config/kustomization.yaml create mode 100644 test/e2e/res/kustomize/overlays/versioned1/kapp.yml create mode 100644 test/e2e/res/kustomize/overlays/versioned1/kustomization.yaml create mode 100644 test/e2e/res/kustomize/overlays/versioned2/kapp.yml create mode 100644 test/e2e/res/kustomize/overlays/versioned2/kustomization.yaml diff --git a/test/e2e/res/kustomize/components/kapp-config/kapp-cm.yml b/test/e2e/res/kustomize/components/kapp-config/kapp-cm.yml new file mode 100644 index 000000000..b77c25701 --- /dev/null +++ b/test/e2e/res/kustomize/components/kapp-config/kapp-cm.yml @@ -0,0 +1,6 @@ +apiVersion: kapp.k14s.io/v1alpha1 +kind: Config +metadata: + name: irrelevant + +StripNameHashSuffix: true diff --git a/test/e2e/res/kustomize/components/kapp-config/kustomization.yaml b/test/e2e/res/kustomize/components/kapp-config/kustomization.yaml new file mode 100644 index 000000000..559cef01f --- /dev/null +++ b/test/e2e/res/kustomize/components/kapp-config/kustomization.yaml @@ -0,0 +1,5 @@ +apiVersion: kustomize.config.k8s.io/v1alpha1 +kind: Component + +resources: + - kapp-cm.yml diff --git a/test/e2e/res/kustomize/overlays/versioned1/kapp.yml b/test/e2e/res/kustomize/overlays/versioned1/kapp.yml new file mode 100644 index 000000000..043cc33bb --- /dev/null +++ b/test/e2e/res/kustomize/overlays/versioned1/kapp.yml @@ -0,0 +1,12 @@ +apiVersion: v1 +data: + foo: foo +kind: ConfigMap +metadata: + name: config-map-7tgk8db28b +--- +StripNameHashSuffix: true +apiVersion: kapp.k14s.io/v1alpha1 +kind: Config +metadata: + name: irrelevant diff --git a/test/e2e/res/kustomize/overlays/versioned1/kustomization.yaml b/test/e2e/res/kustomize/overlays/versioned1/kustomization.yaml new file mode 100644 index 000000000..6a8afcd23 --- /dev/null +++ b/test/e2e/res/kustomize/overlays/versioned1/kustomization.yaml @@ -0,0 +1,10 @@ +apiVersion: kustomize.config.k8s.io/v1beta1 +kind: Kustomization + +components: + - ../../components/kapp-config + +configMapGenerator: + - name: config-map + literals: + - foo=foo diff --git a/test/e2e/res/kustomize/overlays/versioned2/kapp.yml b/test/e2e/res/kustomize/overlays/versioned2/kapp.yml new file mode 100644 index 000000000..c604e23d8 --- /dev/null +++ b/test/e2e/res/kustomize/overlays/versioned2/kapp.yml @@ -0,0 +1,12 @@ +apiVersion: v1 +data: + foo: bar +kind: ConfigMap +metadata: + name: config-map-798k5k7g9f +--- +StripNameHashSuffix: true +apiVersion: kapp.k14s.io/v1alpha1 +kind: Config +metadata: + name: irrelevant diff --git a/test/e2e/res/kustomize/overlays/versioned2/kustomization.yaml b/test/e2e/res/kustomize/overlays/versioned2/kustomization.yaml new file mode 100644 index 000000000..aa5a0bfa8 --- /dev/null +++ b/test/e2e/res/kustomize/overlays/versioned2/kustomization.yaml @@ -0,0 +1,10 @@ +apiVersion: kustomize.config.k8s.io/v1beta1 +kind: Kustomization + +components: + - ../../components/kapp-config + +configMapGenerator: + - name: config-map + literals: + - foo=bar From 8730f1375631f39179ac31d7e013d06dec721707 Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Sun, 10 Jul 2022 14:10:27 +0200 Subject: [PATCH 21/52] more e2e resources --- .../kustomize/overlays/versioned1/kapp.yml | 12 ++++++++++ .../overlays/versioned1/kustomization.yaml | 11 +++++++++ .../kustomize/overlays/versioned2/kapp.yml | 12 ++++++++++ .../overlays/versioned2/kustomization.yaml | 11 +++++++++ .../kustomize/overlays/versioned3/kapp.yml | 24 +++++++++++++++++++ .../overlays/versioned3/kustomization.yaml | 21 ++++++++++++++++ 6 files changed, 91 insertions(+) create mode 100644 test/e2e/res/kustomize/overlays/versioned3/kapp.yml create mode 100644 test/e2e/res/kustomize/overlays/versioned3/kustomization.yaml diff --git a/test/e2e/res/kustomize/overlays/versioned1/kapp.yml b/test/e2e/res/kustomize/overlays/versioned1/kapp.yml index 043cc33bb..377257cd6 100644 --- a/test/e2e/res/kustomize/overlays/versioned1/kapp.yml +++ b/test/e2e/res/kustomize/overlays/versioned1/kapp.yml @@ -3,8 +3,20 @@ data: foo: foo kind: ConfigMap metadata: + annotations: + kapp.k14s.io/num-versions: "1" name: config-map-7tgk8db28b --- +apiVersion: v1 +data: + trulla: troet +kind: ConfigMap +metadata: + annotations: + kapp.k14s.io/num-versions: "1" + kapp.k14s.io/versioned: "" + name: versioned-cm +--- StripNameHashSuffix: true apiVersion: kapp.k14s.io/v1alpha1 kind: Config diff --git a/test/e2e/res/kustomize/overlays/versioned1/kustomization.yaml b/test/e2e/res/kustomize/overlays/versioned1/kustomization.yaml index 6a8afcd23..45c8fbaed 100644 --- a/test/e2e/res/kustomize/overlays/versioned1/kustomization.yaml +++ b/test/e2e/res/kustomize/overlays/versioned1/kustomization.yaml @@ -8,3 +8,14 @@ configMapGenerator: - name: config-map literals: - foo=foo + options: + annotations: + kapp.k14s.io/num-versions: "1" + - name: versioned-cm + literals: + - trulla=troet + options: + disableNameSuffixHash: true + annotations: + kapp.k14s.io/versioned: "" + kapp.k14s.io/num-versions: "1" diff --git a/test/e2e/res/kustomize/overlays/versioned2/kapp.yml b/test/e2e/res/kustomize/overlays/versioned2/kapp.yml index c604e23d8..dfba6440d 100644 --- a/test/e2e/res/kustomize/overlays/versioned2/kapp.yml +++ b/test/e2e/res/kustomize/overlays/versioned2/kapp.yml @@ -3,8 +3,20 @@ data: foo: bar kind: ConfigMap metadata: + annotations: + kapp.k14s.io/num-versions: "1" name: config-map-798k5k7g9f --- +apiVersion: v1 +data: + trulla: erna +kind: ConfigMap +metadata: + annotations: + kapp.k14s.io/num-versions: "1" + kapp.k14s.io/versioned: "" + name: versioned-cm +--- StripNameHashSuffix: true apiVersion: kapp.k14s.io/v1alpha1 kind: Config diff --git a/test/e2e/res/kustomize/overlays/versioned2/kustomization.yaml b/test/e2e/res/kustomize/overlays/versioned2/kustomization.yaml index aa5a0bfa8..e26322088 100644 --- a/test/e2e/res/kustomize/overlays/versioned2/kustomization.yaml +++ b/test/e2e/res/kustomize/overlays/versioned2/kustomization.yaml @@ -8,3 +8,14 @@ configMapGenerator: - name: config-map literals: - foo=bar + options: + annotations: + kapp.k14s.io/num-versions: "1" + - name: versioned-cm + literals: + - trulla=erna + options: + disableNameSuffixHash: true + annotations: + kapp.k14s.io/versioned: "" + kapp.k14s.io/num-versions: "1" diff --git a/test/e2e/res/kustomize/overlays/versioned3/kapp.yml b/test/e2e/res/kustomize/overlays/versioned3/kapp.yml new file mode 100644 index 000000000..aecf9ca37 --- /dev/null +++ b/test/e2e/res/kustomize/overlays/versioned3/kapp.yml @@ -0,0 +1,24 @@ +apiVersion: v1 +data: + foo: bal +kind: ConfigMap +metadata: + annotations: + kapp.k14s.io/num-versions: "1" + name: config-map-5bghbfbdc8 +--- +apiVersion: v1 +data: + trulla: schnuerschuh +kind: ConfigMap +metadata: + annotations: + kapp.k14s.io/num-versions: "1" + kapp.k14s.io/versioned: "" + name: versioned-cm +--- +StripNameHashSuffix: true +apiVersion: kapp.k14s.io/v1alpha1 +kind: Config +metadata: + name: irrelevant diff --git a/test/e2e/res/kustomize/overlays/versioned3/kustomization.yaml b/test/e2e/res/kustomize/overlays/versioned3/kustomization.yaml new file mode 100644 index 000000000..7ae1dede5 --- /dev/null +++ b/test/e2e/res/kustomize/overlays/versioned3/kustomization.yaml @@ -0,0 +1,21 @@ +apiVersion: kustomize.config.k8s.io/v1beta1 +kind: Kustomization + +components: + - ../../components/kapp-config + +configMapGenerator: + - name: config-map + literals: + - foo=bal + options: + annotations: + kapp.k14s.io/num-versions: "1" + - name: versioned-cm + literals: + - trulla=schnuerschuh + options: + disableNameSuffixHash: true + annotations: + kapp.k14s.io/versioned: "" + kapp.k14s.io/num-versions: "1" From faf26d7aacf7683d94906cf8972b3b1250911e24 Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Sun, 17 Jul 2022 12:12:28 +0200 Subject: [PATCH 22/52] Delete old name-hash-suffixed resources --- pkg/kapp/diff/change_set_with_versioned_rs.go | 4 +++- pkg/kapp/diff/versioned_resource.go | 4 ++++ test/e2e/res/kustomize/overlays/versioned1/kapp.yml | 12 ------------ .../kustomize/overlays/versioned1/kustomization.yaml | 11 ----------- test/e2e/res/kustomize/overlays/versioned2/kapp.yml | 12 ------------ .../kustomize/overlays/versioned2/kustomization.yaml | 11 ----------- test/e2e/res/kustomize/overlays/versioned3/kapp.yml | 12 ------------ .../kustomize/overlays/versioned3/kustomization.yaml | 11 ----------- 8 files changed, 7 insertions(+), 70 deletions(-) diff --git a/pkg/kapp/diff/change_set_with_versioned_rs.go b/pkg/kapp/diff/change_set_with_versioned_rs.go index d32595b8c..bf887de34 100644 --- a/pkg/kapp/diff/change_set_with_versioned_rs.go +++ b/pkg/kapp/diff/change_set_with_versioned_rs.go @@ -231,7 +231,7 @@ func (d ChangeSetWithVersionedRs) newNoopChange(existingRes ctlres.Resource) Cha return NewChangePrecalculated(existingRes, nil, nil, ChangeOpNoop, nil, OpsDiff{}) } -func (ChangeSetWithVersionedRs) numOfResourcesToKeep(res ctlres.Resource) (int, error) { +func (d ChangeSetWithVersionedRs) numOfResourcesToKeep(res ctlres.Resource) (int, error) { // TODO get rid of arbitrary cut off numToKeep := 5 @@ -244,6 +244,8 @@ func (ChangeSetWithVersionedRs) numOfResourcesToKeep(res ctlres.Resource) (int, if numToKeep < 1 { return 0, fmt.Errorf("Expected annotation '%s' value to be a >= 1", versionedResNumVersAnnKey) } + } else if !d.versionedResourceName(res).IsTracked() { + numToKeep = 0 } else { numToKeep = 5 } diff --git a/pkg/kapp/diff/versioned_resource.go b/pkg/kapp/diff/versioned_resource.go index f812de4d0..f6320696b 100644 --- a/pkg/kapp/diff/versioned_resource.go +++ b/pkg/kapp/diff/versioned_resource.go @@ -92,6 +92,10 @@ func (d VersionedResource) IsVersioned() bool { return d.TrackVersions() } +func (d VersionedResource) IsTracked() bool { + return d.IsVersioned() && d.TrackVersions() +} + func (d VersionedResource) IsExistingVersioned() bool { // Expect that versioned resources should not be transient diff --git a/test/e2e/res/kustomize/overlays/versioned1/kapp.yml b/test/e2e/res/kustomize/overlays/versioned1/kapp.yml index 377257cd6..043cc33bb 100644 --- a/test/e2e/res/kustomize/overlays/versioned1/kapp.yml +++ b/test/e2e/res/kustomize/overlays/versioned1/kapp.yml @@ -3,20 +3,8 @@ data: foo: foo kind: ConfigMap metadata: - annotations: - kapp.k14s.io/num-versions: "1" name: config-map-7tgk8db28b --- -apiVersion: v1 -data: - trulla: troet -kind: ConfigMap -metadata: - annotations: - kapp.k14s.io/num-versions: "1" - kapp.k14s.io/versioned: "" - name: versioned-cm ---- StripNameHashSuffix: true apiVersion: kapp.k14s.io/v1alpha1 kind: Config diff --git a/test/e2e/res/kustomize/overlays/versioned1/kustomization.yaml b/test/e2e/res/kustomize/overlays/versioned1/kustomization.yaml index 45c8fbaed..6a8afcd23 100644 --- a/test/e2e/res/kustomize/overlays/versioned1/kustomization.yaml +++ b/test/e2e/res/kustomize/overlays/versioned1/kustomization.yaml @@ -8,14 +8,3 @@ configMapGenerator: - name: config-map literals: - foo=foo - options: - annotations: - kapp.k14s.io/num-versions: "1" - - name: versioned-cm - literals: - - trulla=troet - options: - disableNameSuffixHash: true - annotations: - kapp.k14s.io/versioned: "" - kapp.k14s.io/num-versions: "1" diff --git a/test/e2e/res/kustomize/overlays/versioned2/kapp.yml b/test/e2e/res/kustomize/overlays/versioned2/kapp.yml index dfba6440d..c604e23d8 100644 --- a/test/e2e/res/kustomize/overlays/versioned2/kapp.yml +++ b/test/e2e/res/kustomize/overlays/versioned2/kapp.yml @@ -3,20 +3,8 @@ data: foo: bar kind: ConfigMap metadata: - annotations: - kapp.k14s.io/num-versions: "1" name: config-map-798k5k7g9f --- -apiVersion: v1 -data: - trulla: erna -kind: ConfigMap -metadata: - annotations: - kapp.k14s.io/num-versions: "1" - kapp.k14s.io/versioned: "" - name: versioned-cm ---- StripNameHashSuffix: true apiVersion: kapp.k14s.io/v1alpha1 kind: Config diff --git a/test/e2e/res/kustomize/overlays/versioned2/kustomization.yaml b/test/e2e/res/kustomize/overlays/versioned2/kustomization.yaml index e26322088..aa5a0bfa8 100644 --- a/test/e2e/res/kustomize/overlays/versioned2/kustomization.yaml +++ b/test/e2e/res/kustomize/overlays/versioned2/kustomization.yaml @@ -8,14 +8,3 @@ configMapGenerator: - name: config-map literals: - foo=bar - options: - annotations: - kapp.k14s.io/num-versions: "1" - - name: versioned-cm - literals: - - trulla=erna - options: - disableNameSuffixHash: true - annotations: - kapp.k14s.io/versioned: "" - kapp.k14s.io/num-versions: "1" diff --git a/test/e2e/res/kustomize/overlays/versioned3/kapp.yml b/test/e2e/res/kustomize/overlays/versioned3/kapp.yml index aecf9ca37..66a5eb2d2 100644 --- a/test/e2e/res/kustomize/overlays/versioned3/kapp.yml +++ b/test/e2e/res/kustomize/overlays/versioned3/kapp.yml @@ -3,20 +3,8 @@ data: foo: bal kind: ConfigMap metadata: - annotations: - kapp.k14s.io/num-versions: "1" name: config-map-5bghbfbdc8 --- -apiVersion: v1 -data: - trulla: schnuerschuh -kind: ConfigMap -metadata: - annotations: - kapp.k14s.io/num-versions: "1" - kapp.k14s.io/versioned: "" - name: versioned-cm ---- StripNameHashSuffix: true apiVersion: kapp.k14s.io/v1alpha1 kind: Config diff --git a/test/e2e/res/kustomize/overlays/versioned3/kustomization.yaml b/test/e2e/res/kustomize/overlays/versioned3/kustomization.yaml index 7ae1dede5..6fd6655e1 100644 --- a/test/e2e/res/kustomize/overlays/versioned3/kustomization.yaml +++ b/test/e2e/res/kustomize/overlays/versioned3/kustomization.yaml @@ -8,14 +8,3 @@ configMapGenerator: - name: config-map literals: - foo=bal - options: - annotations: - kapp.k14s.io/num-versions: "1" - - name: versioned-cm - literals: - - trulla=schnuerschuh - options: - disableNameSuffixHash: true - annotations: - kapp.k14s.io/versioned: "" - kapp.k14s.io/num-versions: "1" From cd782ecb5b697cd3515740d5636b5e9ce84d1cb1 Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Sun, 17 Jul 2022 14:10:46 +0200 Subject: [PATCH 23/52] Mark method private --- pkg/kapp/diff/versioned_resource.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/kapp/diff/versioned_resource.go b/pkg/kapp/diff/versioned_resource.go index f6320696b..3643841f5 100644 --- a/pkg/kapp/diff/versioned_resource.go +++ b/pkg/kapp/diff/versioned_resource.go @@ -36,7 +36,7 @@ func (d VersionedResource) StripNameHashSuffix() bool { } func (d VersionedResource) SetBaseName(ver int) { - if d.TrackVersions() { + if d.trackVersions() { currentName, _ := d.BaseNameAndVersion() name := fmt.Sprintf("%s%s%d", currentName, nameSuffixSep, ver) d.res.SetName(name) @@ -80,7 +80,7 @@ func (d VersionedResource) UniqVersionedKey() ctlres.UniqueResourceKey { return ctlres.NewUniqueResourceKeyWithCustomName(d.res, baseName) } -func (d VersionedResource) TrackVersions() bool { +func (d VersionedResource) trackVersions() bool { _, hasVersionedAnn := d.res.Annotations()[versionedResAnnKey] return hasVersionedAnn } @@ -89,11 +89,11 @@ func (d VersionedResource) IsVersioned() bool { if d.StripNameHashSuffix() { return true } - return d.TrackVersions() + return d.trackVersions() } func (d VersionedResource) IsTracked() bool { - return d.IsVersioned() && d.TrackVersions() + return d.IsVersioned() && d.trackVersions() } func (d VersionedResource) IsExistingVersioned() bool { @@ -107,7 +107,7 @@ func (d VersionedResource) IsExistingVersioned() bool { _, version := d.BaseNameAndVersion() hasVersion := version != "" - versionUnnecessary := !d.TrackVersions() + versionUnnecessary := !d.trackVersions() return d.IsVersioned() && notTransient && (hasVersion || versionUnnecessary) @@ -123,7 +123,7 @@ func (d VersionedResource) AssignNewVersion() { func (d VersionedResource) UpdateAffected(rs []ctlres.Resource) error { - if !d.TrackVersions() { + if !d.trackVersions() { // if we're not tracking versions we don't update any names and thus // don't need to update any references. return nil From 9391cef4f4f1f399b9c436b2e295f2bfd5b03d7e Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Sun, 17 Jul 2022 14:17:49 +0200 Subject: [PATCH 24/52] Delete name-hash-suffix CMs based on change --- pkg/kapp/diff/change_set_with_versioned_rs.go | 39 ++++++++++++------- 1 file changed, 24 insertions(+), 15 deletions(-) diff --git a/pkg/kapp/diff/change_set_with_versioned_rs.go b/pkg/kapp/diff/change_set_with_versioned_rs.go index bf887de34..9a5580888 100644 --- a/pkg/kapp/diff/change_set_with_versioned_rs.go +++ b/pkg/kapp/diff/change_set_with_versioned_rs.go @@ -121,14 +121,15 @@ func (d ChangeSetWithVersionedRs) assignNewNames( func (d ChangeSetWithVersionedRs) addAndKeepChanges( newRs versionedResources, existingRsGrouped map[string][]ctlres.Resource) ( - []Change, map[string]ctlres.Resource, error) { + []Change, map[string]Change, error) { changes := []Change{} - alreadyAdded := map[string]ctlres.Resource{} + alreadyAdded := map[string]Change{} for _, newRes := range newRs.Versioned { newResKey := d.versionedResourceName(newRes).UniqVersionedKey().String() usedRes := newRes + var change Change if existingRs, found := existingRsGrouped[newResKey]; found { existingRes := existingRs[len(existingRs)-1] @@ -141,21 +142,23 @@ func (d ChangeSetWithVersionedRs) addAndKeepChanges( switch updateChange.Op() { case ChangeOpUpdate: - changes = append(changes, d.newAddChangeFromUpdateChange(newRes, updateChange)) + change = d.newAddChangeFromUpdateChange(newRes, updateChange) + changes = append(changes, change) case ChangeOpKeep: // Use latest copy of resource to update affected resources usedRes = existingRes - changes = append(changes, d.newKeepChange(existingRes)) + change = d.newKeepChange(existingRes) + changes = append(changes, change) default: panic(fmt.Sprintf("Unexpected change op %s", updateChange.Op())) } } else { // Since there no existing resource, create change for new resource - addChange, err := d.newChange(nil, newRes) + change, err := d.newChange(nil, newRes) if err != nil { return nil, nil, err } - changes = append(changes, addChange) + changes = append(changes, change) } // Update both versioned and non-versioned @@ -171,7 +174,7 @@ func (d ChangeSetWithVersionedRs) addAndKeepChanges( return nil, nil, err } - alreadyAdded[newResKey] = newRes + alreadyAdded[newResKey] = change } return changes, alreadyAdded, nil @@ -186,7 +189,7 @@ func (d ChangeSetWithVersionedRs) newAddChangeFromUpdateChange( func (d ChangeSetWithVersionedRs) noopAndDeleteChanges( existingRsGrouped map[string][]ctlres.Resource, - alreadyAdded map[string]ctlres.Resource) ([]Change, error) { + alreadyAdded map[string]Change) ([]Change, error) { changes := []Change{} @@ -194,9 +197,9 @@ func (d ChangeSetWithVersionedRs) noopAndDeleteChanges( for existingResKey, existingRs := range existingRsGrouped { numToKeep := 0 - if newRes, found := alreadyAdded[existingResKey]; found { + if change, found := alreadyAdded[existingResKey]; found { var err error - numToKeep, err = d.numOfResourcesToKeep(newRes) + numToKeep, err = d.numOfResourcesToKeep(change) if err != nil { return nil, err } @@ -231,9 +234,10 @@ func (d ChangeSetWithVersionedRs) newNoopChange(existingRes ctlres.Resource) Cha return NewChangePrecalculated(existingRes, nil, nil, ChangeOpNoop, nil, OpsDiff{}) } -func (d ChangeSetWithVersionedRs) numOfResourcesToKeep(res ctlres.Resource) (int, error) { - // TODO get rid of arbitrary cut off - numToKeep := 5 +func (d ChangeSetWithVersionedRs) numOfResourcesToKeep(change Change) (int, error) { + res := change.NewOrExistingResource() + versionedRes := d.versionedResourceName(res) + var numToKeep int if numToKeepAnn, found := res.Annotations()[versionedResNumVersAnnKey]; found { var err error @@ -244,9 +248,14 @@ func (d ChangeSetWithVersionedRs) numOfResourcesToKeep(res ctlres.Resource) (int if numToKeep < 1 { return 0, fmt.Errorf("Expected annotation '%s' value to be a >= 1", versionedResNumVersAnnKey) } - } else if !d.versionedResourceName(res).IsTracked() { - numToKeep = 0 + } else if versionedRes.IsVersioned() && !versionedRes.IsTracked() { + if change.Op() == ChangeOpKeep { + numToKeep = 1 + } else { + numToKeep = 0 + } } else { + // TODO get rid of arbitrary cut off numToKeep = 5 } From 66cc9eaf189db8cf6a43ae85ce21c1f58af5b1a9 Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Wed, 17 Aug 2022 00:24:11 +0200 Subject: [PATCH 25/52] Add E2E test for name-hash-suffix stripping --- test/e2e/res.go | 22 +++++ test/e2e/versioned_stripnamesuffix_test.go | 102 +++++++++++++++++++++ 2 files changed, 124 insertions(+) create mode 100644 test/e2e/res.go create mode 100644 test/e2e/versioned_stripnamesuffix_test.go diff --git a/test/e2e/res.go b/test/e2e/res.go new file mode 100644 index 000000000..705a1d6f3 --- /dev/null +++ b/test/e2e/res.go @@ -0,0 +1,22 @@ +package e2e + +import ( + "bytes" + "embed" + "io" +) + +//go:embed res +var testResFs embed.FS + +func TestResReader(path string) (io.Reader, error) { + b, err := TestResBytes(path) + if err != nil { + return nil, err + } + return bytes.NewReader(b), nil +} + +func TestResBytes(path string) ([]byte, error) { + return testResFs.ReadFile(path) +} diff --git a/test/e2e/versioned_stripnamesuffix_test.go b/test/e2e/versioned_stripnamesuffix_test.go new file mode 100644 index 000000000..026deac3c --- /dev/null +++ b/test/e2e/versioned_stripnamesuffix_test.go @@ -0,0 +1,102 @@ +// Copyright 2021 VMware, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package e2e + +import ( + "fmt" + "testing" + + uitest "github.com/cppforlife/go-cli-ui/ui/test" + "github.com/stretchr/testify/require" +) + +func setup(t *testing.T) (kapp Kapp, appName string, cleanUp func()) { + env := BuildEnv(t) + logger := Logger{} + kapp = Kapp{t, env.Namespace, env.KappBinaryPath, logger} + + appName = "test-versioned-stripnamesuffix" + cleanUp = func() { + kapp.Run([]string{"delete", "-a", appName}) + } + + cleanUp() + return +} + +func testResOverlayPath(name string) string { + return fmt.Sprintf("res/kustomize/overlays/%s/kapp.yml", name) +} + +func kappDeployOverlay(kapp Kapp, name string, app string) (string, error) { + testResReader, err := TestResReader(testResOverlayPath(name)) + if err != nil { + return "", fmt.Errorf("Could not load overlay for %s! %w", name, err) + } + return kapp.RunWithOpts([]string{"deploy", "-f", "-", "-a", app, "--json", "-c"}, RunOpts{IntoNs: true, StdinReader: testResReader}) +} + +func TestStripNameSuffixBasic(t *testing.T) { + kapp, appName, cleanup := setup(t) + defer cleanup() + + if _, err := kappDeployOverlay(kapp, "versioned1", appName); err != nil { + t.Errorf("Failed to deploy initial overlay!") + } + + stdout, err := kappDeployOverlay(kapp, "versioned2", appName) + + if err != nil { + t.Errorf("Failed to deploy next overlay!") + return + } + + //fmt.Println(stdout) + + expectedDiff := replaceAnnsLabels(` ... + 1, 1 data: + 2 - foo: foo + 2 + foo: bar + 3, 3 kind: ConfigMap + 4, 4 metadata: + ... + 7, 7 kapp.k14s.io/app: "1660686583367025336" + 8 - kapp.k14s.io/association: v1.d0fdf34aa1d77adddf880bb323b33066 + 8 + kapp.k14s.io/association: v1.4fda3fe945a039589026903cb477f5aa + 9, 9 managedFields: + 10, 10 - apiVersion: v1 +`) + expectedNote := "Op: 1 create, 1 delete, 0 update, 0 noop, 0 exists" + + resp := uitest.JSONUIFromBytes(t, []byte(stdout)) + + // Ensure the diff is shown + require.Exactlyf(t, expectedDiff, replaceAnnsLabels(resp.Blocks[0]), "Expected to see correct diff") + + // Ensure old ConfigMap is deleted + require.Exactlyf(t, expectedNote, replaceAnnsLabels(resp.Tables[0].Notes[0]), "Expected to see correct notes") +} + +func TestStripNameSuffixNoop(t *testing.T) { + kapp, appName, cleanup := setup(t) + defer cleanup() + + if _, err := kappDeployOverlay(kapp, "versioned1", appName); err != nil { + t.Errorf("Failed to deploy initial overlay!") + } + + stdout, err := kappDeployOverlay(kapp, "versioned1", appName) + + if err != nil { + t.Errorf("Failed to deploy next overlay!") + return + } + + expectedNote := "Op: 0 create, 0 delete, 0 update, 0 noop, 0 exists" + + resp := uitest.JSONUIFromBytes(t, []byte(stdout)) + + // Ensure current ConfigMap is not deleted + require.Exactlyf(t, expectedNote, replaceAnnsLabels(resp.Tables[0].Notes[0]), "Expected to see correct notes") +} From 9e6a6f1291ee7e1b3e1e71f0d61294e1c5d4ebce Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Wed, 17 Aug 2022 00:30:37 +0200 Subject: [PATCH 26/52] Kustomize resources README --- test/e2e/res/kustomize/README.md | 3 +++ test/e2e/res/kustomize/gen.sh | 5 +++++ 2 files changed, 8 insertions(+) create mode 100644 test/e2e/res/kustomize/README.md create mode 100755 test/e2e/res/kustomize/gen.sh diff --git a/test/e2e/res/kustomize/README.md b/test/e2e/res/kustomize/README.md new file mode 100644 index 000000000..986411347 --- /dev/null +++ b/test/e2e/res/kustomize/README.md @@ -0,0 +1,3 @@ +Test Resources for kustomize name-hash-suffix stripping. + +The files used by kapp are `overlay/*/kapp.yml`. (Re-)generate them using `./gen.sh`. diff --git a/test/e2e/res/kustomize/gen.sh b/test/e2e/res/kustomize/gen.sh new file mode 100755 index 000000000..69c7248fe --- /dev/null +++ b/test/e2e/res/kustomize/gen.sh @@ -0,0 +1,5 @@ +#!/usr/bin/env bash +for overlay in overlays/*/ +do + kustomize build $overlay -o $overlay/kapp.yml +done From 6b435dc9fe79355e5cf864a40d1eec56b85ab15d Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Mon, 29 Aug 2022 17:00:18 +0200 Subject: [PATCH 27/52] Fix persisting podman interactive history --- hack/.gitignore | 1 + hack/podman.sh | 12 +++++++++--- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/hack/.gitignore b/hack/.gitignore index c40b48396..92d88f5c3 100644 --- a/hack/.gitignore +++ b/hack/.gitignore @@ -1 +1,2 @@ .bash_history +.dlv diff --git a/hack/podman.sh b/hack/podman.sh index 4d7019027..5120de7ce 100755 --- a/hack/podman.sh +++ b/hack/podman.sh @@ -7,10 +7,15 @@ action=$1 if [ "$action" ]; then [ -x "$action.sh" ] && action=hack/$action.sh else - interactive="-it -v $PWD/$histfile:/root/$histfile" - histfile=.bash_history [ -f $histfile ] || >$histfile + + interactive="-it -v $PWD/$histfile:/root/$histfile" + + dlv_conf=.dlv + [ -d $dlv_conf ] || mkdir $dlv_conf + + interactive="$interactive -v $PWD/$dlv_conf:/root/.config/dlv" fi # SYS_PTRACE for dlv @@ -20,7 +25,8 @@ podman run --rm \ -v gobuild:/root/.cache/go-build \ -v $HOME/.kube:/root/.kube \ -v $HOME/.minikube:$HOME/.minikube \ - -v $(realpath $PWD/..):/mnt -w /mnt $interactive \ + -v $(realpath $PWD/..):/mnt -w /mnt \ + $interactive \ --cap-add SYS_PTRACE \ --network host \ docker.io/golang $action From 8e1c533a4ceb6fe9482d0823c5ad9ebfed7eb47e Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Mon, 29 Aug 2022 16:42:57 +0200 Subject: [PATCH 28/52] Add copyright header --- test/e2e/res.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/e2e/res.go b/test/e2e/res.go index 705a1d6f3..a26c5ca2b 100644 --- a/test/e2e/res.go +++ b/test/e2e/res.go @@ -1,3 +1,6 @@ +// Copyright 2022 VMware, Inc. +// SPDX-License-Identifier: Apache-2.0 + package e2e import ( From a8fd0ebf5a147c6299dfb765b185ec7336b9a6b4 Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Mon, 29 Aug 2022 16:44:49 +0200 Subject: [PATCH 29/52] Test only operation and diff count Checking diff contents does not reliably work due to managedFields being dependent on k8s version. but to test there is only one diff should also be enough. --- test/e2e/versioned_stripnamesuffix_test.go | 21 ++++----------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/test/e2e/versioned_stripnamesuffix_test.go b/test/e2e/versioned_stripnamesuffix_test.go index 026deac3c..d103d7cf1 100644 --- a/test/e2e/versioned_stripnamesuffix_test.go +++ b/test/e2e/versioned_stripnamesuffix_test.go @@ -54,28 +54,15 @@ func TestStripNameSuffixBasic(t *testing.T) { //fmt.Println(stdout) - expectedDiff := replaceAnnsLabels(` ... - 1, 1 data: - 2 - foo: foo - 2 + foo: bar - 3, 3 kind: ConfigMap - 4, 4 metadata: - ... - 7, 7 kapp.k14s.io/app: "1660686583367025336" - 8 - kapp.k14s.io/association: v1.d0fdf34aa1d77adddf880bb323b33066 - 8 + kapp.k14s.io/association: v1.4fda3fe945a039589026903cb477f5aa - 9, 9 managedFields: - 10, 10 - apiVersion: v1 -`) expectedNote := "Op: 1 create, 1 delete, 0 update, 0 noop, 0 exists" resp := uitest.JSONUIFromBytes(t, []byte(stdout)) - // Ensure the diff is shown - require.Exactlyf(t, expectedDiff, replaceAnnsLabels(resp.Blocks[0]), "Expected to see correct diff") + // Ensure one diff is shown + require.Exactlyf(t, 1, len(resp.Blocks), "Expected to see exactly one diff") // Ensure old ConfigMap is deleted - require.Exactlyf(t, expectedNote, replaceAnnsLabels(resp.Tables[0].Notes[0]), "Expected to see correct notes") + require.Exactlyf(t, expectedNote, replaceAnnsLabels(resp.Tables[0].Notes[0]), "Expected to one delete and one create Op") } func TestStripNameSuffixNoop(t *testing.T) { @@ -98,5 +85,5 @@ func TestStripNameSuffixNoop(t *testing.T) { resp := uitest.JSONUIFromBytes(t, []byte(stdout)) // Ensure current ConfigMap is not deleted - require.Exactlyf(t, expectedNote, replaceAnnsLabels(resp.Tables[0].Notes[0]), "Expected to see correct notes") + require.Exactlyf(t, expectedNote, replaceAnnsLabels(resp.Tables[0].Notes[0]), "Expected to see no Op's") } From 9807418550ed0827f69118fb131da5d7fe8973cb Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Mon, 29 Aug 2022 21:28:30 +0200 Subject: [PATCH 30/52] Make name-hash-suffix-strip resource check configurable --- pkg/kapp/cmd/app/deploy.go | 2 +- pkg/kapp/config/conf.go | 18 +++- pkg/kapp/config/config.go | 7 +- pkg/kapp/config/default.go | 6 +- pkg/kapp/diff/change_set_with_versioned_rs.go | 18 ++-- .../diff/change_set_with_versioned_rs_test.go | 49 +--------- pkg/kapp/diff/stripnamehashsuffix.go | 40 ++++++++ pkg/kapp/diff/stripnamehashsuffix_test.go | 92 +++++++++++++++++++ pkg/kapp/diff/versioned_resource.go | 15 +-- 9 files changed, 175 insertions(+), 72 deletions(-) create mode 100644 pkg/kapp/diff/stripnamehashsuffix.go create mode 100644 pkg/kapp/diff/stripnamehashsuffix_test.go diff --git a/pkg/kapp/cmd/app/deploy.go b/pkg/kapp/cmd/app/deploy.go index d426fcede..7a3bab81a 100644 --- a/pkg/kapp/cmd/app/deploy.go +++ b/pkg/kapp/cmd/app/deploy.go @@ -402,7 +402,7 @@ func (o *DeployOptions) calculateAndPresentChanges(existingResources, changes, err := ctldiff.NewChangeSetWithVersionedRs( existingResources, newResources, conf.TemplateRules(), - o.DiffFlags.ChangeSetOpts, changeFactory, conf.StripNameHashSuffix()).Calculate() + o.DiffFlags.ChangeSetOpts, changeFactory, conf.StripNameHashSuffixConfigs()).Calculate() if err != nil { return clusterChangeSet, nil, false, "", err } diff --git a/pkg/kapp/config/conf.go b/pkg/kapp/config/conf.go index 2754d9c7f..92256691f 100644 --- a/pkg/kapp/config/conf.go +++ b/pkg/kapp/config/conf.go @@ -176,10 +176,20 @@ func (c Conf) ChangeRuleBindings() []ChangeRuleBinding { return result } -func (c Conf) StripNameHashSuffix() bool { - result := false +type StripNameHashSuffixConfigs []StripNameHashSuffixConfig + +func (c StripNameHashSuffixConfigs) AggregateToCtlRes() (enabled bool, resourceMatchers [][]ctlres.ResourceMatcher) { + for _, conf := range c { + enabled = enabled || conf.Enabled + resourceMatchers = append(resourceMatchers, ResourceMatchers(conf.ResourceMatchers).AsResourceMatchers()) + } + return +} + +func (c Conf) StripNameHashSuffixConfigs() StripNameHashSuffixConfigs { + var configs []StripNameHashSuffixConfig for _, config := range c.configs { - result = result || config.StripNameHashSuffix + configs = append(configs, config.StripNameHashSuffixConfig) } - return result + return StripNameHashSuffixConfigs(configs) } diff --git a/pkg/kapp/config/config.go b/pkg/kapp/config/config.go index 2958d4793..028c174cf 100644 --- a/pkg/kapp/config/config.go +++ b/pkg/kapp/config/config.go @@ -39,7 +39,7 @@ type Config struct { ChangeGroupBindings []ChangeGroupBinding ChangeRuleBindings []ChangeRuleBinding - StripNameHashSuffix bool + StripNameHashSuffixConfig StripNameHashSuffixConfig } type WaitRule struct { @@ -140,6 +140,11 @@ type ChangeRuleBinding struct { ResourceMatchers []ResourceMatcher } +type StripNameHashSuffixConfig struct { + Enabled bool + ResourceMatchers []ResourceMatcher +} + func NewConfigFromResource(res ctlres.Resource) (Config, error) { if res.APIVersion() != configAPIVersion { return Config{}, fmt.Errorf( diff --git a/pkg/kapp/config/default.go b/pkg/kapp/config/default.go index 78b4b5a05..a99aeffb0 100644 --- a/pkg/kapp/config/default.go +++ b/pkg/kapp/config/default.go @@ -614,7 +614,11 @@ changeRuleBindings: - anyMatcher: {matchers: *podRelatedMatchers} - hasNamespaceMatcher: {} -StripNameHashSuffix: false +StripNameHashSuffix: + enabled: false + resourceMatchers: + - apiVersionKindMatcher: {kind: ConfigMap, apiVersion: v1} + - apiVersionKindMatcher: {kind: Secret, apiVersion: v1} ` var defaultConfigRes = ctlres.MustNewResourceFromBytes([]byte(defaultConfigYAML)) diff --git a/pkg/kapp/diff/change_set_with_versioned_rs.go b/pkg/kapp/diff/change_set_with_versioned_rs.go index 9a5580888..cc49d5fac 100644 --- a/pkg/kapp/diff/change_set_with_versioned_rs.go +++ b/pkg/kapp/diff/change_set_with_versioned_rs.go @@ -19,17 +19,17 @@ const ( ) type ChangeSetWithVersionedRs struct { - existingRs, newRs []ctlres.Resource - rules []ctlconf.TemplateRule - opts ChangeSetOpts - changeFactory ChangeFactory - stripNameHashSuffix bool + existingRs, newRs []ctlres.Resource + rules []ctlconf.TemplateRule + opts ChangeSetOpts + changeFactory ChangeFactory + stripNameHashSuffixConfig stripNameHashSuffixConfig } func NewChangeSetWithVersionedRs(existingRs, newRs []ctlres.Resource, - rules []ctlconf.TemplateRule, opts ChangeSetOpts, changeFactory ChangeFactory, stripNameHashSuffix bool) *ChangeSetWithVersionedRs { + rules []ctlconf.TemplateRule, opts ChangeSetOpts, changeFactory ChangeFactory, stripNameHashSuffixConfigs ctlconf.StripNameHashSuffixConfigs) *ChangeSetWithVersionedRs { - return &ChangeSetWithVersionedRs{existingRs, newRs, rules, opts, changeFactory, stripNameHashSuffix} + return &ChangeSetWithVersionedRs{existingRs, newRs, rules, opts, changeFactory, newStripNameHashSuffixConfigFromConf(stripNameHashSuffixConfigs)} } func (d ChangeSetWithVersionedRs) Calculate() ([]Change, error) { @@ -78,7 +78,7 @@ func (d ChangeSetWithVersionedRs) Calculate() ([]Change, error) { } func (d ChangeSetWithVersionedRs) versionedResourceName(res ctlres.Resource) VersionedResource { - return VersionedResource{res, nil, d.stripNameHashSuffix} + return VersionedResource{res, nil, d.stripNameHashSuffixConfig} } func (d ChangeSetWithVersionedRs) groupResources(rs []ctlres.Resource) map[string][]ctlres.Resource { @@ -162,7 +162,7 @@ func (d ChangeSetWithVersionedRs) addAndKeepChanges( } // Update both versioned and non-versioned - verRes := VersionedResource{usedRes, d.rules, d.stripNameHashSuffix} + verRes := VersionedResource{usedRes, d.rules, d.stripNameHashSuffixConfig} err := verRes.UpdateAffected(newRs.NonVersioned) if err != nil { diff --git a/pkg/kapp/diff/change_set_with_versioned_rs_test.go b/pkg/kapp/diff/change_set_with_versioned_rs_test.go index 4ff6f0088..c05708133 100644 --- a/pkg/kapp/diff/change_set_with_versioned_rs_test.go +++ b/pkg/kapp/diff/change_set_with_versioned_rs_test.go @@ -176,8 +176,8 @@ data: foo: bar `)) - stripNameHashSuffix := true - changeSetWithVerRes := NewChangeSetWithVersionedRs([]ctlres.Resource{existingRs}, []ctlres.Resource{newRs}, nil, ChangeSetOpts{}, ChangeFactory{}, stripNameHashSuffix) + stripNameHashSuffix := stripNameHashSuffixConfig{Enabled: true, ResourceMatcher: ctlres.AllMatcher{}} + changeSetWithVerRes := ChangeSetWithVersionedRs{[]ctlres.Resource{existingRs}, []ctlres.Resource{newRs}, nil, ChangeSetOpts{}, ChangeFactory{}, stripNameHashSuffix} changes, err := changeSetWithVerRes.Calculate() require.NoError(t, err) @@ -199,50 +199,9 @@ data: } -func TestChangeSet_StripKustomizeSuffix_CMonly(t *testing.T) { - newRs := ctlres.MustNewResourceFromBytes([]byte(` -apiVersion: apps/v1 -kind: Deployment -metadata: - name: my-deployment -spec: - replicas: 2 -`)) - - existingRs := ctlres.MustNewResourceFromBytes([]byte(` -apiVersion: apps/v1 -kind: Deployment -metadata: - name: my-deployment -spec: - replicas: 1 -`)) - - changeSetWithVerRes := newChangeSet(existingRs, newRs) - changeSetWithVerRes.stripNameHashSuffix = true - - changes, err := changeSetWithVerRes.Calculate() - require.NoError(t, err) - - require.Equal(t, ChangeOpUpdate, changes[0].Op(), "Expect to get updated") - - expectedDiff := ` 0, 0 apiVersion: apps/v1 - 1, 1 kind: Deployment - 2, 2 metadata: - 3, 3 name: my-deployment - 4, 4 spec: - 5, 5 - replicas: 1 - 6, 5 + replicas: 2 - 6, 6 -` - - checkChangeDiff(t, changes[0], expectedDiff) - -} - func newChangeSet(existingRes, newRes ctlres.Resource) *ChangeSetWithVersionedRs { - stripNameHashSuffix := false - return NewChangeSetWithVersionedRs([]ctlres.Resource{existingRes}, []ctlres.Resource{newRes}, nil, ChangeSetOpts{}, ChangeFactory{}, stripNameHashSuffix) + stripNameHashSuffix := stripNameHashSuffixConfig{Enabled: false} + return &ChangeSetWithVersionedRs{[]ctlres.Resource{existingRes}, []ctlres.Resource{newRes}, nil, ChangeSetOpts{}, ChangeFactory{}, stripNameHashSuffix} } func checkChangeDiff(t *testing.T, change Change, expectedDiff string) { diff --git a/pkg/kapp/diff/stripnamehashsuffix.go b/pkg/kapp/diff/stripnamehashsuffix.go new file mode 100644 index 000000000..d872823ea --- /dev/null +++ b/pkg/kapp/diff/stripnamehashsuffix.go @@ -0,0 +1,40 @@ +// Copyright 2020 VMware, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package diff + +import ( + ctlconf "github.com/k14s/kapp/pkg/kapp/config" + ctlres "github.com/k14s/kapp/pkg/kapp/resources" +) + +type stripNameHashSuffixConfig struct { + Enabled bool + ResourceMatcher ctlres.ResourceMatcher +} + +func (d stripNameHashSuffixConfig) EnabledFor(res ctlres.Resource) (stripEnabled bool){ + if d.Enabled { + return d.ResourceMatcher.Matches(res) + } + return false +} + +func newStripNameHashSuffixConfig(enabled bool, resourceMatchers [][]ctlres.ResourceMatcher) (result stripNameHashSuffixConfig) { + result = stripNameHashSuffixConfig{Enabled: enabled} + if enabled { + var allMatchers []ctlres.ResourceMatcher + for _, matchers := range resourceMatchers { + allMatchers = append(allMatchers, ctlres.AndMatcher{ + Matchers: matchers, + }) + } + result.ResourceMatcher = ctlres.AndMatcher{Matchers: allMatchers} + } + return result +} + +func newStripNameHashSuffixConfigFromConf(confs ctlconf.StripNameHashSuffixConfigs) stripNameHashSuffixConfig { + enabled, matchers := confs.AggregateToCtlRes() + return newStripNameHashSuffixConfig(enabled, matchers) +} diff --git a/pkg/kapp/diff/stripnamehashsuffix_test.go b/pkg/kapp/diff/stripnamehashsuffix_test.go new file mode 100644 index 000000000..739b11d67 --- /dev/null +++ b/pkg/kapp/diff/stripnamehashsuffix_test.go @@ -0,0 +1,92 @@ +// Copyright 2021 VMware, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package diff + +import ( + "testing" + + ctlres "github.com/k14s/kapp/pkg/kapp/resources" + "github.com/stretchr/testify/require" +) + +// test that we can exclude specific configsmaps from default +func TestStripNameHashSuffix_TestConfig_IncludeExcludeAccrossConfigs(t *testing.T) { + requireStripNameHashSuffixMatches(t, [][]ctlres.ResourceMatcher{ + []ctlres.ResourceMatcher{ + ctlres.AnyMatcher { + Matchers: []ctlres.ResourceMatcher { + ctlres.APIVersionKindMatcher{APIVersion: "v1", Kind: "ConfigMap"}, + ctlres.APIVersionKindMatcher{APIVersion: "v2", Kind: "MyKind"}, + }, + }, + }, + []ctlres.ResourceMatcher{ + ctlres.NotMatcher{ + Matcher: ctlres.KindNamespaceNameMatcher{Kind: "ConfigMap", Name: "foo"}, + }, + }, + }) +} + +func TestStripNameHashSuffix_TestConfig_IncludeExcludeSingleConfig(t *testing.T) { + requireStripNameHashSuffixMatches(t, [][]ctlres.ResourceMatcher{ + []ctlres.ResourceMatcher{ + ctlres.AnyMatcher { + Matchers: []ctlres.ResourceMatcher { + ctlres.APIVersionKindMatcher{APIVersion: "v1", Kind: "ConfigMap"}, + ctlres.APIVersionKindMatcher{APIVersion: "v2", Kind: "MyKind"}, + }, + }, + ctlres.NotMatcher{ + Matcher: ctlres.KindNamespaceNameMatcher{Kind: "ConfigMap", Name: "foo"}, + }, + }, + }) +} + +func requireStripNameHashSuffixMatches(t *testing.T, matchers [][]ctlres.ResourceMatcher) { + includedCM := ctlres.MustNewResourceFromBytes([]byte(` +apiVersion: v1 +kind: ConfigMap +metadata: + name: configmap-abc +data: + foo: foo +`)) + + includedKind := ctlres.MustNewResourceFromBytes([]byte(` +apiVersion: v2 +kind: MyKind +metadata: + name: my-res +spec: + key: val +`)) + + excludedCM := ctlres.MustNewResourceFromBytes([]byte(` +apiVersion: v1 +kind: ConfigMap +metadata: + name: foo +data: + foo: foo +`)) + + excludedKind := ctlres.MustNewResourceFromBytes([]byte(` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: bar +spec: + replicas: 1 +`)) + + config := newStripNameHashSuffixConfig(true, matchers) + + require.True(t, config.EnabledFor(includedCM), "expected included ConfigMap to match!") + require.True(t, config.EnabledFor(includedKind), "expected included Kind to match!") + require.False(t, config.EnabledFor(excludedCM), "expected excluded ConfigMap to not match!") + require.False(t, config.EnabledFor(excludedKind), "expected unmentioned Kind to not match!") + +} diff --git a/pkg/kapp/diff/versioned_resource.go b/pkg/kapp/diff/versioned_resource.go index 3643841f5..52d7b810b 100644 --- a/pkg/kapp/diff/versioned_resource.go +++ b/pkg/kapp/diff/versioned_resource.go @@ -19,20 +19,13 @@ const ( ) type VersionedResource struct { - res ctlres.Resource - allRules []ctlconf.TemplateRule - stripNameHashSuffix bool + res ctlres.Resource + allRules []ctlconf.TemplateRule + stripNameHashSuffixConfig stripNameHashSuffixConfig } func (d VersionedResource) StripNameHashSuffix() bool { - - stripEnabled := d.stripNameHashSuffix - - // TODO should properly check using a ResourceMatcher - matchesRequiredKind := d.res.Kind() == "ConfigMap" || d.res.Kind() == "Secret" - - return stripEnabled && matchesRequiredKind - + return d.stripNameHashSuffixConfig.EnabledFor(d.res) } func (d VersionedResource) SetBaseName(ver int) { From 72253b1618848609b3257fea1793fe286701293b Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Mon, 29 Aug 2022 22:42:27 +0200 Subject: [PATCH 31/52] Lint fixes --- pkg/kapp/diff/stripnamehashsuffix.go | 2 +- pkg/kapp/diff/stripnamehashsuffix_test.go | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/kapp/diff/stripnamehashsuffix.go b/pkg/kapp/diff/stripnamehashsuffix.go index d872823ea..c6b7155fe 100644 --- a/pkg/kapp/diff/stripnamehashsuffix.go +++ b/pkg/kapp/diff/stripnamehashsuffix.go @@ -13,7 +13,7 @@ type stripNameHashSuffixConfig struct { ResourceMatcher ctlres.ResourceMatcher } -func (d stripNameHashSuffixConfig) EnabledFor(res ctlres.Resource) (stripEnabled bool){ +func (d stripNameHashSuffixConfig) EnabledFor(res ctlres.Resource) (stripEnabled bool) { if d.Enabled { return d.ResourceMatcher.Matches(res) } diff --git a/pkg/kapp/diff/stripnamehashsuffix_test.go b/pkg/kapp/diff/stripnamehashsuffix_test.go index 739b11d67..657bce3d8 100644 --- a/pkg/kapp/diff/stripnamehashsuffix_test.go +++ b/pkg/kapp/diff/stripnamehashsuffix_test.go @@ -14,8 +14,8 @@ import ( func TestStripNameHashSuffix_TestConfig_IncludeExcludeAccrossConfigs(t *testing.T) { requireStripNameHashSuffixMatches(t, [][]ctlres.ResourceMatcher{ []ctlres.ResourceMatcher{ - ctlres.AnyMatcher { - Matchers: []ctlres.ResourceMatcher { + ctlres.AnyMatcher{ + Matchers: []ctlres.ResourceMatcher{ ctlres.APIVersionKindMatcher{APIVersion: "v1", Kind: "ConfigMap"}, ctlres.APIVersionKindMatcher{APIVersion: "v2", Kind: "MyKind"}, }, @@ -32,8 +32,8 @@ func TestStripNameHashSuffix_TestConfig_IncludeExcludeAccrossConfigs(t *testing. func TestStripNameHashSuffix_TestConfig_IncludeExcludeSingleConfig(t *testing.T) { requireStripNameHashSuffixMatches(t, [][]ctlres.ResourceMatcher{ []ctlres.ResourceMatcher{ - ctlres.AnyMatcher { - Matchers: []ctlres.ResourceMatcher { + ctlres.AnyMatcher{ + Matchers: []ctlres.ResourceMatcher{ ctlres.APIVersionKindMatcher{APIVersion: "v1", Kind: "ConfigMap"}, ctlres.APIVersionKindMatcher{APIVersion: "v2", Kind: "MyKind"}, }, From dfd2ccbcb9b00cb54f66c158106b7a974fb8f941 Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Mon, 29 Aug 2022 23:07:17 +0200 Subject: [PATCH 32/52] Fixes after module rename --- pkg/kapp/diff/stripnamehashsuffix.go | 4 ++-- pkg/kapp/diff/stripnamehashsuffix_test.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/kapp/diff/stripnamehashsuffix.go b/pkg/kapp/diff/stripnamehashsuffix.go index c6b7155fe..b0d6403e3 100644 --- a/pkg/kapp/diff/stripnamehashsuffix.go +++ b/pkg/kapp/diff/stripnamehashsuffix.go @@ -4,8 +4,8 @@ package diff import ( - ctlconf "github.com/k14s/kapp/pkg/kapp/config" - ctlres "github.com/k14s/kapp/pkg/kapp/resources" + ctlconf "github.com/vmware-tanzu/carvel-kapp/pkg/kapp/config" + ctlres "github.com/vmware-tanzu/carvel-kapp/pkg/kapp/resources" ) type stripNameHashSuffixConfig struct { diff --git a/pkg/kapp/diff/stripnamehashsuffix_test.go b/pkg/kapp/diff/stripnamehashsuffix_test.go index 657bce3d8..12504812c 100644 --- a/pkg/kapp/diff/stripnamehashsuffix_test.go +++ b/pkg/kapp/diff/stripnamehashsuffix_test.go @@ -6,8 +6,8 @@ package diff import ( "testing" - ctlres "github.com/k14s/kapp/pkg/kapp/resources" "github.com/stretchr/testify/require" + ctlres "github.com/vmware-tanzu/carvel-kapp/pkg/kapp/resources" ) // test that we can exclude specific configsmaps from default From 3081bd5f33948608333c1e273ec6fe607b6bff04 Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Sun, 4 Sep 2022 16:54:40 +0200 Subject: [PATCH 33/52] Fix test resources after config changed --- test/e2e/res/kustomize/components/kapp-config/kapp-cm.yml | 3 ++- test/e2e/res/kustomize/overlays/versioned1/kapp.yml | 3 ++- test/e2e/res/kustomize/overlays/versioned2/kapp.yml | 3 ++- test/e2e/res/kustomize/overlays/versioned3/kapp.yml | 3 ++- 4 files changed, 8 insertions(+), 4 deletions(-) diff --git a/test/e2e/res/kustomize/components/kapp-config/kapp-cm.yml b/test/e2e/res/kustomize/components/kapp-config/kapp-cm.yml index b77c25701..a49de3ac6 100644 --- a/test/e2e/res/kustomize/components/kapp-config/kapp-cm.yml +++ b/test/e2e/res/kustomize/components/kapp-config/kapp-cm.yml @@ -3,4 +3,5 @@ kind: Config metadata: name: irrelevant -StripNameHashSuffix: true +StripNameHashSuffixConfig: + Enabled: true diff --git a/test/e2e/res/kustomize/overlays/versioned1/kapp.yml b/test/e2e/res/kustomize/overlays/versioned1/kapp.yml index 043cc33bb..462f20cd5 100644 --- a/test/e2e/res/kustomize/overlays/versioned1/kapp.yml +++ b/test/e2e/res/kustomize/overlays/versioned1/kapp.yml @@ -5,7 +5,8 @@ kind: ConfigMap metadata: name: config-map-7tgk8db28b --- -StripNameHashSuffix: true +StripNameHashSuffixConfig: + Enabled: true apiVersion: kapp.k14s.io/v1alpha1 kind: Config metadata: diff --git a/test/e2e/res/kustomize/overlays/versioned2/kapp.yml b/test/e2e/res/kustomize/overlays/versioned2/kapp.yml index c604e23d8..b68c21e5c 100644 --- a/test/e2e/res/kustomize/overlays/versioned2/kapp.yml +++ b/test/e2e/res/kustomize/overlays/versioned2/kapp.yml @@ -5,7 +5,8 @@ kind: ConfigMap metadata: name: config-map-798k5k7g9f --- -StripNameHashSuffix: true +StripNameHashSuffixConfig: + Enabled: true apiVersion: kapp.k14s.io/v1alpha1 kind: Config metadata: diff --git a/test/e2e/res/kustomize/overlays/versioned3/kapp.yml b/test/e2e/res/kustomize/overlays/versioned3/kapp.yml index 66a5eb2d2..353bf216d 100644 --- a/test/e2e/res/kustomize/overlays/versioned3/kapp.yml +++ b/test/e2e/res/kustomize/overlays/versioned3/kapp.yml @@ -5,7 +5,8 @@ kind: ConfigMap metadata: name: config-map-5bghbfbdc8 --- -StripNameHashSuffix: true +StripNameHashSuffixConfig: + Enabled: true apiVersion: kapp.k14s.io/v1alpha1 kind: Config metadata: From d837a402debaa25cd42fc92ced149f4ec8ae5b5e Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Sun, 4 Sep 2022 16:54:58 +0200 Subject: [PATCH 34/52] Allow to call gen.sh from any directory --- test/e2e/res/kustomize/gen.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/test/e2e/res/kustomize/gen.sh b/test/e2e/res/kustomize/gen.sh index 69c7248fe..1ba4306d3 100755 --- a/test/e2e/res/kustomize/gen.sh +++ b/test/e2e/res/kustomize/gen.sh @@ -1,4 +1,5 @@ #!/usr/bin/env bash +cd $(dirname ${BASH_SOURCE}) for overlay in overlays/*/ do kustomize build $overlay -o $overlay/kapp.yml From 9cb989a6892da6fcf541e9b2cfc5b2a6c6e120b6 Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Sun, 4 Sep 2022 16:54:15 +0200 Subject: [PATCH 35/52] Fix comparing diff test --- test/e2e/versioned_stripnamesuffix_test.go | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/test/e2e/versioned_stripnamesuffix_test.go b/test/e2e/versioned_stripnamesuffix_test.go index d103d7cf1..9e8800a09 100644 --- a/test/e2e/versioned_stripnamesuffix_test.go +++ b/test/e2e/versioned_stripnamesuffix_test.go @@ -6,6 +6,7 @@ package e2e import ( "fmt" "testing" + "regexp" uitest "github.com/cppforlife/go-cli-ui/ui/test" "github.com/stretchr/testify/require" @@ -52,17 +53,25 @@ func TestStripNameSuffixBasic(t *testing.T) { return } - //fmt.Println(stdout) + // comparing the whole diff is unreliable; depending on the k8s version + // managedFields will (not) be set and as such (not) included in the diff. + // in that regard it also seems unreliable to assume "data" diff will + // always be in the first lines. + addRE := regexp.MustCompile(`(?m)^ \d+ \+ foo: bar$`) + delRE := regexp.MustCompile(`(?m)^ \d+ \- foo: foo$`) expectedNote := "Op: 1 create, 1 delete, 0 update, 0 noop, 0 exists" resp := uitest.JSONUIFromBytes(t, []byte(stdout)) - // Ensure one diff is shown - require.Exactlyf(t, 1, len(resp.Blocks), "Expected to see exactly one diff") + diffBlock := resp.Blocks[0] + actualNote := resp.Tables[0].Notes[0] + + require.Regexpf(t, addRE, diffBlock, "Expected to see new line in diff") + require.Regexpf(t, delRE, diffBlock, "Expected to see old line in diff") // Ensure old ConfigMap is deleted - require.Exactlyf(t, expectedNote, replaceAnnsLabels(resp.Tables[0].Notes[0]), "Expected to one delete and one create Op") + require.Exactlyf(t, expectedNote, actualNote, "Expected to one delete and one create Op") } func TestStripNameSuffixNoop(t *testing.T) { From be2c3f2d030c75f240dd801adec3e2ea4813d7c1 Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Sun, 4 Sep 2022 20:26:51 +0200 Subject: [PATCH 36/52] gofmt --- test/e2e/versioned_stripnamesuffix_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e/versioned_stripnamesuffix_test.go b/test/e2e/versioned_stripnamesuffix_test.go index 9e8800a09..b16849527 100644 --- a/test/e2e/versioned_stripnamesuffix_test.go +++ b/test/e2e/versioned_stripnamesuffix_test.go @@ -5,8 +5,8 @@ package e2e import ( "fmt" - "testing" "regexp" + "testing" uitest "github.com/cppforlife/go-cli-ui/ui/test" "github.com/stretchr/testify/require" From c8b1e260de5161df492c52d0f8b465cbada14146 Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Sun, 11 Sep 2022 10:51:31 +0200 Subject: [PATCH 37/52] Drop old suffix strip implementation --- pkg/kapp/diff/change_set_with_versioned_rs.go | 81 +++++++++---------- pkg/kapp/diff/versioned_resource.go | 73 +---------------- 2 files changed, 41 insertions(+), 113 deletions(-) diff --git a/pkg/kapp/diff/change_set_with_versioned_rs.go b/pkg/kapp/diff/change_set_with_versioned_rs.go index ae65eabf8..81a88561f 100644 --- a/pkg/kapp/diff/change_set_with_versioned_rs.go +++ b/pkg/kapp/diff/change_set_with_versioned_rs.go @@ -33,10 +33,10 @@ func NewChangeSetWithVersionedRs(existingRs, newRs []ctlres.Resource, } func (d ChangeSetWithVersionedRs) Calculate() ([]Change, error) { - existingRs := d.existingVersionedResources(d.existingRs) + existingRs := existingVersionedResources(d.existingRs) existingRsGrouped := d.groupResources(existingRs.Versioned) - newRs := d.newVersionedResources(d.newRs) + newRs := newVersionedResources(d.newRs) allChanges := []Change{} d.assignNewNames(newRs, existingRsGrouped) @@ -77,24 +77,19 @@ func (d ChangeSetWithVersionedRs) Calculate() ([]Change, error) { return allChanges, nil } -func (d ChangeSetWithVersionedRs) versionedResourceName(res ctlres.Resource) VersionedResource { - return VersionedResource{res, nil, d.stripNameHashSuffixConfig} -} - func (d ChangeSetWithVersionedRs) groupResources(rs []ctlres.Resource) map[string][]ctlres.Resource { result := map[string][]ctlres.Resource{} groupByFunc := func(res ctlres.Resource) string { - versionedRes := d.versionedResourceName(res) - if versionedRes.IsVersioned() { - return versionedRes.UniqVersionedKey().String() + if _, found := res.Annotations()[versionedResAnnKey]; found { + return VersionedResource{res, nil}.UniqVersionedKey().String() } panic("Expected to find versioned annotation on resource") } for resKey, subRs := range (GroupResources{rs, groupByFunc}).Resources() { sort.Slice(subRs, func(i, j int) bool { - return d.versionedResourceName(subRs[i]).Version() < d.versionedResourceName(subRs[j]).Version() + return VersionedResource{subRs[i], nil}.Version() < VersionedResource{subRs[j], nil}.Version() }) result[resKey] = subRs } @@ -107,29 +102,28 @@ func (d ChangeSetWithVersionedRs) assignNewNames( // TODO name isnt used during diffing, should it? for _, newRes := range newRs.Versioned { - newVerRes := d.versionedResourceName(newRes) + newVerRes := VersionedResource{newRes, nil} newResKey := newVerRes.UniqVersionedKey().String() if existingRs, found := existingRsGrouped[newResKey]; found { - existingRes := d.versionedResourceName(existingRs[len(existingRs)-1]) - newVerRes.AssignNextVersion(existingRes) + existingRes := existingRs[len(existingRs)-1] + newVerRes.SetBaseName(VersionedResource{existingRes, nil}.Version() + 1) } else { - newVerRes.AssignNewVersion() + newVerRes.SetBaseName(1) } } } func (d ChangeSetWithVersionedRs) addAndKeepChanges( newRs versionedResources, existingRsGrouped map[string][]ctlres.Resource) ( - []Change, map[string]Change, error) { + []Change, map[string]ctlres.Resource, error) { changes := []Change{} - alreadyAdded := map[string]Change{} + alreadyAdded := map[string]ctlres.Resource{} for _, newRes := range newRs.Versioned { - newResKey := d.versionedResourceName(newRes).UniqVersionedKey().String() + newResKey := VersionedResource{newRes, nil}.UniqVersionedKey().String() usedRes := newRes - var change Change if existingRs, found := existingRsGrouped[newResKey]; found { existingRes := existingRs[len(existingRs)-1] @@ -142,27 +136,25 @@ func (d ChangeSetWithVersionedRs) addAndKeepChanges( switch updateChange.Op() { case ChangeOpUpdate: - change = d.newAddChangeFromUpdateChange(newRes, updateChange) - changes = append(changes, change) + changes = append(changes, d.newAddChangeFromUpdateChange(newRes, updateChange)) case ChangeOpKeep: // Use latest copy of resource to update affected resources usedRes = existingRes - change = d.newKeepChange(existingRes) - changes = append(changes, change) + changes = append(changes, d.newKeepChange(existingRes)) default: panic(fmt.Sprintf("Unexpected change op %s", updateChange.Op())) } } else { // Since there no existing resource, create change for new resource - change, err := d.newChange(nil, newRes) + addChange, err := d.newChange(nil, newRes) if err != nil { return nil, nil, err } - changes = append(changes, change) + changes = append(changes, addChange) } // Update both versioned and non-versioned - verRes := VersionedResource{usedRes, d.rules, d.stripNameHashSuffixConfig} + verRes := VersionedResource{usedRes, d.rules} err := verRes.UpdateAffected(newRs.NonVersioned) if err != nil { @@ -174,7 +166,7 @@ func (d ChangeSetWithVersionedRs) addAndKeepChanges( return nil, nil, err } - alreadyAdded[newResKey] = change + alreadyAdded[newResKey] = newRes } return changes, alreadyAdded, nil @@ -189,7 +181,7 @@ func (d ChangeSetWithVersionedRs) newAddChangeFromUpdateChange( func (d ChangeSetWithVersionedRs) noopAndDeleteChanges( existingRsGrouped map[string][]ctlres.Resource, - alreadyAdded map[string]Change) ([]Change, error) { + alreadyAdded map[string]ctlres.Resource) ([]Change, error) { changes := []Change{} @@ -197,9 +189,9 @@ func (d ChangeSetWithVersionedRs) noopAndDeleteChanges( for existingResKey, existingRs := range existingRsGrouped { numToKeep := 0 - if change, found := alreadyAdded[existingResKey]; found { + if newRes, found := alreadyAdded[existingResKey]; found { var err error - numToKeep, err = d.numOfResourcesToKeep(change) + numToKeep, err = d.numOfResourcesToKeep(newRes) if err != nil { return nil, err } @@ -234,10 +226,9 @@ func (d ChangeSetWithVersionedRs) newNoopChange(existingRes ctlres.Resource) Cha return NewChangePrecalculated(existingRes, nil, nil, ChangeOpNoop, nil, OpsDiff{}) } -func (d ChangeSetWithVersionedRs) numOfResourcesToKeep(change Change) (int, error) { - res := change.NewOrExistingResource() - versionedRes := d.versionedResourceName(res) - var numToKeep int +func (ChangeSetWithVersionedRs) numOfResourcesToKeep(res ctlres.Resource) (int, error) { + // TODO get rid of arbitrary cut off + numToKeep := 5 if numToKeepAnn, found := res.Annotations()[versionedResNumVersAnnKey]; found { var err error @@ -248,14 +239,7 @@ func (d ChangeSetWithVersionedRs) numOfResourcesToKeep(change Change) (int, erro if numToKeep < 1 { return 0, fmt.Errorf("Expected annotation '%s' value to be a >= 1", versionedResNumVersAnnKey) } - } else if versionedRes.IsVersioned() && !versionedRes.IsTracked() { - if change.Op() == ChangeOpKeep { - numToKeep = 1 - } else { - numToKeep = 0 - } } else { - // TODO get rid of arbitrary cut off numToKeep = 5 } @@ -275,12 +259,13 @@ type versionedResources struct { NonVersioned []ctlres.Resource } -func (d ChangeSetWithVersionedRs) newVersionedResources(rs []ctlres.Resource) versionedResources { +func newVersionedResources(rs []ctlres.Resource) versionedResources { var result versionedResources for _, res := range rs { + _, hasVersionedAnn := res.Annotations()[versionedResAnnKey] _, hasVersionedOrigAnn := res.Annotations()[versionedResOrigAnnKey] - if d.versionedResourceName(res).IsVersioned() { + if hasVersionedAnn { result.Versioned = append(result.Versioned, res) if hasVersionedOrigAnn { result.NonVersioned = append(result.NonVersioned, res.DeepCopy()) @@ -292,10 +277,18 @@ func (d ChangeSetWithVersionedRs) newVersionedResources(rs []ctlres.Resource) ve return result } -func (d ChangeSetWithVersionedRs) existingVersionedResources(rs []ctlres.Resource) versionedResources { +func existingVersionedResources(rs []ctlres.Resource) versionedResources { var result versionedResources for _, res := range rs { - if d.versionedResourceName(res).IsExistingVersioned() { + // Expect that versioned resources should not be transient + // (Annotations may have been copied from versioned resources + // onto transient resources for non-versioning related purposes). + _, hasVersionedAnn := res.Annotations()[versionedResAnnKey] + + versionedRs := VersionedResource{res: res} + _, version := versionedRs.BaseNameAndVersion() + + if hasVersionedAnn && !res.Transient() && version != "" { result.Versioned = append(result.Versioned, res) } else { result.NonVersioned = append(result.NonVersioned, res) diff --git a/pkg/kapp/diff/versioned_resource.go b/pkg/kapp/diff/versioned_resource.go index 6e6a5f186..1240907f1 100644 --- a/pkg/kapp/diff/versioned_resource.go +++ b/pkg/kapp/diff/versioned_resource.go @@ -19,21 +19,13 @@ const ( ) type VersionedResource struct { - res ctlres.Resource - allRules []ctlconf.TemplateRule - stripNameHashSuffixConfig stripNameHashSuffixConfig -} - -func (d VersionedResource) StripNameHashSuffix() bool { - return d.stripNameHashSuffixConfig.EnabledFor(d.res) + res ctlres.Resource + allRules []ctlconf.TemplateRule } func (d VersionedResource) SetBaseName(ver int) { - if d.trackVersions() { - currentName, _ := d.BaseNameAndVersion() - name := fmt.Sprintf("%s%s%d", currentName, nameSuffixSep, ver) - d.res.SetName(name) - } + name := fmt.Sprintf("%s%s%d", d.res.Name(), nameSuffixSep, ver) + d.res.SetName(name) } func (d VersionedResource) BaseNameAndVersion() (string, string) { @@ -42,21 +34,12 @@ func (d VersionedResource) BaseNameAndVersion() (string, string) { if len(pieces) > 1 { return strings.Join(pieces[0:len(pieces)-1], nameSuffixSep), pieces[len(pieces)-1] } - if d.StripNameHashSuffix() { - pieces = strings.Split(name, "-") - if len(pieces) > 1 { - return strings.Join(pieces[0:len(pieces)-1], "-"), "" - } - } return name, "" } func (d VersionedResource) Version() int { _, ver := d.BaseNameAndVersion() if len(ver) == 0 { - if d.StripNameHashSuffix() { - return -1 - } panic(fmt.Sprintf("Missing version in versioned resource '%s'", d.res.Description())) } @@ -73,55 +56,7 @@ func (d VersionedResource) UniqVersionedKey() ctlres.UniqueResourceKey { return ctlres.NewUniqueResourceKeyWithCustomName(d.res, baseName) } -func (d VersionedResource) trackVersions() bool { - _, hasVersionedAnn := d.res.Annotations()[versionedResAnnKey] - return hasVersionedAnn -} - -func (d VersionedResource) IsVersioned() bool { - if d.StripNameHashSuffix() { - return true - } - return d.trackVersions() -} - -func (d VersionedResource) IsTracked() bool { - return d.IsVersioned() && d.trackVersions() -} - -func (d VersionedResource) IsExistingVersioned() bool { - - // Expect that versioned resources should not be transient - // (Annotations may have been copied from versioned resources - // onto transient resources for non-versioning related purposes). - notTransient := !d.res.Transient() - - // TODO check moved during refactoring, but not sure why it is required - _, version := d.BaseNameAndVersion() - hasVersion := version != "" - - versionUnnecessary := !d.trackVersions() - - return d.IsVersioned() && notTransient && (hasVersion || versionUnnecessary) - -} - -func (d VersionedResource) AssignNextVersion(old VersionedResource) { - d.SetBaseName(old.Version() + 1) -} - -func (d VersionedResource) AssignNewVersion() { - d.SetBaseName(1) -} - func (d VersionedResource) UpdateAffected(rs []ctlres.Resource) error { - - if !d.trackVersions() { - // if we're not tracking versions we don't update any names and thus - // don't need to update any references. - return nil - } - rules, err := d.matchingRules() if err != nil { return err From cb5da9e84fb469688e46c5aa7dab9f9204ee6a81 Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Sun, 11 Sep 2022 13:09:44 +0200 Subject: [PATCH 38/52] Skip testing unimplemented functionality for now --- pkg/kapp/diff/change_set_with_versioned_rs_test.go | 2 ++ test/e2e/versioned_stripnamesuffix_test.go | 8 ++++++++ 2 files changed, 10 insertions(+) diff --git a/pkg/kapp/diff/change_set_with_versioned_rs_test.go b/pkg/kapp/diff/change_set_with_versioned_rs_test.go index 60329c6c1..22512f326 100644 --- a/pkg/kapp/diff/change_set_with_versioned_rs_test.go +++ b/pkg/kapp/diff/change_set_with_versioned_rs_test.go @@ -158,6 +158,8 @@ metadata: func TestChangeSet_StripKustomizeSuffix(t *testing.T) { + t.Skip("not yet implemented") + existingRs := ctlres.MustNewResourceFromBytes([]byte(` apiVersion: v1 kind: ConfigMap diff --git a/test/e2e/versioned_stripnamesuffix_test.go b/test/e2e/versioned_stripnamesuffix_test.go index b16849527..efe40f02b 100644 --- a/test/e2e/versioned_stripnamesuffix_test.go +++ b/test/e2e/versioned_stripnamesuffix_test.go @@ -39,6 +39,10 @@ func kappDeployOverlay(kapp Kapp, name string, app string) (string, error) { } func TestStripNameSuffixBasic(t *testing.T) { + + t.Skip("not implemented yet") + return + kapp, appName, cleanup := setup(t) defer cleanup() @@ -75,6 +79,10 @@ func TestStripNameSuffixBasic(t *testing.T) { } func TestStripNameSuffixNoop(t *testing.T) { + + t.Skip("not implemented yet") + return + kapp, appName, cleanup := setup(t) defer cleanup() From 2bf6a39c44184780384fb3ac07c1b12754091dd1 Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Sun, 11 Sep 2022 12:34:35 +0200 Subject: [PATCH 39/52] instantiate VersionedResource as such --- pkg/kapp/diff/change_set_with_versioned_rs.go | 113 ++++++++++-------- pkg/kapp/diff/group_resources.go | 14 +-- 2 files changed, 65 insertions(+), 62 deletions(-) diff --git a/pkg/kapp/diff/change_set_with_versioned_rs.go b/pkg/kapp/diff/change_set_with_versioned_rs.go index 81a88561f..fe9e8bc38 100644 --- a/pkg/kapp/diff/change_set_with_versioned_rs.go +++ b/pkg/kapp/diff/change_set_with_versioned_rs.go @@ -33,10 +33,10 @@ func NewChangeSetWithVersionedRs(existingRs, newRs []ctlres.Resource, } func (d ChangeSetWithVersionedRs) Calculate() ([]Change, error) { - existingRs := existingVersionedResources(d.existingRs) + existingRs := d.existingVersionedResources() existingRsGrouped := d.groupResources(existingRs.Versioned) - newRs := newVersionedResources(d.newRs) + newRs := d.newVersionedResources() allChanges := []Change{} d.assignNewNames(newRs, existingRsGrouped) @@ -77,37 +77,37 @@ func (d ChangeSetWithVersionedRs) Calculate() ([]Change, error) { return allChanges, nil } -func (d ChangeSetWithVersionedRs) groupResources(rs []ctlres.Resource) map[string][]ctlres.Resource { - result := map[string][]ctlres.Resource{} +func (d ChangeSetWithVersionedRs) groupResources(vrs []VersionedResource) map[string][]VersionedResource { + result := map[string][]VersionedResource{} - groupByFunc := func(res ctlres.Resource) string { + groupByFunc := func(ver VersionedResource) string { + res := ver.res if _, found := res.Annotations()[versionedResAnnKey]; found { - return VersionedResource{res, nil}.UniqVersionedKey().String() + return ver.UniqVersionedKey().String() } panic("Expected to find versioned annotation on resource") } - for resKey, subRs := range (GroupResources{rs, groupByFunc}).Resources() { - sort.Slice(subRs, func(i, j int) bool { - return VersionedResource{subRs[i], nil}.Version() < VersionedResource{subRs[j], nil}.Version() + for resKey, subVRs := range (GroupVersionedResources{vrs, groupByFunc}).Resources() { + sort.Slice(subVRs, func(i, j int) bool { + return subVRs[i].Version() < subVRs[j].Version() }) - result[resKey] = subRs + result[resKey] = subVRs } return result } func (d ChangeSetWithVersionedRs) assignNewNames( - newRs versionedResources, existingRsGrouped map[string][]ctlres.Resource) { + newVRs versionedResources, existingVRsGrouped map[string][]VersionedResource) { // TODO name isnt used during diffing, should it? - for _, newRes := range newRs.Versioned { - newVerRes := VersionedResource{newRes, nil} + for _, newVerRes := range newVRs.Versioned { newResKey := newVerRes.UniqVersionedKey().String() - if existingRs, found := existingRsGrouped[newResKey]; found { - existingRes := existingRs[len(existingRs)-1] - newVerRes.SetBaseName(VersionedResource{existingRes, nil}.Version() + 1) + if existingVRs, found := existingVRsGrouped[newResKey]; found { + existingVerRes := existingVRs[len(existingVRs)-1] + newVerRes.SetBaseName(existingVerRes.Version() + 1) } else { newVerRes.SetBaseName(1) } @@ -115,38 +115,38 @@ func (d ChangeSetWithVersionedRs) assignNewNames( } func (d ChangeSetWithVersionedRs) addAndKeepChanges( - newRs versionedResources, existingRsGrouped map[string][]ctlres.Resource) ( - []Change, map[string]ctlres.Resource, error) { + newVRs versionedResources, existingVRsGrouped map[string][]VersionedResource) ( + []Change, map[string]VersionedResource, error) { changes := []Change{} - alreadyAdded := map[string]ctlres.Resource{} + alreadyAdded := map[string]VersionedResource{} - for _, newRes := range newRs.Versioned { - newResKey := VersionedResource{newRes, nil}.UniqVersionedKey().String() - usedRes := newRes + for _, newVerRes := range newVRs.Versioned { + newResKey := newVerRes.UniqVersionedKey().String() + usedVerRes := newVerRes - if existingRs, found := existingRsGrouped[newResKey]; found { - existingRes := existingRs[len(existingRs)-1] + if existingVRs, found := existingVRsGrouped[newResKey]; found { + existingVerRes := existingVRs[len(existingVRs)-1] // Calculate update change to determine if anything changed - updateChange, err := d.newChange(existingRes, newRes) + updateChange, err := d.newChange(existingVerRes.res, newVerRes.res) if err != nil { return nil, nil, err } switch updateChange.Op() { case ChangeOpUpdate: - changes = append(changes, d.newAddChangeFromUpdateChange(newRes, updateChange)) + changes = append(changes, d.newAddChangeFromUpdateChange(newVerRes.res, updateChange)) case ChangeOpKeep: // Use latest copy of resource to update affected resources - usedRes = existingRes - changes = append(changes, d.newKeepChange(existingRes)) + usedVerRes = existingVerRes + changes = append(changes, d.newKeepChange(existingVerRes.res)) default: panic(fmt.Sprintf("Unexpected change op %s", updateChange.Op())) } } else { // Since there no existing resource, create change for new resource - addChange, err := d.newChange(nil, newRes) + addChange, err := d.newChange(nil, newVerRes.res) if err != nil { return nil, nil, err } @@ -154,19 +154,26 @@ func (d ChangeSetWithVersionedRs) addAndKeepChanges( } // Update both versioned and non-versioned - verRes := VersionedResource{usedRes, d.rules} - err := verRes.UpdateAffected(newRs.NonVersioned) + err := usedVerRes.UpdateAffected(newVRs.NonVersioned) if err != nil { return nil, nil, err } - err = verRes.UpdateAffected(newRs.Versioned) + // TODO ResourceHolder? + // err = usedVerRes.UpdateAffected(newVRs.Versioned) + // workaround: + newRs := []ctlres.Resource{} + for _, newVerRes := range newVRs.Versioned { + newRs = append(newRs, newVerRes.res) + } + // workaround end + err = usedVerRes.UpdateAffected(newRs) if err != nil { return nil, nil, err } - alreadyAdded[newResKey] = newRes + alreadyAdded[newResKey] = newVerRes } return changes, alreadyAdded, nil @@ -180,29 +187,29 @@ func (d ChangeSetWithVersionedRs) newAddChangeFromUpdateChange( } func (d ChangeSetWithVersionedRs) noopAndDeleteChanges( - existingRsGrouped map[string][]ctlres.Resource, - alreadyAdded map[string]ctlres.Resource) ([]Change, error) { + existingVRsGrouped map[string][]VersionedResource, + alreadyAdded map[string]VersionedResource) ([]Change, error) { changes := []Change{} // Find existing resources that were not already diffed (not in new set of resources) - for existingResKey, existingRs := range existingRsGrouped { + for existingResKey, existingVRs := range existingVRsGrouped { numToKeep := 0 - if newRes, found := alreadyAdded[existingResKey]; found { + if newVRes, found := alreadyAdded[existingResKey]; found { var err error - numToKeep, err = d.numOfResourcesToKeep(newRes) + numToKeep, err = d.numOfResourcesToKeep(newVRes.res) if err != nil { return nil, err } } - if numToKeep > len(existingRs) { - numToKeep = len(existingRs) + if numToKeep > len(existingVRs) { + numToKeep = len(existingVRs) } // Create changes to delete all or extra resources - for _, existingRes := range existingRs[0 : len(existingRs)-numToKeep] { - change, err := d.newChange(existingRes, nil) + for _, existingVRes := range existingVRs[0 : len(existingVRs)-numToKeep] { + change, err := d.newChange(existingVRes.res, nil) if err != nil { return nil, err } @@ -210,8 +217,8 @@ func (d ChangeSetWithVersionedRs) noopAndDeleteChanges( } // Create changes that "noop" resources - for _, existingRes := range existingRs[len(existingRs)-numToKeep:] { - changes = append(changes, d.newNoopChange(existingRes)) + for _, existingVRes := range existingVRs[len(existingVRs)-numToKeep:] { + changes = append(changes, d.newNoopChange(existingVRes.res)) } } @@ -255,18 +262,18 @@ func (d ChangeSetWithVersionedRs) newChange(existingRes, newRes ctlres.Resource) } type versionedResources struct { - Versioned []ctlres.Resource + Versioned []VersionedResource NonVersioned []ctlres.Resource } -func newVersionedResources(rs []ctlres.Resource) versionedResources { +func (d ChangeSetWithVersionedRs) newVersionedResources() versionedResources { var result versionedResources - for _, res := range rs { + for _, res := range d.newRs { _, hasVersionedAnn := res.Annotations()[versionedResAnnKey] _, hasVersionedOrigAnn := res.Annotations()[versionedResOrigAnnKey] if hasVersionedAnn { - result.Versioned = append(result.Versioned, res) + result.Versioned = append(result.Versioned, VersionedResource{res, d.rules}) if hasVersionedOrigAnn { result.NonVersioned = append(result.NonVersioned, res.DeepCopy()) } @@ -277,19 +284,19 @@ func newVersionedResources(rs []ctlres.Resource) versionedResources { return result } -func existingVersionedResources(rs []ctlres.Resource) versionedResources { +func (d ChangeSetWithVersionedRs) existingVersionedResources() versionedResources { var result versionedResources - for _, res := range rs { + for _, res := range d.existingRs { // Expect that versioned resources should not be transient // (Annotations may have been copied from versioned resources // onto transient resources for non-versioning related purposes). _, hasVersionedAnn := res.Annotations()[versionedResAnnKey] - versionedRs := VersionedResource{res: res} - _, version := versionedRs.BaseNameAndVersion() + versionedRes := VersionedResource{res, d.rules} + _, version := versionedRes.BaseNameAndVersion() if hasVersionedAnn && !res.Transient() && version != "" { - result.Versioned = append(result.Versioned, res) + result.Versioned = append(result.Versioned, versionedRes) } else { result.NonVersioned = append(result.NonVersioned, res) } diff --git a/pkg/kapp/diff/group_resources.go b/pkg/kapp/diff/group_resources.go index 8d54d256b..b5ada61ed 100644 --- a/pkg/kapp/diff/group_resources.go +++ b/pkg/kapp/diff/group_resources.go @@ -3,17 +3,13 @@ package diff -import ( - ctlres "github.com/vmware-tanzu/carvel-kapp/pkg/kapp/resources" -) - -type GroupResources struct { - resources []ctlres.Resource - groupFunc func(ctlres.Resource) string +type GroupVersionedResources struct { + resources []VersionedResource + groupFunc func(VersionedResource) string } -func (r GroupResources) Resources() map[string][]ctlres.Resource { - result := map[string][]ctlres.Resource{} +func (r GroupVersionedResources) Resources() map[string][]VersionedResource { + result := map[string][]VersionedResource{} for _, res := range r.resources { id := r.groupFunc(res) From f485fcbe0456c47062f7c9fb98a8a5842054b2ba Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Sun, 11 Sep 2022 13:10:33 +0200 Subject: [PATCH 40/52] Turn VersionedResource into an interface to allow implementing alternative versioned resources --- pkg/kapp/diff/change_set_with_versioned_rs.go | 22 ++++++------- pkg/kapp/diff/versioned_resource.go | 31 +++++++++++++------ 2 files changed, 33 insertions(+), 20 deletions(-) diff --git a/pkg/kapp/diff/change_set_with_versioned_rs.go b/pkg/kapp/diff/change_set_with_versioned_rs.go index fe9e8bc38..577037391 100644 --- a/pkg/kapp/diff/change_set_with_versioned_rs.go +++ b/pkg/kapp/diff/change_set_with_versioned_rs.go @@ -81,7 +81,7 @@ func (d ChangeSetWithVersionedRs) groupResources(vrs []VersionedResource) map[st result := map[string][]VersionedResource{} groupByFunc := func(ver VersionedResource) string { - res := ver.res + res := ver.Res() if _, found := res.Annotations()[versionedResAnnKey]; found { return ver.UniqVersionedKey().String() } @@ -129,24 +129,24 @@ func (d ChangeSetWithVersionedRs) addAndKeepChanges( existingVerRes := existingVRs[len(existingVRs)-1] // Calculate update change to determine if anything changed - updateChange, err := d.newChange(existingVerRes.res, newVerRes.res) + updateChange, err := d.newChange(existingVerRes.Res(), newVerRes.Res()) if err != nil { return nil, nil, err } switch updateChange.Op() { case ChangeOpUpdate: - changes = append(changes, d.newAddChangeFromUpdateChange(newVerRes.res, updateChange)) + changes = append(changes, d.newAddChangeFromUpdateChange(newVerRes.Res(), updateChange)) case ChangeOpKeep: // Use latest copy of resource to update affected resources usedVerRes = existingVerRes - changes = append(changes, d.newKeepChange(existingVerRes.res)) + changes = append(changes, d.newKeepChange(existingVerRes.Res())) default: panic(fmt.Sprintf("Unexpected change op %s", updateChange.Op())) } } else { // Since there no existing resource, create change for new resource - addChange, err := d.newChange(nil, newVerRes.res) + addChange, err := d.newChange(nil, newVerRes.Res()) if err != nil { return nil, nil, err } @@ -165,7 +165,7 @@ func (d ChangeSetWithVersionedRs) addAndKeepChanges( // workaround: newRs := []ctlres.Resource{} for _, newVerRes := range newVRs.Versioned { - newRs = append(newRs, newVerRes.res) + newRs = append(newRs, newVerRes.Res()) } // workaround end err = usedVerRes.UpdateAffected(newRs) @@ -198,7 +198,7 @@ func (d ChangeSetWithVersionedRs) noopAndDeleteChanges( if newVRes, found := alreadyAdded[existingResKey]; found { var err error - numToKeep, err = d.numOfResourcesToKeep(newVRes.res) + numToKeep, err = d.numOfResourcesToKeep(newVRes.Res()) if err != nil { return nil, err } @@ -209,7 +209,7 @@ func (d ChangeSetWithVersionedRs) noopAndDeleteChanges( // Create changes to delete all or extra resources for _, existingVRes := range existingVRs[0 : len(existingVRs)-numToKeep] { - change, err := d.newChange(existingVRes.res, nil) + change, err := d.newChange(existingVRes.Res(), nil) if err != nil { return nil, err } @@ -218,7 +218,7 @@ func (d ChangeSetWithVersionedRs) noopAndDeleteChanges( // Create changes that "noop" resources for _, existingVRes := range existingVRs[len(existingVRs)-numToKeep:] { - changes = append(changes, d.newNoopChange(existingVRes.res)) + changes = append(changes, d.newNoopChange(existingVRes.Res())) } } @@ -273,7 +273,7 @@ func (d ChangeSetWithVersionedRs) newVersionedResources() versionedResources { _, hasVersionedOrigAnn := res.Annotations()[versionedResOrigAnnKey] if hasVersionedAnn { - result.Versioned = append(result.Versioned, VersionedResource{res, d.rules}) + result.Versioned = append(result.Versioned, VersionedResourceImpl{res, d.rules}) if hasVersionedOrigAnn { result.NonVersioned = append(result.NonVersioned, res.DeepCopy()) } @@ -292,7 +292,7 @@ func (d ChangeSetWithVersionedRs) existingVersionedResources() versionedResource // onto transient resources for non-versioning related purposes). _, hasVersionedAnn := res.Annotations()[versionedResAnnKey] - versionedRes := VersionedResource{res, d.rules} + versionedRes := VersionedResourceImpl{res, d.rules} _, version := versionedRes.BaseNameAndVersion() if hasVersionedAnn && !res.Transient() && version != "" { diff --git a/pkg/kapp/diff/versioned_resource.go b/pkg/kapp/diff/versioned_resource.go index 1240907f1..206a36f66 100644 --- a/pkg/kapp/diff/versioned_resource.go +++ b/pkg/kapp/diff/versioned_resource.go @@ -18,17 +18,30 @@ const ( nameSuffixSep = "-ver-" ) -type VersionedResource struct { +type VersionedResource interface { + Res() ctlres.Resource + SetBaseName(ver int) + BaseNameAndVersion() (string, string) + Version() int + UniqVersionedKey() ctlres.UniqueResourceKey + UpdateAffected(rs []ctlres.Resource) error +} + +type VersionedResourceImpl struct { res ctlres.Resource allRules []ctlconf.TemplateRule } -func (d VersionedResource) SetBaseName(ver int) { +func (d VersionedResourceImpl) Res() ctlres.Resource { + return d.res +} + +func (d VersionedResourceImpl) SetBaseName(ver int) { name := fmt.Sprintf("%s%s%d", d.res.Name(), nameSuffixSep, ver) d.res.SetName(name) } -func (d VersionedResource) BaseNameAndVersion() (string, string) { +func (d VersionedResourceImpl) BaseNameAndVersion() (string, string) { name := d.res.Name() pieces := strings.Split(name, nameSuffixSep) if len(pieces) > 1 { @@ -37,7 +50,7 @@ func (d VersionedResource) BaseNameAndVersion() (string, string) { return name, "" } -func (d VersionedResource) Version() int { +func (d VersionedResourceImpl) Version() int { _, ver := d.BaseNameAndVersion() if len(ver) == 0 { panic(fmt.Sprintf("Missing version in versioned resource '%s'", d.res.Description())) @@ -51,12 +64,12 @@ func (d VersionedResource) Version() int { return verInt } -func (d VersionedResource) UniqVersionedKey() ctlres.UniqueResourceKey { +func (d VersionedResourceImpl) UniqVersionedKey() ctlres.UniqueResourceKey { baseName, _ := d.BaseNameAndVersion() return ctlres.NewUniqueResourceKeyWithCustomName(d.res, baseName) } -func (d VersionedResource) UpdateAffected(rs []ctlres.Resource) error { +func (d VersionedResourceImpl) UpdateAffected(rs []ctlres.Resource) error { rules, err := d.matchingRules() if err != nil { return err @@ -73,7 +86,7 @@ func (d VersionedResource) UpdateAffected(rs []ctlres.Resource) error { return nil } -func (d VersionedResource) updateAffected(rule ctlconf.TemplateRule, rs []ctlres.Resource) error { +func (d VersionedResourceImpl) updateAffected(rule ctlconf.TemplateRule, rs []ctlres.Resource) error { for _, affectedObjRef := range rule.AffectedResources.ObjectReferences { matchers := ctlconf.ResourceMatchers(affectedObjRef.ResourceMatchers).AsResourceMatchers() @@ -122,7 +135,7 @@ func (d VersionedResource) updateAffected(rule ctlconf.TemplateRule, rs []ctlres return nil } -func (d VersionedResource) buildObjRefReplacementFunc( +func (d VersionedResourceImpl) buildObjRefReplacementFunc( affectedObjRef ctlconf.TemplateAffectedObjRef) func(map[string]interface{}) error { baseName, _ := d.BaseNameAndVersion() @@ -171,7 +184,7 @@ func (d VersionedResource) buildObjRefReplacementFunc( } } -func (d VersionedResource) matchingRules() ([]ctlconf.TemplateRule, error) { +func (d VersionedResourceImpl) matchingRules() ([]ctlconf.TemplateRule, error) { var result []ctlconf.TemplateRule for _, rule := range d.allRules { From 5c555a002bc98e0eb04fa9da6216df5aaa401804 Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Sun, 11 Sep 2022 15:37:00 +0200 Subject: [PATCH 41/52] Reimplement suffix strip dropping previous CMs not yet implemented --- pkg/kapp/diff/change_set_with_versioned_rs.go | 82 +++++++++++++------ .../diff/change_set_with_versioned_rs_test.go | 2 - pkg/kapp/diff/stripnamehashsuffix.go | 36 ++++++++ test/e2e/versioned_stripnamesuffix_test.go | 78 ++++++++---------- 4 files changed, 125 insertions(+), 73 deletions(-) diff --git a/pkg/kapp/diff/change_set_with_versioned_rs.go b/pkg/kapp/diff/change_set_with_versioned_rs.go index 577037391..a7d6f635f 100644 --- a/pkg/kapp/diff/change_set_with_versioned_rs.go +++ b/pkg/kapp/diff/change_set_with_versioned_rs.go @@ -81,11 +81,7 @@ func (d ChangeSetWithVersionedRs) groupResources(vrs []VersionedResource) map[st result := map[string][]VersionedResource{} groupByFunc := func(ver VersionedResource) string { - res := ver.Res() - if _, found := res.Annotations()[versionedResAnnKey]; found { - return ver.UniqVersionedKey().String() - } - panic("Expected to find versioned annotation on resource") + return ver.UniqVersionedKey().String() } for resKey, subVRs := range (GroupVersionedResources{vrs, groupByFunc}).Resources() { @@ -269,17 +265,7 @@ type versionedResources struct { func (d ChangeSetWithVersionedRs) newVersionedResources() versionedResources { var result versionedResources for _, res := range d.newRs { - _, hasVersionedAnn := res.Annotations()[versionedResAnnKey] - _, hasVersionedOrigAnn := res.Annotations()[versionedResOrigAnnKey] - - if hasVersionedAnn { - result.Versioned = append(result.Versioned, VersionedResourceImpl{res, d.rules}) - if hasVersionedOrigAnn { - result.NonVersioned = append(result.NonVersioned, res.DeepCopy()) - } - } else { - result.NonVersioned = append(result.NonVersioned, res) - } + result.appendRes(d.VersionedFromNewResource(res)) } return result } @@ -287,19 +273,61 @@ func (d ChangeSetWithVersionedRs) newVersionedResources() versionedResources { func (d ChangeSetWithVersionedRs) existingVersionedResources() versionedResources { var result versionedResources for _, res := range d.existingRs { - // Expect that versioned resources should not be transient - // (Annotations may have been copied from versioned resources - // onto transient resources for non-versioning related purposes). - _, hasVersionedAnn := res.Annotations()[versionedResAnnKey] + result.appendRes(d.VersionedFromExistingResource(res)) + } + return result +} + +func (d *versionedResources) appendRes(ver VersionedResource, nonVer ctlres.Resource) { + if ver != nil { + d.Versioned = append(d.Versioned, ver) + } + if nonVer != nil { + d.NonVersioned = append(d.NonVersioned, nonVer) + } +} - versionedRes := VersionedResourceImpl{res, d.rules} - _, version := versionedRes.BaseNameAndVersion() +func (d ChangeSetWithVersionedRs) VersionedFromNewResource(res ctlres.Resource) (versioned VersionedResource, nonVersioned ctlres.Resource) { - if hasVersionedAnn && !res.Transient() && version != "" { - result.Versioned = append(result.Versioned, versionedRes) - } else { - result.NonVersioned = append(result.NonVersioned, res) + _, hasVersionedAnn := res.Annotations()[versionedResAnnKey] + _, hasVersionedOrigAnn := res.Annotations()[versionedResOrigAnnKey] + + if hasVersionedAnn { + versioned = VersionedResourceImpl{res, d.rules} + if hasVersionedOrigAnn { + nonVersioned = res.DeepCopy() } + return } - return result + + if d.stripNameHashSuffixConfig.EnabledFor(res) { + versioned = HashSuffixResource{res} + return + } + + return nil, res +} + +func (d ChangeSetWithVersionedRs) VersionedFromExistingResource(res ctlres.Resource) (versioned VersionedResource, nonVersioned ctlres.Resource) { + + // Expect that versioned resources should not be transient + // (Annotations may have been copied from versioned resources + // onto transient resources for non-versioning related purposes). + if !res.Transient() { + + _, hasVersionedAnn := res.Annotations()[versionedResAnnKey] + if hasVersionedAnn { + versionedRes := VersionedResourceImpl{res, d.rules} + _, version := versionedRes.BaseNameAndVersion() + if version != "" { + return versionedRes, nil + } + } + + if d.stripNameHashSuffixConfig.EnabledFor(res) { + return HashSuffixResource{res}, nil + } + } + + return nil, res } diff --git a/pkg/kapp/diff/change_set_with_versioned_rs_test.go b/pkg/kapp/diff/change_set_with_versioned_rs_test.go index 22512f326..60329c6c1 100644 --- a/pkg/kapp/diff/change_set_with_versioned_rs_test.go +++ b/pkg/kapp/diff/change_set_with_versioned_rs_test.go @@ -158,8 +158,6 @@ metadata: func TestChangeSet_StripKustomizeSuffix(t *testing.T) { - t.Skip("not yet implemented") - existingRs := ctlres.MustNewResourceFromBytes([]byte(` apiVersion: v1 kind: ConfigMap diff --git a/pkg/kapp/diff/stripnamehashsuffix.go b/pkg/kapp/diff/stripnamehashsuffix.go index b0d6403e3..86e8bb664 100644 --- a/pkg/kapp/diff/stripnamehashsuffix.go +++ b/pkg/kapp/diff/stripnamehashsuffix.go @@ -4,6 +4,8 @@ package diff import ( + "strings" + ctlconf "github.com/vmware-tanzu/carvel-kapp/pkg/kapp/config" ctlres "github.com/vmware-tanzu/carvel-kapp/pkg/kapp/resources" ) @@ -38,3 +40,37 @@ func newStripNameHashSuffixConfigFromConf(confs ctlconf.StripNameHashSuffixConfi enabled, matchers := confs.AggregateToCtlRes() return newStripNameHashSuffixConfig(enabled, matchers) } + +type HashSuffixResource struct { + res ctlres.Resource +} + +func (d HashSuffixResource) Res() ctlres.Resource { + return d.res +} + +func (d HashSuffixResource) SetBaseName(ver int) { + // not necessary +} + +func (d HashSuffixResource) BaseNameAndVersion() (string, string) { + pieces := strings.Split(d.res.Name(), "-") + if len(pieces) > 1 { + return strings.Join(pieces[0:len(pieces)-1], "-"), "" + } + panic("expected suffix!") +} + +func (d HashSuffixResource) Version() int { + return -1 +} + +func (d HashSuffixResource) UniqVersionedKey() ctlres.UniqueResourceKey { + baseName, _ := d.BaseNameAndVersion() + return ctlres.NewUniqueResourceKeyWithCustomName(d.res, baseName) +} + +func (d HashSuffixResource) UpdateAffected(rs []ctlres.Resource) error { + // no updates required + return nil +} diff --git a/test/e2e/versioned_stripnamesuffix_test.go b/test/e2e/versioned_stripnamesuffix_test.go index efe40f02b..2f6f61780 100644 --- a/test/e2e/versioned_stripnamesuffix_test.go +++ b/test/e2e/versioned_stripnamesuffix_test.go @@ -8,22 +8,36 @@ import ( "regexp" "testing" + ui "github.com/cppforlife/go-cli-ui/ui" uitest "github.com/cppforlife/go-cli-ui/ui/test" "github.com/stretchr/testify/require" ) -func setup(t *testing.T) (kapp Kapp, appName string, cleanUp func()) { +func run(t *testing.T, overlay1, overlay2 string) (resp ui.JSONUIResp) { env := BuildEnv(t) logger := Logger{} - kapp = Kapp{t, env.Namespace, env.KappBinaryPath, logger} + kapp := Kapp{t, env.Namespace, env.KappBinaryPath, logger} - appName = "test-versioned-stripnamesuffix" - cleanUp = func() { + appName := "test-versioned-stripnamesuffix" + cleanUp := func() { kapp.Run([]string{"delete", "-a", appName}) } cleanUp() - return + + if _, err := kappDeployOverlay(kapp, overlay1, appName); err != nil { + t.Errorf("Failed to deploy initial overlay!") + } + + stdout, err := kappDeployOverlay(kapp, overlay2, appName) + + defer cleanUp() + + if err != nil { + t.Errorf("Failed to deploy next overlay!") + } + + return uitest.JSONUIFromBytes(t, []byte(stdout)) } func testResOverlayPath(name string) string { @@ -40,22 +54,7 @@ func kappDeployOverlay(kapp Kapp, name string, app string) (string, error) { func TestStripNameSuffixBasic(t *testing.T) { - t.Skip("not implemented yet") - return - - kapp, appName, cleanup := setup(t) - defer cleanup() - - if _, err := kappDeployOverlay(kapp, "versioned1", appName); err != nil { - t.Errorf("Failed to deploy initial overlay!") - } - - stdout, err := kappDeployOverlay(kapp, "versioned2", appName) - - if err != nil { - t.Errorf("Failed to deploy next overlay!") - return - } + resp := run(t, "versioned1", "versioned2") // comparing the whole diff is unreliable; depending on the k8s version // managedFields will (not) be set and as such (not) included in the diff. @@ -64,43 +63,34 @@ func TestStripNameSuffixBasic(t *testing.T) { addRE := regexp.MustCompile(`(?m)^ \d+ \+ foo: bar$`) delRE := regexp.MustCompile(`(?m)^ \d+ \- foo: foo$`) - expectedNote := "Op: 1 create, 1 delete, 0 update, 0 noop, 0 exists" - - resp := uitest.JSONUIFromBytes(t, []byte(stdout)) - diffBlock := resp.Blocks[0] - actualNote := resp.Tables[0].Notes[0] require.Regexpf(t, addRE, diffBlock, "Expected to see new line in diff") require.Regexpf(t, delRE, diffBlock, "Expected to see old line in diff") - - // Ensure old ConfigMap is deleted - require.Exactlyf(t, expectedNote, actualNote, "Expected to one delete and one create Op") } -func TestStripNameSuffixNoop(t *testing.T) { +func TestStripNameSuffixDeleteOld(t *testing.T) { - t.Skip("not implemented yet") - return + t.Skip("not yet implemented") - kapp, appName, cleanup := setup(t) - defer cleanup() + resp := run(t, "versioned1", "versioned2") - if _, err := kappDeployOverlay(kapp, "versioned1", appName); err != nil { - t.Errorf("Failed to deploy initial overlay!") - } + expectedNote := "Op: 1 create, 1 delete, 0 update, 0 noop, 0 exists" + + actualNote := resp.Tables[0].Notes[0] - stdout, err := kappDeployOverlay(kapp, "versioned1", appName) + // Ensure old ConfigMap is deleted + require.Exactlyf(t, expectedNote, actualNote, "Expected to one delete and one create Op") +} - if err != nil { - t.Errorf("Failed to deploy next overlay!") - return - } +func TestStripNameSuffixNoop(t *testing.T) { + + resp := run(t, "versioned1", "versioned1") expectedNote := "Op: 0 create, 0 delete, 0 update, 0 noop, 0 exists" - resp := uitest.JSONUIFromBytes(t, []byte(stdout)) + actualNote := resp.Tables[0].Notes[0] // Ensure current ConfigMap is not deleted - require.Exactlyf(t, expectedNote, replaceAnnsLabels(resp.Tables[0].Notes[0]), "Expected to see no Op's") + require.Exactlyf(t, expectedNote, actualNote, "Expected to see no Op's") } From 121cb030e583f7e09050c1a336945f1c29c38543 Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Sun, 11 Sep 2022 18:02:50 +0200 Subject: [PATCH 42/52] Reimplement deleting old has-suffixed resources --- pkg/kapp/diff/change_set_with_versioned_rs.go | 28 ++++++++++++++++--- test/e2e/versioned_stripnamesuffix_test.go | 2 -- 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/pkg/kapp/diff/change_set_with_versioned_rs.go b/pkg/kapp/diff/change_set_with_versioned_rs.go index a7d6f635f..d9897e21c 100644 --- a/pkg/kapp/diff/change_set_with_versioned_rs.go +++ b/pkg/kapp/diff/change_set_with_versioned_rs.go @@ -192,9 +192,10 @@ func (d ChangeSetWithVersionedRs) noopAndDeleteChanges( for existingResKey, existingVRs := range existingVRsGrouped { numToKeep := 0 - if newVRes, found := alreadyAdded[existingResKey]; found { + newVRes, found := alreadyAdded[existingResKey] + if found { var err error - numToKeep, err = d.numOfResourcesToKeep(newVRes.Res()) + numToKeep, err = d.numOfResourcesToKeep(newVRes) if err != nil { return nil, err } @@ -205,7 +206,18 @@ func (d ChangeSetWithVersionedRs) noopAndDeleteChanges( // Create changes to delete all or extra resources for _, existingVRes := range existingVRs[0 : len(existingVRs)-numToKeep] { - change, err := d.newChange(existingVRes.Res(), nil) + + existingRes := existingVRes.Res() + + var newRes ctlres.Resource + if numToKeep == 0 && found && existingRes.Name() == newVRes.Res().Name() { + // versioned resources without "last X" semantics would also + // delete the not-actually-old existing resource when it is + // reapplied without changes. + newRes = newVRes.Res() + } + + change, err := d.newChange(existingRes, newRes) if err != nil { return nil, err } @@ -229,9 +241,17 @@ func (d ChangeSetWithVersionedRs) newNoopChange(existingRes ctlres.Resource) Cha return NewChangePrecalculated(existingRes, nil, nil, ChangeOpNoop, nil, OpsDiff{}) } -func (ChangeSetWithVersionedRs) numOfResourcesToKeep(res ctlres.Resource) (int, error) { +func (ChangeSetWithVersionedRs) numOfResourcesToKeep(vres VersionedResource) (int, error) { + switch vres.(type) { + case HashSuffixResource: + // there is no meaningful way to order hash-suffixed resources and as such there is no "last X" resources semantic. + // thus simply delete all old resources. + return 0, nil + } + // TODO get rid of arbitrary cut off numToKeep := 5 + res := vres.Res() if numToKeepAnn, found := res.Annotations()[versionedResNumVersAnnKey]; found { var err error diff --git a/test/e2e/versioned_stripnamesuffix_test.go b/test/e2e/versioned_stripnamesuffix_test.go index 2f6f61780..183574035 100644 --- a/test/e2e/versioned_stripnamesuffix_test.go +++ b/test/e2e/versioned_stripnamesuffix_test.go @@ -71,8 +71,6 @@ func TestStripNameSuffixBasic(t *testing.T) { func TestStripNameSuffixDeleteOld(t *testing.T) { - t.Skip("not yet implemented") - resp := run(t, "versioned1", "versioned2") expectedNote := "Op: 1 create, 1 delete, 0 update, 0 noop, 0 exists" From e22da5b4ebafc0e26e489bf45c50b2013a7daa16 Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Sun, 11 Sep 2022 20:16:34 +0200 Subject: [PATCH 43/52] Make dedicated method for converting []VersionedResource --- pkg/kapp/diff/change_set_with_versioned_rs.go | 23 +++++++++++-------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/pkg/kapp/diff/change_set_with_versioned_rs.go b/pkg/kapp/diff/change_set_with_versioned_rs.go index d9897e21c..da15696fc 100644 --- a/pkg/kapp/diff/change_set_with_versioned_rs.go +++ b/pkg/kapp/diff/change_set_with_versioned_rs.go @@ -151,20 +151,12 @@ func (d ChangeSetWithVersionedRs) addAndKeepChanges( // Update both versioned and non-versioned - err := usedVerRes.UpdateAffected(newVRs.NonVersioned) + err := usedVerRes.UpdateAffected(newVRs.NonVersionedRs()) if err != nil { return nil, nil, err } - // TODO ResourceHolder? - // err = usedVerRes.UpdateAffected(newVRs.Versioned) - // workaround: - newRs := []ctlres.Resource{} - for _, newVerRes := range newVRs.Versioned { - newRs = append(newRs, newVerRes.Res()) - } - // workaround end - err = usedVerRes.UpdateAffected(newRs) + err = usedVerRes.UpdateAffected(newVRs.VersionedRs()) if err != nil { return nil, nil, err } @@ -282,6 +274,17 @@ type versionedResources struct { NonVersioned []ctlres.Resource } +func (d versionedResources) VersionedRs() (rs []ctlres.Resource) { + for _, vres := range d.Versioned { + rs = append(rs, vres.Res()) + } + return +} + +func (d versionedResources) NonVersionedRs() []ctlres.Resource { + return d.NonVersioned +} + func (d ChangeSetWithVersionedRs) newVersionedResources() versionedResources { var result versionedResources for _, res := range d.newRs { From f5333ab18620f69c511ea71e9310973bd6f064c6 Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Thu, 22 Sep 2022 12:35:57 +0200 Subject: [PATCH 44/52] Remove naked return --- pkg/kapp/config/conf.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/kapp/config/conf.go b/pkg/kapp/config/conf.go index a3a2b0b62..bb6c8f9ab 100644 --- a/pkg/kapp/config/conf.go +++ b/pkg/kapp/config/conf.go @@ -181,12 +181,14 @@ func (c Conf) ChangeRuleBindings() []ChangeRuleBinding { type StripNameHashSuffixConfigs []StripNameHashSuffixConfig -func (c StripNameHashSuffixConfigs) AggregateToCtlRes() (enabled bool, resourceMatchers [][]ctlres.ResourceMatcher) { +func (c StripNameHashSuffixConfigs) AggregateToCtlRes() (bool, [][]ctlres.ResourceMatcher) { + var enabled bool + var resourceMatchers [][]ctlres.ResourceMatcher for _, conf := range c { enabled = enabled || conf.Enabled resourceMatchers = append(resourceMatchers, ResourceMatchers(conf.ResourceMatchers).AsResourceMatchers()) } - return + return enabled, resourceMatchers } func (c Conf) StripNameHashSuffixConfigs() StripNameHashSuffixConfigs { From e5cfb41784d42326d22400f46483810dadd5c8fc Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Thu, 22 Sep 2022 13:16:03 +0200 Subject: [PATCH 45/52] Keep logic within one method --- pkg/kapp/diff/change_set_with_versioned_rs.go | 96 +++++++++---------- 1 file changed, 47 insertions(+), 49 deletions(-) diff --git a/pkg/kapp/diff/change_set_with_versioned_rs.go b/pkg/kapp/diff/change_set_with_versioned_rs.go index da15696fc..e0e00b7ff 100644 --- a/pkg/kapp/diff/change_set_with_versioned_rs.go +++ b/pkg/kapp/diff/change_set_with_versioned_rs.go @@ -274,6 +274,14 @@ type versionedResources struct { NonVersioned []ctlres.Resource } +func (d *versionedResources) AddVersionedRes(vr VersionedResource) { + d.Versioned = append(d.Versioned, vr) +} + +func (d *versionedResources) AddNonVersionedRes(res ctlres.Resource) { + d.NonVersioned = append(d.NonVersioned, res) +} + func (d versionedResources) VersionedRs() (rs []ctlres.Resource) { for _, vres := range d.Versioned { rs = append(rs, vres.Res()) @@ -287,70 +295,60 @@ func (d versionedResources) NonVersionedRs() []ctlres.Resource { func (d ChangeSetWithVersionedRs) newVersionedResources() versionedResources { var result versionedResources - for _, res := range d.newRs { - result.appendRes(d.VersionedFromNewResource(res)) - } - return result -} -func (d ChangeSetWithVersionedRs) existingVersionedResources() versionedResources { - var result versionedResources - for _, res := range d.existingRs { - result.appendRes(d.VersionedFromExistingResource(res)) - } - return result -} - -func (d *versionedResources) appendRes(ver VersionedResource, nonVer ctlres.Resource) { - if ver != nil { - d.Versioned = append(d.Versioned, ver) - } - if nonVer != nil { - d.NonVersioned = append(d.NonVersioned, nonVer) - } -} + for _, res := range d.newRs { -func (d ChangeSetWithVersionedRs) VersionedFromNewResource(res ctlres.Resource) (versioned VersionedResource, nonVersioned ctlres.Resource) { + _, hasVersionedAnn := res.Annotations()[versionedResAnnKey] + _, hasVersionedOrigAnn := res.Annotations()[versionedResOrigAnnKey] - _, hasVersionedAnn := res.Annotations()[versionedResAnnKey] - _, hasVersionedOrigAnn := res.Annotations()[versionedResOrigAnnKey] + if hasVersionedAnn { + result.AddVersionedRes(VersionedResourceImpl{res, d.rules}) + if hasVersionedOrigAnn { + result.AddNonVersionedRes(res.DeepCopy()) + } + continue + } - if hasVersionedAnn { - versioned = VersionedResourceImpl{res, d.rules} - if hasVersionedOrigAnn { - nonVersioned = res.DeepCopy() + if d.stripNameHashSuffixConfig.EnabledFor(res) { + result.AddVersionedRes(HashSuffixResource{res}) + continue } - return - } - if d.stripNameHashSuffixConfig.EnabledFor(res) { - versioned = HashSuffixResource{res} - return + result.AddNonVersionedRes(res) } - return nil, res + return result } -func (d ChangeSetWithVersionedRs) VersionedFromExistingResource(res ctlres.Resource) (versioned VersionedResource, nonVersioned ctlres.Resource) { +func (d ChangeSetWithVersionedRs) existingVersionedResources() versionedResources { + var result versionedResources + + for _, res := range d.existingRs { - // Expect that versioned resources should not be transient - // (Annotations may have been copied from versioned resources - // onto transient resources for non-versioning related purposes). - if !res.Transient() { + // Expect that versioned resources should not be transient + // (Annotations may have been copied from versioned resources + // onto transient resources for non-versioning related purposes). + if !res.Transient() { + + _, hasVersionedAnn := res.Annotations()[versionedResAnnKey] + if hasVersionedAnn { + versionedRes := VersionedResourceImpl{res, d.rules} + _, version := versionedRes.BaseNameAndVersion() + if version != "" { + result.AddVersionedRes(versionedRes) + continue + } + } - _, hasVersionedAnn := res.Annotations()[versionedResAnnKey] - if hasVersionedAnn { - versionedRes := VersionedResourceImpl{res, d.rules} - _, version := versionedRes.BaseNameAndVersion() - if version != "" { - return versionedRes, nil + if d.stripNameHashSuffixConfig.EnabledFor(res) { + result.AddVersionedRes(HashSuffixResource{res}) + continue } - } - if d.stripNameHashSuffixConfig.EnabledFor(res) { - return HashSuffixResource{res}, nil } + + result.AddNonVersionedRes(res) } - return nil, res + return result } From be8bf30960b4d2cef7838eedc0ccf7776eb2d3a6 Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Tue, 27 Sep 2022 09:44:18 +0200 Subject: [PATCH 46/52] Remove disabled assert the operation is "add" because we're only manipulating the "add" diff to show the difference to the previous version --- pkg/kapp/diff/change_set_with_versioned_rs_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/pkg/kapp/diff/change_set_with_versioned_rs_test.go b/pkg/kapp/diff/change_set_with_versioned_rs_test.go index 60329c6c1..b51a3c6f5 100644 --- a/pkg/kapp/diff/change_set_with_versioned_rs_test.go +++ b/pkg/kapp/diff/change_set_with_versioned_rs_test.go @@ -182,8 +182,6 @@ data: changes, err := changeSetWithVerRes.Calculate() require.NoError(t, err) - //require.Equal(t, ChangeOpUpdate, changes[0].Op(), "Expect to get updated") // TODO: check why this is add, not update - expectedDiff := ` 0, 0 apiVersion: v1 1, 1 data: 2, 2 - foo: foo From ba249a73fbce2f94c7e34bdf355d9e9eba0eb64d Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Thu, 29 Sep 2022 10:59:29 +0200 Subject: [PATCH 47/52] enabling suffix strip by matcher presence preparation for dropping the enabled flag altogether; I have tried both at once before and the matchers completely broke, so doing the complex part (no matchers matching nothing) first with only *toggling* the default to enabled instead of directly throwing it out. --- pkg/kapp/config/default.go | 5 +-- pkg/kapp/diff/stripnamehashsuffix.go | 17 +++++-- pkg/kapp/diff/stripnamehashsuffix_test.go | 45 ++++++++++++++++++- pkg/kapp/resources/matchers.go | 3 ++ .../components/kapp-config/kapp-cm.yml | 7 ++- .../kustomize/overlays/versioned1/kapp.yml | 11 ++++- .../kustomize/overlays/versioned2/kapp.yml | 11 ++++- .../kustomize/overlays/versioned3/kapp.yml | 11 ++++- 8 files changed, 98 insertions(+), 12 deletions(-) diff --git a/pkg/kapp/config/default.go b/pkg/kapp/config/default.go index 4e46da13c..79b3f984c 100644 --- a/pkg/kapp/config/default.go +++ b/pkg/kapp/config/default.go @@ -645,10 +645,7 @@ changeRuleBindings: - hasNamespaceMatcher: {} StripNameHashSuffix: - enabled: false - resourceMatchers: - - apiVersionKindMatcher: {kind: ConfigMap, apiVersion: v1} - - apiVersionKindMatcher: {kind: Secret, apiVersion: v1} + enabled: true ` var defaultConfigRes = ctlres.MustNewResourceFromBytes([]byte(defaultConfigYAML)) diff --git a/pkg/kapp/diff/stripnamehashsuffix.go b/pkg/kapp/diff/stripnamehashsuffix.go index 86e8bb664..5fdf97462 100644 --- a/pkg/kapp/diff/stripnamehashsuffix.go +++ b/pkg/kapp/diff/stripnamehashsuffix.go @@ -27,10 +27,21 @@ func newStripNameHashSuffixConfig(enabled bool, resourceMatchers [][]ctlres.Reso if enabled { var allMatchers []ctlres.ResourceMatcher for _, matchers := range resourceMatchers { - allMatchers = append(allMatchers, ctlres.AndMatcher{ - Matchers: matchers, - }) + // configurations that do not specify any matchers (like default + // config) have nil, which would lead AndMatcher to never match. + // as all the configs AndMatchers are ANDed, too, this would prevent + // matchers from any configuration to match, which would strip the + // whole suffix-strip configuration of any usefulness; as such we + // exclude nil matchers. :) + if matchers != nil { + allMatchers = append(allMatchers, ctlres.AndMatcher{ + Matchers: matchers, + }) + } } + // N.B. if no config specifies any matchers (the default), then + // allMatchers will stay uninitialized/nil and the AndMatcher will never + // match, effectively disabling suffix strip. result.ResourceMatcher = ctlres.AndMatcher{Matchers: allMatchers} } return result diff --git a/pkg/kapp/diff/stripnamehashsuffix_test.go b/pkg/kapp/diff/stripnamehashsuffix_test.go index 12504812c..a7eb97251 100644 --- a/pkg/kapp/diff/stripnamehashsuffix_test.go +++ b/pkg/kapp/diff/stripnamehashsuffix_test.go @@ -10,7 +10,6 @@ import ( ctlres "github.com/vmware-tanzu/carvel-kapp/pkg/kapp/resources" ) -// test that we can exclude specific configsmaps from default func TestStripNameHashSuffix_TestConfig_IncludeExcludeAccrossConfigs(t *testing.T) { requireStripNameHashSuffixMatches(t, [][]ctlres.ResourceMatcher{ []ctlres.ResourceMatcher{ @@ -45,6 +44,50 @@ func TestStripNameHashSuffix_TestConfig_IncludeExcludeSingleConfig(t *testing.T) }) } +func TestStripNameHashSuffix_TestConfig_DefaultInclude(t *testing.T) { + requireStripNameHashSuffixMatches(t, [][]ctlres.ResourceMatcher{ + nil, + []ctlres.ResourceMatcher{ + ctlres.AnyMatcher{ + Matchers: []ctlres.ResourceMatcher{ + ctlres.APIVersionKindMatcher{APIVersion: "v1", Kind: "ConfigMap"}, + ctlres.APIVersionKindMatcher{APIVersion: "v2", Kind: "MyKind"}, + }, + }, + }, + []ctlres.ResourceMatcher{ + ctlres.NotMatcher{ + Matcher: ctlres.KindNamespaceNameMatcher{Kind: "ConfigMap", Name: "foo"}, + }, + }, + }) +} + +func TestStripNameHashSuffix_TestConfig_DefaultNone(t *testing.T) { + excludedCM := ctlres.MustNewResourceFromBytes([]byte(` +apiVersion: v1 +kind: ConfigMap +metadata: + name: foo +data: + foo: foo +`)) + + excludedKind := ctlres.MustNewResourceFromBytes([]byte(` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: bar +spec: + replicas: 1 +`)) + + config := newStripNameHashSuffixConfig(true, [][]ctlres.ResourceMatcher{nil}) + + require.False(t, config.EnabledFor(excludedCM), "expected not matching anything (here: ConfigMap) by default!") + require.False(t, config.EnabledFor(excludedKind), "expected not matching anything (here: Deployment) by default!") +} + func requireStripNameHashSuffixMatches(t *testing.T, matchers [][]ctlres.ResourceMatcher) { includedCM := ctlres.MustNewResourceFromBytes([]byte(` apiVersion: v1 diff --git a/pkg/kapp/resources/matchers.go b/pkg/kapp/resources/matchers.go index 28b31124e..9f8049ab2 100644 --- a/pkg/kapp/resources/matchers.go +++ b/pkg/kapp/resources/matchers.go @@ -77,6 +77,9 @@ type AndMatcher struct { var _ ResourceMatcher = AndMatcher{} func (m AndMatcher) Matches(res Resource) bool { + if m.Matchers == nil { + return false + } for _, m := range m.Matchers { if !m.Matches(res) { return false diff --git a/test/e2e/res/kustomize/components/kapp-config/kapp-cm.yml b/test/e2e/res/kustomize/components/kapp-config/kapp-cm.yml index a49de3ac6..0d89b0ea8 100644 --- a/test/e2e/res/kustomize/components/kapp-config/kapp-cm.yml +++ b/test/e2e/res/kustomize/components/kapp-config/kapp-cm.yml @@ -4,4 +4,9 @@ metadata: name: irrelevant StripNameHashSuffixConfig: - Enabled: true + enabled: true + resourceMatchers: + - anyMatcher: + matchers: + - apiVersionKindMatcher: {kind: ConfigMap, apiVersion: v1} + - apiVersionKindMatcher: {kind: Secret, apiVersion: v1} diff --git a/test/e2e/res/kustomize/overlays/versioned1/kapp.yml b/test/e2e/res/kustomize/overlays/versioned1/kapp.yml index 462f20cd5..7ef7c14e1 100644 --- a/test/e2e/res/kustomize/overlays/versioned1/kapp.yml +++ b/test/e2e/res/kustomize/overlays/versioned1/kapp.yml @@ -6,7 +6,16 @@ metadata: name: config-map-7tgk8db28b --- StripNameHashSuffixConfig: - Enabled: true + enabled: true + resourceMatchers: + - anyMatcher: + matchers: + - apiVersionKindMatcher: + apiVersion: v1 + kind: ConfigMap + - apiVersionKindMatcher: + apiVersion: v1 + kind: Secret apiVersion: kapp.k14s.io/v1alpha1 kind: Config metadata: diff --git a/test/e2e/res/kustomize/overlays/versioned2/kapp.yml b/test/e2e/res/kustomize/overlays/versioned2/kapp.yml index b68c21e5c..880fc1a6b 100644 --- a/test/e2e/res/kustomize/overlays/versioned2/kapp.yml +++ b/test/e2e/res/kustomize/overlays/versioned2/kapp.yml @@ -6,7 +6,16 @@ metadata: name: config-map-798k5k7g9f --- StripNameHashSuffixConfig: - Enabled: true + enabled: true + resourceMatchers: + - anyMatcher: + matchers: + - apiVersionKindMatcher: + apiVersion: v1 + kind: ConfigMap + - apiVersionKindMatcher: + apiVersion: v1 + kind: Secret apiVersion: kapp.k14s.io/v1alpha1 kind: Config metadata: diff --git a/test/e2e/res/kustomize/overlays/versioned3/kapp.yml b/test/e2e/res/kustomize/overlays/versioned3/kapp.yml index 353bf216d..6a19cc9f7 100644 --- a/test/e2e/res/kustomize/overlays/versioned3/kapp.yml +++ b/test/e2e/res/kustomize/overlays/versioned3/kapp.yml @@ -6,7 +6,16 @@ metadata: name: config-map-5bghbfbdc8 --- StripNameHashSuffixConfig: - Enabled: true + enabled: true + resourceMatchers: + - anyMatcher: + matchers: + - apiVersionKindMatcher: + apiVersion: v1 + kind: ConfigMap + - apiVersionKindMatcher: + apiVersion: v1 + kind: Secret apiVersion: kapp.k14s.io/v1alpha1 kind: Config metadata: From 4f51583928e0bff2a91e928f12d0e5fdce0a90b1 Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Thu, 29 Sep 2022 11:25:13 +0200 Subject: [PATCH 48/52] Drop suffix-strip enable flag it seems the field in config does not need to be an object anymore, could as well be a (by default nil) list of matchers, but let's not touch too much at once. --- pkg/kapp/config/conf.go | 6 +-- pkg/kapp/config/config.go | 1 - pkg/kapp/config/default.go | 3 +- .../diff/change_set_with_versioned_rs_test.go | 4 +- pkg/kapp/diff/stripnamehashsuffix.go | 50 +++++++++---------- pkg/kapp/diff/stripnamehashsuffix_test.go | 4 +- 6 files changed, 30 insertions(+), 38 deletions(-) diff --git a/pkg/kapp/config/conf.go b/pkg/kapp/config/conf.go index bb6c8f9ab..64c37c7ba 100644 --- a/pkg/kapp/config/conf.go +++ b/pkg/kapp/config/conf.go @@ -181,14 +181,12 @@ func (c Conf) ChangeRuleBindings() []ChangeRuleBinding { type StripNameHashSuffixConfigs []StripNameHashSuffixConfig -func (c StripNameHashSuffixConfigs) AggregateToCtlRes() (bool, [][]ctlres.ResourceMatcher) { - var enabled bool +func (c StripNameHashSuffixConfigs) AggregateToCtlRes() [][]ctlres.ResourceMatcher { var resourceMatchers [][]ctlres.ResourceMatcher for _, conf := range c { - enabled = enabled || conf.Enabled resourceMatchers = append(resourceMatchers, ResourceMatchers(conf.ResourceMatchers).AsResourceMatchers()) } - return enabled, resourceMatchers + return resourceMatchers } func (c Conf) StripNameHashSuffixConfigs() StripNameHashSuffixConfigs { diff --git a/pkg/kapp/config/config.go b/pkg/kapp/config/config.go index 11f50c130..416d5fb4e 100644 --- a/pkg/kapp/config/config.go +++ b/pkg/kapp/config/config.go @@ -142,7 +142,6 @@ type ChangeRuleBinding struct { } type StripNameHashSuffixConfig struct { - Enabled bool ResourceMatchers []ResourceMatcher } diff --git a/pkg/kapp/config/default.go b/pkg/kapp/config/default.go index 79b3f984c..f14f89f36 100644 --- a/pkg/kapp/config/default.go +++ b/pkg/kapp/config/default.go @@ -644,8 +644,7 @@ changeRuleBindings: - anyMatcher: {matchers: *podRelatedMatchers} - hasNamespaceMatcher: {} -StripNameHashSuffix: - enabled: true +StripNameHashSuffix: {} ` var defaultConfigRes = ctlres.MustNewResourceFromBytes([]byte(defaultConfigYAML)) diff --git a/pkg/kapp/diff/change_set_with_versioned_rs_test.go b/pkg/kapp/diff/change_set_with_versioned_rs_test.go index b51a3c6f5..1b8627b7f 100644 --- a/pkg/kapp/diff/change_set_with_versioned_rs_test.go +++ b/pkg/kapp/diff/change_set_with_versioned_rs_test.go @@ -176,7 +176,7 @@ data: foo: bar `)) - stripNameHashSuffix := stripNameHashSuffixConfig{Enabled: true, ResourceMatcher: ctlres.AllMatcher{}} + stripNameHashSuffix := stripNameHashSuffixConfig{ResourceMatcher: ctlres.AllMatcher{}} changeSetWithVerRes := ChangeSetWithVersionedRs{[]ctlres.Resource{existingRs}, []ctlres.Resource{newRs}, nil, ChangeSetOpts{}, ChangeFactory{}, stripNameHashSuffix} changes, err := changeSetWithVerRes.Calculate() @@ -198,7 +198,7 @@ data: } func newChangeSet(existingRes, newRes ctlres.Resource) *ChangeSetWithVersionedRs { - stripNameHashSuffix := stripNameHashSuffixConfig{Enabled: false} + stripNameHashSuffix := stripNameHashSuffixConfig{nil} return &ChangeSetWithVersionedRs{[]ctlres.Resource{existingRes}, []ctlres.Resource{newRes}, nil, ChangeSetOpts{}, ChangeFactory{}, stripNameHashSuffix} } diff --git a/pkg/kapp/diff/stripnamehashsuffix.go b/pkg/kapp/diff/stripnamehashsuffix.go index 5fdf97462..3c0b4f245 100644 --- a/pkg/kapp/diff/stripnamehashsuffix.go +++ b/pkg/kapp/diff/stripnamehashsuffix.go @@ -11,45 +11,41 @@ import ( ) type stripNameHashSuffixConfig struct { - Enabled bool ResourceMatcher ctlres.ResourceMatcher } func (d stripNameHashSuffixConfig) EnabledFor(res ctlres.Resource) (stripEnabled bool) { - if d.Enabled { - return d.ResourceMatcher.Matches(res) - } - return false + return d.ResourceMatcher.Matches(res) } -func newStripNameHashSuffixConfig(enabled bool, resourceMatchers [][]ctlres.ResourceMatcher) (result stripNameHashSuffixConfig) { - result = stripNameHashSuffixConfig{Enabled: enabled} - if enabled { - var allMatchers []ctlres.ResourceMatcher - for _, matchers := range resourceMatchers { - // configurations that do not specify any matchers (like default - // config) have nil, which would lead AndMatcher to never match. - // as all the configs AndMatchers are ANDed, too, this would prevent - // matchers from any configuration to match, which would strip the - // whole suffix-strip configuration of any usefulness; as such we - // exclude nil matchers. :) - if matchers != nil { - allMatchers = append(allMatchers, ctlres.AndMatcher{ - Matchers: matchers, - }) - } +func newStripNameHashSuffixConfig(resourceMatchers [][]ctlres.ResourceMatcher) (result stripNameHashSuffixConfig) { + result = stripNameHashSuffixConfig{} + + var allMatchers []ctlres.ResourceMatcher + for _, matchers := range resourceMatchers { + // configurations that do not specify any matchers (like default + // config) have nil, which would lead AndMatcher to never match. + // as all the configs AndMatchers are ANDed, too, this would prevent + // matchers from any configuration to match, which would strip the + // whole suffix-strip configuration of any usefulness; as such we + // exclude nil matchers. :) + if matchers != nil { + allMatchers = append(allMatchers, ctlres.AndMatcher{ + Matchers: matchers, + }) } - // N.B. if no config specifies any matchers (the default), then - // allMatchers will stay uninitialized/nil and the AndMatcher will never - // match, effectively disabling suffix strip. - result.ResourceMatcher = ctlres.AndMatcher{Matchers: allMatchers} } + // N.B. if no config specifies any matchers (the default), then + // allMatchers will stay uninitialized/nil and the AndMatcher will never + // match, effectively disabling suffix strip. + result.ResourceMatcher = ctlres.AndMatcher{Matchers: allMatchers} + return result } func newStripNameHashSuffixConfigFromConf(confs ctlconf.StripNameHashSuffixConfigs) stripNameHashSuffixConfig { - enabled, matchers := confs.AggregateToCtlRes() - return newStripNameHashSuffixConfig(enabled, matchers) + matchers := confs.AggregateToCtlRes() + return newStripNameHashSuffixConfig(matchers) } type HashSuffixResource struct { diff --git a/pkg/kapp/diff/stripnamehashsuffix_test.go b/pkg/kapp/diff/stripnamehashsuffix_test.go index a7eb97251..870baec66 100644 --- a/pkg/kapp/diff/stripnamehashsuffix_test.go +++ b/pkg/kapp/diff/stripnamehashsuffix_test.go @@ -82,7 +82,7 @@ spec: replicas: 1 `)) - config := newStripNameHashSuffixConfig(true, [][]ctlres.ResourceMatcher{nil}) + config := newStripNameHashSuffixConfig([][]ctlres.ResourceMatcher{nil}) require.False(t, config.EnabledFor(excludedCM), "expected not matching anything (here: ConfigMap) by default!") require.False(t, config.EnabledFor(excludedKind), "expected not matching anything (here: Deployment) by default!") @@ -125,7 +125,7 @@ spec: replicas: 1 `)) - config := newStripNameHashSuffixConfig(true, matchers) + config := newStripNameHashSuffixConfig(matchers) require.True(t, config.EnabledFor(includedCM), "expected included ConfigMap to match!") require.True(t, config.EnabledFor(includedKind), "expected included Kind to match!") From aaca09749579db656676547a347078f4a2d13f96 Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Thu, 29 Sep 2022 12:37:25 +0200 Subject: [PATCH 49/52] Fix test --- pkg/kapp/diff/change_set_with_versioned_rs_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/kapp/diff/change_set_with_versioned_rs_test.go b/pkg/kapp/diff/change_set_with_versioned_rs_test.go index 1b8627b7f..09a0b4a9f 100644 --- a/pkg/kapp/diff/change_set_with_versioned_rs_test.go +++ b/pkg/kapp/diff/change_set_with_versioned_rs_test.go @@ -198,7 +198,7 @@ data: } func newChangeSet(existingRes, newRes ctlres.Resource) *ChangeSetWithVersionedRs { - stripNameHashSuffix := stripNameHashSuffixConfig{nil} + stripNameHashSuffix := newStripNameHashSuffixConfig(nil) return &ChangeSetWithVersionedRs{[]ctlres.Resource{existingRes}, []ctlres.Resource{newRes}, nil, ChangeSetOpts{}, ChangeFactory{}, stripNameHashSuffix} } From 76991f6f1107eb0b68839d812070a31075e9b2fe Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Sun, 16 Oct 2022 22:18:17 +0200 Subject: [PATCH 50/52] replace left-over naked return --- pkg/kapp/diff/change_set_with_versioned_rs.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/kapp/diff/change_set_with_versioned_rs.go b/pkg/kapp/diff/change_set_with_versioned_rs.go index e0e00b7ff..b52f078a7 100644 --- a/pkg/kapp/diff/change_set_with_versioned_rs.go +++ b/pkg/kapp/diff/change_set_with_versioned_rs.go @@ -282,11 +282,12 @@ func (d *versionedResources) AddNonVersionedRes(res ctlres.Resource) { d.NonVersioned = append(d.NonVersioned, res) } -func (d versionedResources) VersionedRs() (rs []ctlres.Resource) { +func (d versionedResources) VersionedRs() []ctlres.Resource { + var rs []ctlres.Resource for _, vres := range d.Versioned { rs = append(rs, vres.Res()) } - return + return rs } func (d versionedResources) NonVersionedRs() []ctlres.Resource { From 8f7848ad648b92ad11bd68fc2af67868e591da5f Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Sun, 16 Oct 2022 21:02:06 +0200 Subject: [PATCH 51/52] Streamline suffix strip config --- pkg/kapp/config/conf.go | 18 ++--- pkg/kapp/config/config.go | 13 +++- pkg/kapp/config/default.go | 2 - pkg/kapp/diff/change_set_with_versioned_rs.go | 2 +- .../diff/change_set_with_versioned_rs_test.go | 2 +- pkg/kapp/diff/stripnamehashsuffix.go | 43 +++++------- pkg/kapp/diff/stripnamehashsuffix_test.go | 66 ++++++++----------- .../components/kapp-config/kapp-cm.yml | 10 +-- .../kustomize/overlays/versioned1/kapp.yml | 16 ++--- .../kustomize/overlays/versioned2/kapp.yml | 16 ++--- .../kustomize/overlays/versioned3/kapp.yml | 16 ++--- 11 files changed, 81 insertions(+), 123 deletions(-) diff --git a/pkg/kapp/config/conf.go b/pkg/kapp/config/conf.go index 64c37c7ba..61fe54e35 100644 --- a/pkg/kapp/config/conf.go +++ b/pkg/kapp/config/conf.go @@ -179,20 +179,10 @@ func (c Conf) ChangeRuleBindings() []ChangeRuleBinding { return result } -type StripNameHashSuffixConfigs []StripNameHashSuffixConfig - -func (c StripNameHashSuffixConfigs) AggregateToCtlRes() [][]ctlres.ResourceMatcher { - var resourceMatchers [][]ctlres.ResourceMatcher - for _, conf := range c { - resourceMatchers = append(resourceMatchers, ResourceMatchers(conf.ResourceMatchers).AsResourceMatchers()) - } - return resourceMatchers -} - -func (c Conf) StripNameHashSuffixConfigs() StripNameHashSuffixConfigs { - var configs []StripNameHashSuffixConfig +func (c Conf) StripNameHashSuffixConfigs() []StripNameHashSuffixConfig { + var result []StripNameHashSuffixConfig for _, config := range c.configs { - configs = append(configs, config.StripNameHashSuffixConfig) + result = append(result, config.StripNameHashSuffixConfigs...) } - return StripNameHashSuffixConfigs(configs) + return result } diff --git a/pkg/kapp/config/config.go b/pkg/kapp/config/config.go index 416d5fb4e..44da2564e 100644 --- a/pkg/kapp/config/config.go +++ b/pkg/kapp/config/config.go @@ -39,7 +39,7 @@ type Config struct { ChangeGroupBindings []ChangeGroupBinding ChangeRuleBindings []ChangeRuleBinding - StripNameHashSuffixConfig StripNameHashSuffixConfig + StripNameHashSuffixConfigs []StripNameHashSuffixConfig } type WaitRule struct { @@ -142,7 +142,8 @@ type ChangeRuleBinding struct { } type StripNameHashSuffixConfig struct { - ResourceMatchers []ResourceMatcher + Includes []ResourceMatcher + Excludes []ResourceMatcher } func NewConfigFromResource(res ctlres.Resource) (Config, error) { @@ -320,3 +321,11 @@ func (r WaitRule) ResourceMatcher() ctlres.ResourceMatcher { Matchers: ResourceMatchers(r.ResourceMatchers).AsResourceMatchers(), } } + +func (r StripNameHashSuffixConfig) IncludeMatchers() []ctlres.ResourceMatcher { + return ResourceMatchers(r.Includes).AsResourceMatchers() +} + +func (r StripNameHashSuffixConfig) ExcludeMatchers() []ctlres.ResourceMatcher { + return ResourceMatchers(r.Excludes).AsResourceMatchers() +} diff --git a/pkg/kapp/config/default.go b/pkg/kapp/config/default.go index f14f89f36..7330204ce 100644 --- a/pkg/kapp/config/default.go +++ b/pkg/kapp/config/default.go @@ -643,8 +643,6 @@ changeRuleBindings: - anyMatcher: {matchers: *rbacMatchers} - anyMatcher: {matchers: *podRelatedMatchers} - hasNamespaceMatcher: {} - -StripNameHashSuffix: {} ` var defaultConfigRes = ctlres.MustNewResourceFromBytes([]byte(defaultConfigYAML)) diff --git a/pkg/kapp/diff/change_set_with_versioned_rs.go b/pkg/kapp/diff/change_set_with_versioned_rs.go index b52f078a7..e08c262f2 100644 --- a/pkg/kapp/diff/change_set_with_versioned_rs.go +++ b/pkg/kapp/diff/change_set_with_versioned_rs.go @@ -27,7 +27,7 @@ type ChangeSetWithVersionedRs struct { } func NewChangeSetWithVersionedRs(existingRs, newRs []ctlres.Resource, - rules []ctlconf.TemplateRule, opts ChangeSetOpts, changeFactory ChangeFactory, stripNameHashSuffixConfigs ctlconf.StripNameHashSuffixConfigs) *ChangeSetWithVersionedRs { + rules []ctlconf.TemplateRule, opts ChangeSetOpts, changeFactory ChangeFactory, stripNameHashSuffixConfigs []ctlconf.StripNameHashSuffixConfig) *ChangeSetWithVersionedRs { return &ChangeSetWithVersionedRs{existingRs, newRs, rules, opts, changeFactory, newStripNameHashSuffixConfigFromConf(stripNameHashSuffixConfigs)} } diff --git a/pkg/kapp/diff/change_set_with_versioned_rs_test.go b/pkg/kapp/diff/change_set_with_versioned_rs_test.go index 09a0b4a9f..41bf9887f 100644 --- a/pkg/kapp/diff/change_set_with_versioned_rs_test.go +++ b/pkg/kapp/diff/change_set_with_versioned_rs_test.go @@ -198,7 +198,7 @@ data: } func newChangeSet(existingRes, newRes ctlres.Resource) *ChangeSetWithVersionedRs { - stripNameHashSuffix := newStripNameHashSuffixConfig(nil) + stripNameHashSuffix := newStripNameHashSuffixConfigEmpty() return &ChangeSetWithVersionedRs{[]ctlres.Resource{existingRes}, []ctlres.Resource{newRes}, nil, ChangeSetOpts{}, ChangeFactory{}, stripNameHashSuffix} } diff --git a/pkg/kapp/diff/stripnamehashsuffix.go b/pkg/kapp/diff/stripnamehashsuffix.go index 3c0b4f245..75615ab84 100644 --- a/pkg/kapp/diff/stripnamehashsuffix.go +++ b/pkg/kapp/diff/stripnamehashsuffix.go @@ -18,34 +18,27 @@ func (d stripNameHashSuffixConfig) EnabledFor(res ctlres.Resource) (stripEnabled return d.ResourceMatcher.Matches(res) } -func newStripNameHashSuffixConfig(resourceMatchers [][]ctlres.ResourceMatcher) (result stripNameHashSuffixConfig) { - result = stripNameHashSuffixConfig{} - - var allMatchers []ctlres.ResourceMatcher - for _, matchers := range resourceMatchers { - // configurations that do not specify any matchers (like default - // config) have nil, which would lead AndMatcher to never match. - // as all the configs AndMatchers are ANDed, too, this would prevent - // matchers from any configuration to match, which would strip the - // whole suffix-strip configuration of any usefulness; as such we - // exclude nil matchers. :) - if matchers != nil { - allMatchers = append(allMatchers, ctlres.AndMatcher{ - Matchers: matchers, - }) - } +func newStripNameHashSuffixConfigFromConf(confs []ctlconf.StripNameHashSuffixConfig) stripNameHashSuffixConfig { + includeMatchers := []ctlres.ResourceMatcher{} + excludeMatchers := []ctlres.ResourceMatcher{} + for _, conf := range confs { + includeMatchers = append(includeMatchers, conf.IncludeMatchers()...) + excludeMatchers = append(excludeMatchers, conf.ExcludeMatchers()...) + } + return stripNameHashSuffixConfig{ + ResourceMatcher: ctlres.AndMatcher{ + Matchers: []ctlres.ResourceMatcher{ + ctlres.AnyMatcher{Matchers: includeMatchers}, + ctlres.NotMatcher{ + Matcher: ctlres.AnyMatcher{Matchers: excludeMatchers}, + }, + }, + }, } - // N.B. if no config specifies any matchers (the default), then - // allMatchers will stay uninitialized/nil and the AndMatcher will never - // match, effectively disabling suffix strip. - result.ResourceMatcher = ctlres.AndMatcher{Matchers: allMatchers} - - return result } -func newStripNameHashSuffixConfigFromConf(confs ctlconf.StripNameHashSuffixConfigs) stripNameHashSuffixConfig { - matchers := confs.AggregateToCtlRes() - return newStripNameHashSuffixConfig(matchers) +func newStripNameHashSuffixConfigEmpty() stripNameHashSuffixConfig { + return newStripNameHashSuffixConfigFromConf([]ctlconf.StripNameHashSuffixConfig{}) } type HashSuffixResource struct { diff --git a/pkg/kapp/diff/stripnamehashsuffix_test.go b/pkg/kapp/diff/stripnamehashsuffix_test.go index 870baec66..20cb92d50 100644 --- a/pkg/kapp/diff/stripnamehashsuffix_test.go +++ b/pkg/kapp/diff/stripnamehashsuffix_test.go @@ -7,57 +7,47 @@ import ( "testing" "github.com/stretchr/testify/require" + ctlconf "github.com/vmware-tanzu/carvel-kapp/pkg/kapp/config" ctlres "github.com/vmware-tanzu/carvel-kapp/pkg/kapp/resources" ) func TestStripNameHashSuffix_TestConfig_IncludeExcludeAccrossConfigs(t *testing.T) { - requireStripNameHashSuffixMatches(t, [][]ctlres.ResourceMatcher{ - []ctlres.ResourceMatcher{ - ctlres.AnyMatcher{ - Matchers: []ctlres.ResourceMatcher{ - ctlres.APIVersionKindMatcher{APIVersion: "v1", Kind: "ConfigMap"}, - ctlres.APIVersionKindMatcher{APIVersion: "v2", Kind: "MyKind"}, + requireStripNameHashSuffixMatches(t, []ctlconf.StripNameHashSuffixConfig{ + ctlconf.StripNameHashSuffixConfig{ + Includes: []ctlconf.ResourceMatcher{ + ctlconf.ResourceMatcher{ + APIVersionKindMatcher: &ctlconf.APIVersionKindMatcher{APIVersion: "v1", Kind: "ConfigMap"}, + }, + ctlconf.ResourceMatcher{ + APIVersionKindMatcher: &ctlconf.APIVersionKindMatcher{APIVersion: "v2", Kind: "MyKind"}, }, }, }, - []ctlres.ResourceMatcher{ - ctlres.NotMatcher{ - Matcher: ctlres.KindNamespaceNameMatcher{Kind: "ConfigMap", Name: "foo"}, + ctlconf.StripNameHashSuffixConfig{ + Excludes: []ctlconf.ResourceMatcher{ + ctlconf.ResourceMatcher{ + KindNamespaceNameMatcher: &ctlconf.KindNamespaceNameMatcher{Kind: "ConfigMap", Name: "foo"}, + }, }, }, }) } func TestStripNameHashSuffix_TestConfig_IncludeExcludeSingleConfig(t *testing.T) { - requireStripNameHashSuffixMatches(t, [][]ctlres.ResourceMatcher{ - []ctlres.ResourceMatcher{ - ctlres.AnyMatcher{ - Matchers: []ctlres.ResourceMatcher{ - ctlres.APIVersionKindMatcher{APIVersion: "v1", Kind: "ConfigMap"}, - ctlres.APIVersionKindMatcher{APIVersion: "v2", Kind: "MyKind"}, + requireStripNameHashSuffixMatches(t, []ctlconf.StripNameHashSuffixConfig{ + ctlconf.StripNameHashSuffixConfig{ + Includes: []ctlconf.ResourceMatcher{ + ctlconf.ResourceMatcher{ + APIVersionKindMatcher: &ctlconf.APIVersionKindMatcher{APIVersion: "v1", Kind: "ConfigMap"}, }, - }, - ctlres.NotMatcher{ - Matcher: ctlres.KindNamespaceNameMatcher{Kind: "ConfigMap", Name: "foo"}, - }, - }, - }) -} - -func TestStripNameHashSuffix_TestConfig_DefaultInclude(t *testing.T) { - requireStripNameHashSuffixMatches(t, [][]ctlres.ResourceMatcher{ - nil, - []ctlres.ResourceMatcher{ - ctlres.AnyMatcher{ - Matchers: []ctlres.ResourceMatcher{ - ctlres.APIVersionKindMatcher{APIVersion: "v1", Kind: "ConfigMap"}, - ctlres.APIVersionKindMatcher{APIVersion: "v2", Kind: "MyKind"}, + ctlconf.ResourceMatcher{ + APIVersionKindMatcher: &ctlconf.APIVersionKindMatcher{APIVersion: "v2", Kind: "MyKind"}, }, }, - }, - []ctlres.ResourceMatcher{ - ctlres.NotMatcher{ - Matcher: ctlres.KindNamespaceNameMatcher{Kind: "ConfigMap", Name: "foo"}, + Excludes: []ctlconf.ResourceMatcher{ + ctlconf.ResourceMatcher{ + KindNamespaceNameMatcher: &ctlconf.KindNamespaceNameMatcher{Kind: "ConfigMap", Name: "foo"}, + }, }, }, }) @@ -82,13 +72,13 @@ spec: replicas: 1 `)) - config := newStripNameHashSuffixConfig([][]ctlres.ResourceMatcher{nil}) + config := newStripNameHashSuffixConfigEmpty() require.False(t, config.EnabledFor(excludedCM), "expected not matching anything (here: ConfigMap) by default!") require.False(t, config.EnabledFor(excludedKind), "expected not matching anything (here: Deployment) by default!") } -func requireStripNameHashSuffixMatches(t *testing.T, matchers [][]ctlres.ResourceMatcher) { +func requireStripNameHashSuffixMatches(t *testing.T, confs []ctlconf.StripNameHashSuffixConfig) { includedCM := ctlres.MustNewResourceFromBytes([]byte(` apiVersion: v1 kind: ConfigMap @@ -125,7 +115,7 @@ spec: replicas: 1 `)) - config := newStripNameHashSuffixConfig(matchers) + config := newStripNameHashSuffixConfigFromConf(confs) require.True(t, config.EnabledFor(includedCM), "expected included ConfigMap to match!") require.True(t, config.EnabledFor(includedKind), "expected included Kind to match!") diff --git a/test/e2e/res/kustomize/components/kapp-config/kapp-cm.yml b/test/e2e/res/kustomize/components/kapp-config/kapp-cm.yml index 0d89b0ea8..d30041ec8 100644 --- a/test/e2e/res/kustomize/components/kapp-config/kapp-cm.yml +++ b/test/e2e/res/kustomize/components/kapp-config/kapp-cm.yml @@ -3,10 +3,6 @@ kind: Config metadata: name: irrelevant -StripNameHashSuffixConfig: - enabled: true - resourceMatchers: - - anyMatcher: - matchers: - - apiVersionKindMatcher: {kind: ConfigMap, apiVersion: v1} - - apiVersionKindMatcher: {kind: Secret, apiVersion: v1} +stripNameHashSuffixConfigs: +- includes: + - apiVersionKindMatcher: {apiVersion: v1, kind: ConfigMap} diff --git a/test/e2e/res/kustomize/overlays/versioned1/kapp.yml b/test/e2e/res/kustomize/overlays/versioned1/kapp.yml index 7ef7c14e1..b13525143 100644 --- a/test/e2e/res/kustomize/overlays/versioned1/kapp.yml +++ b/test/e2e/res/kustomize/overlays/versioned1/kapp.yml @@ -5,18 +5,12 @@ kind: ConfigMap metadata: name: config-map-7tgk8db28b --- -StripNameHashSuffixConfig: - enabled: true - resourceMatchers: - - anyMatcher: - matchers: - - apiVersionKindMatcher: - apiVersion: v1 - kind: ConfigMap - - apiVersionKindMatcher: - apiVersion: v1 - kind: Secret apiVersion: kapp.k14s.io/v1alpha1 kind: Config metadata: name: irrelevant +stripNameHashSuffixConfigs: +- includes: + - apiVersionKindMatcher: + apiVersion: v1 + kind: ConfigMap diff --git a/test/e2e/res/kustomize/overlays/versioned2/kapp.yml b/test/e2e/res/kustomize/overlays/versioned2/kapp.yml index 880fc1a6b..d351ae197 100644 --- a/test/e2e/res/kustomize/overlays/versioned2/kapp.yml +++ b/test/e2e/res/kustomize/overlays/versioned2/kapp.yml @@ -5,18 +5,12 @@ kind: ConfigMap metadata: name: config-map-798k5k7g9f --- -StripNameHashSuffixConfig: - enabled: true - resourceMatchers: - - anyMatcher: - matchers: - - apiVersionKindMatcher: - apiVersion: v1 - kind: ConfigMap - - apiVersionKindMatcher: - apiVersion: v1 - kind: Secret apiVersion: kapp.k14s.io/v1alpha1 kind: Config metadata: name: irrelevant +stripNameHashSuffixConfigs: +- includes: + - apiVersionKindMatcher: + apiVersion: v1 + kind: ConfigMap diff --git a/test/e2e/res/kustomize/overlays/versioned3/kapp.yml b/test/e2e/res/kustomize/overlays/versioned3/kapp.yml index 6a19cc9f7..b6e9e87fd 100644 --- a/test/e2e/res/kustomize/overlays/versioned3/kapp.yml +++ b/test/e2e/res/kustomize/overlays/versioned3/kapp.yml @@ -5,18 +5,12 @@ kind: ConfigMap metadata: name: config-map-5bghbfbdc8 --- -StripNameHashSuffixConfig: - enabled: true - resourceMatchers: - - anyMatcher: - matchers: - - apiVersionKindMatcher: - apiVersion: v1 - kind: ConfigMap - - apiVersionKindMatcher: - apiVersion: v1 - kind: Secret apiVersion: kapp.k14s.io/v1alpha1 kind: Config metadata: name: irrelevant +stripNameHashSuffixConfigs: +- includes: + - apiVersionKindMatcher: + apiVersion: v1 + kind: ConfigMap From 40fa319b308c6c72107a62e06702cfd960d12555 Mon Sep 17 00:00:00 2001 From: "Christoph \"criztovyl\" Schulz" Date: Sun, 16 Oct 2022 21:15:51 +0200 Subject: [PATCH 52/52] Convert to tests to table tests --- pkg/kapp/diff/stripnamehashsuffix_test.go | 175 +++++++++++++-------- test/e2e/versioned_stripnamesuffix_test.go | 49 +++--- 2 files changed, 133 insertions(+), 91 deletions(-) diff --git a/pkg/kapp/diff/stripnamehashsuffix_test.go b/pkg/kapp/diff/stripnamehashsuffix_test.go index 20cb92d50..ff8303496 100644 --- a/pkg/kapp/diff/stripnamehashsuffix_test.go +++ b/pkg/kapp/diff/stripnamehashsuffix_test.go @@ -4,6 +4,7 @@ package diff import ( + "fmt" "testing" "github.com/stretchr/testify/require" @@ -11,79 +12,22 @@ import ( ctlres "github.com/vmware-tanzu/carvel-kapp/pkg/kapp/resources" ) -func TestStripNameHashSuffix_TestConfig_IncludeExcludeAccrossConfigs(t *testing.T) { - requireStripNameHashSuffixMatches(t, []ctlconf.StripNameHashSuffixConfig{ - ctlconf.StripNameHashSuffixConfig{ - Includes: []ctlconf.ResourceMatcher{ - ctlconf.ResourceMatcher{ - APIVersionKindMatcher: &ctlconf.APIVersionKindMatcher{APIVersion: "v1", Kind: "ConfigMap"}, - }, - ctlconf.ResourceMatcher{ - APIVersionKindMatcher: &ctlconf.APIVersionKindMatcher{APIVersion: "v2", Kind: "MyKind"}, - }, - }, - }, - ctlconf.StripNameHashSuffixConfig{ - Excludes: []ctlconf.ResourceMatcher{ - ctlconf.ResourceMatcher{ - KindNamespaceNameMatcher: &ctlconf.KindNamespaceNameMatcher{Kind: "ConfigMap", Name: "foo"}, - }, - }, - }, - }) -} - -func TestStripNameHashSuffix_TestConfig_IncludeExcludeSingleConfig(t *testing.T) { - requireStripNameHashSuffixMatches(t, []ctlconf.StripNameHashSuffixConfig{ - ctlconf.StripNameHashSuffixConfig{ - Includes: []ctlconf.ResourceMatcher{ - ctlconf.ResourceMatcher{ - APIVersionKindMatcher: &ctlconf.APIVersionKindMatcher{APIVersion: "v1", Kind: "ConfigMap"}, - }, - ctlconf.ResourceMatcher{ - APIVersionKindMatcher: &ctlconf.APIVersionKindMatcher{APIVersion: "v2", Kind: "MyKind"}, - }, - }, - Excludes: []ctlconf.ResourceMatcher{ - ctlconf.ResourceMatcher{ - KindNamespaceNameMatcher: &ctlconf.KindNamespaceNameMatcher{Kind: "ConfigMap", Name: "foo"}, - }, - }, - }, - }) -} +func TestStripNameHashSuffix_TestConfigIncludeExclude(t *testing.T) { -func TestStripNameHashSuffix_TestConfig_DefaultNone(t *testing.T) { - excludedCM := ctlres.MustNewResourceFromBytes([]byte(` + includedCM := ctlres.MustNewResourceFromBytes([]byte(` apiVersion: v1 kind: ConfigMap metadata: - name: foo + name: configmap-abc data: foo: foo `)) - excludedKind := ctlres.MustNewResourceFromBytes([]byte(` -apiVersion: apps/v1 -kind: Deployment -metadata: - name: bar -spec: - replicas: 1 -`)) - - config := newStripNameHashSuffixConfigEmpty() - - require.False(t, config.EnabledFor(excludedCM), "expected not matching anything (here: ConfigMap) by default!") - require.False(t, config.EnabledFor(excludedKind), "expected not matching anything (here: Deployment) by default!") -} - -func requireStripNameHashSuffixMatches(t *testing.T, confs []ctlconf.StripNameHashSuffixConfig) { - includedCM := ctlres.MustNewResourceFromBytes([]byte(` + excludedCM := ctlres.MustNewResourceFromBytes([]byte(` apiVersion: v1 kind: ConfigMap metadata: - name: configmap-abc + name: foo data: foo: foo `)) @@ -97,6 +41,91 @@ spec: key: val `)) + excludedKind := ctlres.MustNewResourceFromBytes([]byte(` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: bar +spec: + replicas: 1 +`)) + + includes := []ctlconf.ResourceMatcher{ + ctlconf.ResourceMatcher{ + APIVersionKindMatcher: &ctlconf.APIVersionKindMatcher{APIVersion: includedCM.APIVersion(), Kind: includedCM.Kind()}, + }, + ctlconf.ResourceMatcher{ + APIVersionKindMatcher: &ctlconf.APIVersionKindMatcher{APIVersion: includedKind.APIVersion(), Kind: includedKind.Kind()}, + }, + } + + excludes := []ctlconf.ResourceMatcher{ + ctlconf.ResourceMatcher{ + KindNamespaceNameMatcher: &ctlconf.KindNamespaceNameMatcher{Kind: excludedCM.Kind(), Name: excludedCM.Name()}, + }, + } + + configCases := []struct { + desc string + confs []ctlconf.StripNameHashSuffixConfig + }{ + { + "SingleConfig", + []ctlconf.StripNameHashSuffixConfig{ + ctlconf.StripNameHashSuffixConfig{ + Includes: includes, + Excludes: excludes, + }, + }, + }, + { + "AccrossConfigs", + []ctlconf.StripNameHashSuffixConfig{ + ctlconf.StripNameHashSuffixConfig{ + Includes: includes, + }, + ctlconf.StripNameHashSuffixConfig{ + Excludes: excludes, + }, + }, + }, + } + + for _, configTc := range configCases { + t.Run(configTc.desc, func(t *testing.T) { + + config := newStripNameHashSuffixConfigFromConf(configTc.confs) + + resCases := []struct { + kind string + res ctlres.Resource + expected bool + }{ + {"ConfigMap", includedCM, true}, + {"ConfigMap", excludedCM, false}, + {"Kind", includedKind, true}, + {"Kind", excludedKind, false}, + } + + for _, resTc := range resCases { + var inEx, not string + if resTc.expected { + inEx = "included" + not = "" + } else { + inEx = "excluded" + not = " not" + } + desc := fmt.Sprintf("%s_%s", resTc.kind, inEx) + t.Run(desc, func(t *testing.T) { + require.Equalf(t, resTc.expected, config.EnabledFor(resTc.res), "expected %s %s to%s match!", inEx, resTc.kind, not) + }) + } + }) + } +} + +func TestStripNameHashSuffix_TestConfig_DefaultNone(t *testing.T) { excludedCM := ctlres.MustNewResourceFromBytes([]byte(` apiVersion: v1 kind: ConfigMap @@ -115,11 +144,19 @@ spec: replicas: 1 `)) - config := newStripNameHashSuffixConfigFromConf(confs) - - require.True(t, config.EnabledFor(includedCM), "expected included ConfigMap to match!") - require.True(t, config.EnabledFor(includedKind), "expected included Kind to match!") - require.False(t, config.EnabledFor(excludedCM), "expected excluded ConfigMap to not match!") - require.False(t, config.EnabledFor(excludedKind), "expected unmentioned Kind to not match!") - + config := newStripNameHashSuffixConfigFromConf([]ctlconf.StripNameHashSuffixConfig{}) + + cases := []struct { + kind string + res ctlres.Resource + }{ + {"ConfigMap", excludedCM}, + {"Deployment", excludedKind}, + } + + for _, tc := range cases { + t.Run(tc.kind, func(t *testing.T) { + require.Falsef(t, config.EnabledFor(excludedCM), "expected %s to not match by default!") + }) + } } diff --git a/test/e2e/versioned_stripnamesuffix_test.go b/test/e2e/versioned_stripnamesuffix_test.go index 183574035..3f7597ed3 100644 --- a/test/e2e/versioned_stripnamesuffix_test.go +++ b/test/e2e/versioned_stripnamesuffix_test.go @@ -52,36 +52,41 @@ func kappDeployOverlay(kapp Kapp, name string, app string) (string, error) { return kapp.RunWithOpts([]string{"deploy", "-f", "-", "-a", app, "--json", "-c"}, RunOpts{IntoNs: true, StdinReader: testResReader}) } -func TestStripNameSuffixBasic(t *testing.T) { - - resp := run(t, "versioned1", "versioned2") - - // comparing the whole diff is unreliable; depending on the k8s version - // managedFields will (not) be set and as such (not) included in the diff. - // in that regard it also seems unreliable to assume "data" diff will - // always be in the first lines. - addRE := regexp.MustCompile(`(?m)^ \d+ \+ foo: bar$`) - delRE := regexp.MustCompile(`(?m)^ \d+ \- foo: foo$`) - - diffBlock := resp.Blocks[0] - - require.Regexpf(t, addRE, diffBlock, "Expected to see new line in diff") - require.Regexpf(t, delRE, diffBlock, "Expected to see old line in diff") +func TestStripNameSuffix(t *testing.T) { + t.Run("Basic", testStripNameSuffixBasic) + t.Run("NoOp", testStripNameSuffixNoop) } -func TestStripNameSuffixDeleteOld(t *testing.T) { +func testStripNameSuffixBasic(t *testing.T) { resp := run(t, "versioned1", "versioned2") - expectedNote := "Op: 1 create, 1 delete, 0 update, 0 noop, 0 exists" - - actualNote := resp.Tables[0].Notes[0] + for _, tc := range []struct { + name string + regex *regexp.Regexp + }{ + // comparing the whole diff is unreliable; depending on the k8s version + // managedFields will (not) be set and as such (not) included in the diff. + // in that regard it also seems unreliable to assume "data" diff will + // always be in the first lines. + {"new", regexp.MustCompile(`(?m)^ \d+ \+ foo: bar$`)}, + {"old", regexp.MustCompile(`(?m)^ \d+ \- foo: foo$`)}, + } { + t.Run("Diff_"+tc.name, func(t *testing.T) { + diffBlock := resp.Blocks[0] + require.Regexpf(t, tc.regex, diffBlock, "Expected to see %s line in diff", tc.name) + }) + } - // Ensure old ConfigMap is deleted - require.Exactlyf(t, expectedNote, actualNote, "Expected to one delete and one create Op") + t.Run("DeleteOld", func(t *testing.T) { + expectedNote := "Op: 1 create, 1 delete, 0 update, 0 noop, 0 exists" + actualNote := resp.Tables[0].Notes[0] + // Ensure old ConfigMap is deleted + require.Exactlyf(t, expectedNote, actualNote, "Expected one delete and one create Op") + }) } -func TestStripNameSuffixNoop(t *testing.T) { +func testStripNameSuffixNoop(t *testing.T) { resp := run(t, "versioned1", "versioned1")