From 87b680e1c0670b69795751f1d4e508faedefe86f Mon Sep 17 00:00:00 2001 From: vanou Date: Wed, 19 Feb 2020 23:53:10 +0900 Subject: [PATCH 01/12] Correct explanation of ApendAll method in ResMap interface This commit corrects expalation of ApendAll method in ResMap interface. AppendAll method should fail on CurId collision, not OrgId collision. --- api/resmap/resmap.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/resmap/resmap.go b/api/resmap/resmap.go index c1bd8910c..380dbe79d 100644 --- a/api/resmap/resmap.go +++ b/api/resmap/resmap.go @@ -114,7 +114,7 @@ type ResMap interface { Append(*resource.Resource) error // AppendAll appends another ResMap to self, - // failing on any OrgId collision. + // failing on any CurId collision. AppendAll(ResMap) error // AbsorbAll appends, replaces or merges the contents From c95a40933b828187f61d5dfdec06ef2fd85ccbca Mon Sep 17 00:00:00 2001 From: Phillip Wittrock Date: Fri, 21 Feb 2020 10:18:52 -0800 Subject: [PATCH 02/12] Update kyaml to v0.0.13 --- releasing/VERSIONS | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/releasing/VERSIONS b/releasing/VERSIONS index ffac7d0de..fa3eeacc3 100644 --- a/releasing/VERSIONS +++ b/releasing/VERSIONS @@ -6,7 +6,7 @@ # kyaml version export kyaml_major=0 export kyaml_minor=0 -export kyaml_patch=12 +export kyaml_patch=13 # kstatus version export kstatus_major=0 @@ -21,7 +21,7 @@ export api_patch=2 # cmd/config version export cmd_config_major=0 export cmd_config_minor=0 -export cmd_config_patch=12 +export cmd_config_patch=13 # cmd/kubectl version export cmd_kubectl_major=0 From 96ac25fff5451d2ecb9a1db8caf9abdeb805c632 Mon Sep 17 00:00:00 2001 From: Jeff Regan Date: Mon, 24 Feb 2020 10:17:52 -0800 Subject: [PATCH 03/12] Add lint-kustomize to prow-presubmit-check To do tool installs. --- Makefile | 1 + 1 file changed, 1 insertion(+) diff --git a/Makefile b/Makefile index f11f0a7d3..763f8037e 100644 --- a/Makefile +++ b/Makefile @@ -21,6 +21,7 @@ verify-kustomize: \ # https://github.com/kubernetes/test-infra/tree/master/config/jobs/kubernetes-sigs/kustomize .PHONY: prow-presubmit-check prow-presubmit-check: \ + lint-kustomize \ test-unit-kustomize-all .PHONY: verify-kustomize-e2e From a8b5ec2c616364b95d26444a34db42054dac4569 Mon Sep 17 00:00:00 2001 From: Phani Teja Marupaka Date: Mon, 24 Feb 2020 12:35:14 -0800 Subject: [PATCH 04/12] Suggested Changes and Unit Tests --- .../settersutil/substitutioncreator.go | 39 +++-- .../settersutil/substitutioncreator_test.go | 154 ++++++++++++++++++ 2 files changed, 175 insertions(+), 18 deletions(-) create mode 100644 kyaml/setters2/settersutil/substitutioncreator_test.go diff --git a/kyaml/setters2/settersutil/substitutioncreator.go b/kyaml/setters2/settersutil/substitutioncreator.go index 15059abd0..4134f4271 100644 --- a/kyaml/setters2/settersutil/substitutioncreator.go +++ b/kyaml/setters2/settersutil/substitutioncreator.go @@ -109,10 +109,13 @@ func (c SubstitutionCreator) CreateSettersForSubstitution(openAPIPath string) er sd := setters2.SetterDefinition{ // get the setter name from ref. Ex: from #/definitions/io.k8s.cli.setters.image_setter // extract image_setter - Name: strings.Split(value.Ref, ".")[4], - Value: m[value.Marker], + Name: strings.TrimPrefix(value.Ref, "#/definitions/io.k8s.cli.setters."), + Value: m[value.Marker], + } + err := sd.AddToFile(openAPIPath) + if err != nil { + return err } - sd.AddToFile(openAPIPath) } } return nil @@ -126,23 +129,23 @@ func (c SubstitutionCreator) GetValuesForMarkers() (map[string]string, error) { if err != nil { return nil, err } - s:=c.FieldValue - p:=c.Pattern - i:=0 - j:=0 + s := c.FieldValue + p := c.Pattern + i := 0 + j := 0 // iterate s, p with indices i, j respectively and when j hits the index of a marker, freeze j and iterate // i and capture string till we find the substring just after current marker and before next marker // Ex: s = "something/ubuntu:0.1.0", p = "something/IMAGE::VERSION", till j reaches 10 // just proceed i and j and check if s[i]==p[j] // when j is 10, freeze j and move i till it sees substring '::' which derives IMAGE = ubuntu and so on. - for i< len(s) && j< len(p) { + for i < len(s) && j < len(p) { if marker, ok := indices[j]; ok { value := "" - e := j+len(marker) + e := j + len(marker) - for i j { res = min(k-j, res) } @@ -201,8 +204,8 @@ func lenToNextMarker(m map[int]string, j int) int { } func min(a int, b int) int { - if a Date: Tue, 25 Feb 2020 11:38:36 -0800 Subject: [PATCH 05/12] Disable travis testing now that prow works. --- .travis.yml | 43 ------------------------------------------- 1 file changed, 43 deletions(-) delete mode 100644 .travis.yml diff --git a/.travis.yml b/.travis.yml deleted file mode 100644 index 99941c685..000000000 --- a/.travis.yml +++ /dev/null @@ -1,43 +0,0 @@ -os: - - linux - - osx -# TODO: Speed up the slowness of the osx travis runs -# Maybe cache brew installs? -# -# TODO: Uncomment when some gets the tests running on Windows. -# - windows - -addons: - apt: - packages: - - tree - homebrew: - packages: - - tree - update: true - -# Only clone the most recent commit. -git: - depth: 1 - -language: go - -go: - - "1.13" - -go_import_path: sigs.k8s.io/kustomize - -before_install: - - source ./travis/consider-early-travis-exit.sh - -# Skip the install process; let pre-commit.sh do it. -install: true - -script: - - make verify-kustomize - - ./travis/kyaml-pre-commit.sh - - ./travis/check-go-mod.sh - -# TBD. Suppressing for now. -notifications: - email: false From d70f3a7958a52eddd741e301f167285be38df0f9 Mon Sep 17 00:00:00 2001 From: Phani Teja Marupaka Date: Mon, 24 Feb 2020 13:34:16 -0800 Subject: [PATCH 06/12] Update go files --- kyaml/go.mod | 5 +- kyaml/go.sum | 10 ++- .../settersutil/substitutioncreator.go | 87 ++++++++++--------- .../settersutil/substitutioncreator_test.go | 78 +++++------------ 4 files changed, 75 insertions(+), 105 deletions(-) diff --git a/kyaml/go.mod b/kyaml/go.mod index de2170cd8..33eefaad1 100644 --- a/kyaml/go.mod +++ b/kyaml/go.mod @@ -9,6 +9,7 @@ require ( github.com/sergi/go-diff v1.1.0 github.com/stretchr/testify v1.4.0 github.com/xlab/treeprint v0.0.0-20181112141820-a009c3971eca - gopkg.in/yaml.v2 v2.2.4 - gopkg.in/yaml.v3 v3.0.0-20191026110619-0b21df46bc1d + golang.org/x/net v0.0.0-20191004110552-13f9640d40b9 // indirect + gopkg.in/yaml.v2 v2.2.7 + gopkg.in/yaml.v3 v3.0.0-20191120175047-4206685974f2 ) diff --git a/kyaml/go.sum b/kyaml/go.sum index 98d3f4c1c..05b1b96aa 100644 --- a/kyaml/go.sum +++ b/kyaml/go.sum @@ -39,18 +39,20 @@ golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACk golang.org/x/net v0.0.0-20190613194153-d28f0bde5980/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/net v0.0.0-20190827160401-ba9fcec4b297 h1:k7pJ2yAPLPgbskkFdhRCsA77k2fySZ1zf2zCjvQCiIM= golang.org/x/net v0.0.0-20190827160401-ba9fcec4b297/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= +golang.org/x/net v0.0.0-20191004110552-13f9640d40b9 h1:rjwSpXsdiK0dV8/Naq3kAw9ymfAeJIyd0upUIElB+lI= +golang.org/x/net v0.0.0-20191004110552-13f9640d40b9/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.2 h1:tW2bmiBqwgJj/UpqtC8EpXEZVYOwU0yG4iWbprSVAcs= golang.org/x/text v0.3.2/go.mod h1:bEr9sfX3Q8Zfm5fL9x+3itogRgK3+ptLWKqgva+5dAk= golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= -gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127 h1:qIbj1fsPNlZgppZ+VLlY7N33q108Sa+fhmuc+sWQYwY= gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15 h1:YR8cESwS4TdDjEe65xsg0ogRM/Nc3DYOhEAlW+xobZo= gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/yaml.v2 v2.2.2/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= -gopkg.in/yaml.v2 v2.2.4 h1:/eiJrUcujPVeJ3xlSWaiNi3uSVmDGBK1pDHUHAnao1I= gopkg.in/yaml.v2 v2.2.4/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= -gopkg.in/yaml.v3 v3.0.0-20191026110619-0b21df46bc1d h1:LCPbGQ34PMrwad11aMZ+dbz5SAsq/0ySjRwQ8I9Qwd8= -gopkg.in/yaml.v3 v3.0.0-20191026110619-0b21df46bc1d/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= +gopkg.in/yaml.v2 v2.2.7 h1:VUgggvou5XRW9mHwD/yXxIYSMtY0zoKQf/v226p2nyo= +gopkg.in/yaml.v2 v2.2.7/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= +gopkg.in/yaml.v3 v3.0.0-20191120175047-4206685974f2 h1:XZx7nhd5GMaZpmDaEHFVafUZC7ya0fuo7cSJ3UCKYmM= +gopkg.in/yaml.v3 v3.0.0-20191120175047-4206685974f2/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/kyaml/setters2/settersutil/substitutioncreator.go b/kyaml/setters2/settersutil/substitutioncreator.go index 4134f4271..4acef990f 100644 --- a/kyaml/setters2/settersutil/substitutioncreator.go +++ b/kyaml/setters2/settersutil/substitutioncreator.go @@ -4,11 +4,10 @@ package settersutil import ( - "io/ioutil" "math" "strings" - "sigs.k8s.io/kind/pkg/errors" + "sigs.k8s.io/kustomize/kyaml/errors" "sigs.k8s.io/kustomize/kyaml/kio" "sigs.k8s.io/kustomize/kyaml/openapi" "sigs.k8s.io/kustomize/kyaml/setters2" @@ -78,13 +77,7 @@ func (c SubstitutionCreator) Create(openAPIPath, resourcesPath string) error { // CreateSettersForSubstitution creates the setters for all the references in the substitution // values if they don't already exist in openAPIPath file. func (c SubstitutionCreator) CreateSettersForSubstitution(openAPIPath string) error { - b, err := ioutil.ReadFile(openAPIPath) - if err != nil { - return err - } - - // parse the yaml file - y, err := yaml.Parse(string(b)) + y, err := yaml.ReadFile(openAPIPath) if err != nil { return err } @@ -129,58 +122,55 @@ func (c SubstitutionCreator) GetValuesForMarkers() (map[string]string, error) { if err != nil { return nil, err } - s := c.FieldValue - p := c.Pattern - i := 0 - j := 0 - // iterate s, p with indices i, j respectively and when j hits the index of a marker, freeze j and iterate - // i and capture string till we find the substring just after current marker and before next marker + fv := c.FieldValue + pattern := c.Pattern + fvInd := 0 + patternInd := 0 + // iterate fv, pattern with indices fvInd, patternInd respectively and when patternInd hits the index of a marker, + // freeze patternInd and iterate fvInd and capture string till we find the substring just after current marker + // and before next marker - // Ex: s = "something/ubuntu:0.1.0", p = "something/IMAGE::VERSION", till j reaches 10 - // just proceed i and j and check if s[i]==p[j] - // when j is 10, freeze j and move i till it sees substring '::' which derives IMAGE = ubuntu and so on. - for i < len(s) && j < len(p) { - if marker, ok := indices[j]; ok { - value := "" - e := j + len(marker) - - for i < len(s) && (e == len(p) || - s[i:min(len(s), i+lenToNextMarker(indices, e))] != p[e:min(e+lenToNextMarker(indices, e), len(p))]) { - value += string(s[i]) - i++ - } + // Ex: fv = "something/ubuntu:0.1.0", pattern = "something/IMAGE:VERSION", till patternInd reaches 10 + // just proceed fvInd and patternInd and check if fv[fvInd]==pattern[patternInd] when patternInd is 10, + // freeze patternInd and move fvInd till it sees substring ':' which derives IMAGE = ubuntu and so on. + for fvInd < len(fv) && patternInd < len(pattern) { + // if we hit marker index, extract its corresponding value + if marker, ok := indices[patternInd]; ok { + // increment the patternInd to end of marker. This helps us to extract the substring before next marker. + patternInd += len(marker) + value := c.extractValueForMarker(&fvInd, fv, patternInd, indices) // if marker is repeated in the pattern, make sure that the corresponding values // are same and throw error if not. if prevValue, ok := m[marker]; ok && prevValue != value { - return nil, errors.Errorf("Same marker is found to have different values in field value.") + return nil, errors.Errorf("same marker is found to have different values in field value") } m[marker] = value - j += len(marker) } else { - if s[i] != p[j] { - return nil, errors.Errorf("Unable to derive values for markers. Create setters for all markers and then try again.") + // Ex: fv = "samething/ubuntu:0.1.0" pattern = "something/IMAGE:VERSION". Error out at 'a' in fv. + if fv[fvInd] != pattern[patternInd] { + return nil, errors.Errorf("unable to derive values for markers, create setters for all markers and then try again") } - i++ - j++ + fvInd++ + patternInd++ } } // check if both strings are completely visited or throw error - if i < len(s) || j < len(p) { - return nil, errors.Errorf("Unable to derive values for markers. Create setters for all markers and then try again.") + if fvInd < len(fv) || patternInd < len(pattern) { + return nil, errors.Errorf("unable to derive values for markers, create setters for all markers and then try again") } return m, nil } // GetStartIndices returns the start indices of all the markers in the pattern func (c SubstitutionCreator) GetStartIndices() (map[int]string, error) { - inds := make(map[int]string) + indices := make(map[int]string) p := c.Pattern for _, value := range c.Values { m := value.Marker found := false for i := range p { if strings.HasPrefix(p[i:], m) { - inds[i] = m + indices[i] = m found = true } } @@ -188,12 +178,27 @@ func (c SubstitutionCreator) GetStartIndices() (map[int]string, error) { return nil, errors.Errorf("Unable to find marker " + m + " in the pattern") } } - return inds, nil + return indices, nil +} + +func (c SubstitutionCreator) extractValueForMarker(fvInd *int, fv string, patternInd int, indices map[int]string) string { + value := "" + lm := lenTillNextMarker(indices, patternInd) + + // keep appending chars to value till we find the substring between 2 markers in fv + // In example fv = "something/ubuntu:0.1.0", pattern = "something/IMAGE:VERSION", + // proceed till we find substring ":" in fv which makes value = ubuntu for marker IMAGE + for *fvInd < len(fv) && (patternInd == len(c.Pattern) || + fv[*fvInd:min(len(fv), *fvInd+lm)] != c.Pattern[patternInd:min(patternInd+lm, len(c.Pattern))]) { + value += string(fv[*fvInd]) + *fvInd++ + } + return value } // lenToNextMarker takes in the indices map, an index and returns the distance to -// next greater index -func lenToNextMarker(m map[int]string, j int) int { +// next marker start index in indices map +func lenTillNextMarker(m map[int]string, j int) int { res := math.MaxInt32 for k := range m { if k > j { diff --git a/kyaml/setters2/settersutil/substitutioncreator_test.go b/kyaml/setters2/settersutil/substitutioncreator_test.go index 9909d4254..c39956424 100644 --- a/kyaml/setters2/settersutil/substitutioncreator_test.go +++ b/kyaml/setters2/settersutil/substitutioncreator_test.go @@ -11,19 +11,9 @@ import ( ) func TestGetValuesForMarkersPositive(t *testing.T) { - value1 := setters2.Value{ - Marker: "IMAGE", - } - - value2 := setters2.Value{ - Marker: "VERSION", - } - - values := []setters2.Value{value1, value2} - c := SubstitutionCreator{ Pattern: "something/IMAGE::VERSION/otherthing/IMAGE::VERSION/", - Values: values, + Values: Values(), FieldValue: "something/nginx::0.1.0/otherthing/nginx::0.1.0/", } @@ -38,19 +28,9 @@ func TestGetValuesForMarkersPositive(t *testing.T) { } func TestGetValuesForMarkersDiffMarkerValues(t *testing.T) { - value1 := setters2.Value{ - Marker: "IMAGE", - } - - value2 := setters2.Value{ - Marker: "VERSION", - } - - values := []setters2.Value{value1, value2} - c := SubstitutionCreator{ Pattern: "something/IMAGE:VERSION/IMAGE", - Values: values, + Values: Values(), FieldValue: "something/nginx:0.1.0/ubuntu", } @@ -60,25 +40,15 @@ func TestGetValuesForMarkersDiffMarkerValues(t *testing.T) { t.FailNow() } - if !assert.Equal(t, err.Error(), "Same marker is found to have different values in field value.") { + if !assert.Equal(t, err.Error(), "same marker is found to have different values in field value") { t.FailNow() } } func TestGetValuesForMarkersNoMatch(t *testing.T) { - value1 := setters2.Value{ - Marker: "IMAGE", - } - - value2 := setters2.Value{ - Marker: "VERSION", - } - - values := []setters2.Value{value1, value2} - c := SubstitutionCreator{ Pattern: "something/IMAGE:VERSION", - Values: values, + Values: Values(), FieldValue: "otherthing/nginx:0.1.0", } @@ -88,25 +58,15 @@ func TestGetValuesForMarkersNoMatch(t *testing.T) { t.FailNow() } - if !assert.Equal(t, err.Error(), "Unable to derive values for markers. Create setters for all markers and then try again.") { + if !assert.Equal(t, err.Error(), "unable to derive values for markers, create setters for all markers and then try again") { t.FailNow() } } func TestGetValuesForMarkersNoMatch2(t *testing.T) { - value1 := setters2.Value{ - Marker: "IMAGE", - } - - value2 := setters2.Value{ - Marker: "VERSION", - } - - values := []setters2.Value{value1, value2} - c := SubstitutionCreator{ Pattern: "something/IMAGE:VERSION/abc", - Values: values, + Values: Values(), FieldValue: "something/nginx:0.1.0/abcd", } @@ -116,29 +76,19 @@ func TestGetValuesForMarkersNoMatch2(t *testing.T) { t.FailNow() } - if !assert.Equal(t, err.Error(), "Unable to derive values for markers. Create setters for all markers and then try again.") { + if !assert.Equal(t, err.Error(), "unable to derive values for markers, create setters for all markers and then try again") { t.FailNow() } } func TestGetValuesForMarkersSubStngMarkers(t *testing.T) { - value1 := setters2.Value{ - Marker: "IMAGE", - } - - value2 := setters2.Value{ - Marker: "VERSION", - } - value3 := setters2.Value{ Marker: "MAGE", } - values := []setters2.Value{value1, value2, value3} - c := SubstitutionCreator{ Pattern: "something/IMAGE:VERSION/abc/MAGE", - Values: values, + Values: append(Values(), value3), FieldValue: "something/nginx:0.1.0/abc/ubuntu", } @@ -152,3 +102,15 @@ func TestGetValuesForMarkersSubStngMarkers(t *testing.T) { assert.Equal(t, m["VERSION"], "0.1.0") assert.Equal(t, m["MAGE"], "ubuntu") } + +func Values() []setters2.Value { + value1 := setters2.Value{ + Marker: "IMAGE", + } + + value2 := setters2.Value{ + Marker: "VERSION", + } + + return []setters2.Value{value1, value2} +} From 275bf05ae242ef8491977396163d07c714a72fe1 Mon Sep 17 00:00:00 2001 From: Phani Teja Marupaka Date: Tue, 25 Feb 2020 17:15:24 -0800 Subject: [PATCH 07/12] Refactoring and Table tests --- .../settersutil/substitutioncreator.go | 110 ++++++---- .../settersutil/substitutioncreator_test.go | 191 +++++++++--------- 2 files changed, 169 insertions(+), 132 deletions(-) diff --git a/kyaml/setters2/settersutil/substitutioncreator.go b/kyaml/setters2/settersutil/substitutioncreator.go index 4acef990f..fc187c63e 100644 --- a/kyaml/setters2/settersutil/substitutioncreator.go +++ b/kyaml/setters2/settersutil/substitutioncreator.go @@ -4,7 +4,6 @@ package settersutil import ( - "math" "strings" "sigs.k8s.io/kustomize/kyaml/errors" @@ -138,17 +137,23 @@ func (c SubstitutionCreator) GetValuesForMarkers() (map[string]string, error) { if marker, ok := indices[patternInd]; ok { // increment the patternInd to end of marker. This helps us to extract the substring before next marker. patternInd += len(marker) - value := c.extractValueForMarker(&fvInd, fv, patternInd, indices) + var value string + if value, fvInd, err = c.extractValueForMarker(fvInd, fv, patternInd, indices); err != nil { + return nil, err + } // if marker is repeated in the pattern, make sure that the corresponding values // are same and throw error if not. if prevValue, ok := m[marker]; ok && prevValue != value { - return nil, errors.Errorf("same marker is found to have different values in field value") + return nil, errors.Errorf( + "marker %s is found to have different values %s and %s", marker, prevValue, value) } m[marker] = value } else { // Ex: fv = "samething/ubuntu:0.1.0" pattern = "something/IMAGE:VERSION". Error out at 'a' in fv. if fv[fvInd] != pattern[patternInd] { - return nil, errors.Errorf("unable to derive values for markers, create setters for all markers and then try again") + return nil, errors.Errorf( + "unable to derive values for markers, " + + "create setters for all markers and then try again") } fvInd++ patternInd++ @@ -156,7 +161,9 @@ func (c SubstitutionCreator) GetValuesForMarkers() (map[string]string, error) { } // check if both strings are completely visited or throw error if fvInd < len(fv) || patternInd < len(pattern) { - return nil, errors.Errorf("unable to derive values for markers, create setters for all markers and then try again") + return nil, errors.Errorf( + "unable to derive values for markers, " + + "create setters for all markers and then try again") } return m, nil } @@ -164,48 +171,83 @@ func (c SubstitutionCreator) GetValuesForMarkers() (map[string]string, error) { // GetStartIndices returns the start indices of all the markers in the pattern func (c SubstitutionCreator) GetStartIndices() (map[int]string, error) { indices := make(map[int]string) - p := c.Pattern for _, value := range c.Values { - m := value.Marker found := false - for i := range p { - if strings.HasPrefix(p[i:], m) { - indices[i] = m + for i := range c.Pattern { + if strings.HasPrefix(c.Pattern[i:], value.Marker) { + indices[i] = value.Marker found = true } } if !found { - return nil, errors.Errorf("Unable to find marker " + m + " in the pattern") + return nil, errors.Errorf("unable to find marker " + value.Marker + " in the pattern") } } + if err := validateMarkers(indices); err != nil { + return nil, err + } return indices, nil } -func (c SubstitutionCreator) extractValueForMarker(fvInd *int, fv string, patternInd int, indices map[int]string) string { - value := "" - lm := lenTillNextMarker(indices, patternInd) - - // keep appending chars to value till we find the substring between 2 markers in fv - // In example fv = "something/ubuntu:0.1.0", pattern = "something/IMAGE:VERSION", - // proceed till we find substring ":" in fv which makes value = ubuntu for marker IMAGE - for *fvInd < len(fv) && (patternInd == len(c.Pattern) || - fv[*fvInd:min(len(fv), *fvInd+lm)] != c.Pattern[patternInd:min(patternInd+lm, len(c.Pattern))]) { - value += string(fv[*fvInd]) - *fvInd++ - } - return value -} - -// lenToNextMarker takes in the indices map, an index and returns the distance to -// next marker start index in indices map -func lenTillNextMarker(m map[int]string, j int) int { - res := math.MaxInt32 - for k := range m { - if k > j { - res = min(k-j, res) +// validateMarkers takes the indices map, checks if any of 2 markers not have delimiters, +// checks if any marker is substring of other and returns error +func validateMarkers(indices map[int]string) error { + for k1, v1 := range indices { + for k2, v2 := range indices { + if k1 != k2 && k1+len(v1) == k2 { + return errors.Errorf( + "markers %s and %s are found to have no delimiters between them,"+ + " pre-create setters and try again", v1, v2) + } + if v1 != v2 && strings.Contains(v1, v2) { + return errors.Errorf( + "markers %s is substring of %s,"+ + " no marker should be substring of other", v2, v1) + } } } - return res + return nil +} + +// extractValueForMarker returns the value string for a marker and the incremented index +func (c SubstitutionCreator) extractValueForMarker(fvInd int, fv string, patternInd int, indices map[int]string) (string, int, error) { + nonMarkerStr := strTillNextMarker(indices, patternInd, c.Pattern) + + // return the remaining string of fv till end if patternInd is at end of pattern + if patternInd == len(c.Pattern) { + return fv[fvInd:], len(fv), nil + } + + // split remaining fv starting from fvInd with the non marker substring delimiter and get the first value + // In example fv = "something/ubuntu::0.1.0", pattern = "something/IMAGE::VERSION", + // split with "::" delimiter in fv which gives markerValue = ubuntu for marker IMAGE + // increment fvInd by length of extracted marker value and return fvInd + if markerValues := strings.Split(fv[fvInd:], nonMarkerStr); len(markerValues) > 0 { + return markerValues[0], fvInd + len(markerValues[0]), nil + } + + return "", -1, errors.Errorf( + "unable to derive values for markers," + + " create setters for all markers and then try again") +} + +// substrOfLen takes a string, start index and length and returns substring of given length +// or till end of string +func substrOfLen(str string, startInd int, length int) string { + return str[startInd:min(len(str), startInd+length)] +} + +// strTillNextMarker takes in the indices map, a start index and returns the substring till +// start of next marker +func strTillNextMarker(indices map[int]string, startInd int, pattern string) string { + // initialize with max value which is length of pattern + nextMarkerStartInd := len(pattern) + for ind := range indices { + if ind > startInd { + nextMarkerStartInd = min(ind-startInd, nextMarkerStartInd) + } + } + return substrOfLen(pattern, startInd, nextMarkerStartInd) } func min(a int, b int) int { diff --git a/kyaml/setters2/settersutil/substitutioncreator_test.go b/kyaml/setters2/settersutil/substitutioncreator_test.go index c39956424..9c6b2951b 100644 --- a/kyaml/setters2/settersutil/substitutioncreator_test.go +++ b/kyaml/setters2/settersutil/substitutioncreator_test.go @@ -7,110 +7,105 @@ import ( "testing" "github.com/stretchr/testify/assert" + "sigs.k8s.io/kustomize/kyaml/errors" "sigs.k8s.io/kustomize/kyaml/setters2" ) -func TestGetValuesForMarkersPositive(t *testing.T) { - c := SubstitutionCreator{ - Pattern: "something/IMAGE::VERSION/otherthing/IMAGE::VERSION/", - Values: Values(), - FieldValue: "something/nginx::0.1.0/otherthing/nginx::0.1.0/", +func TestGetValuesForMarkers(t *testing.T) { + var tests = []struct { + name string + pattern string + fieldValue string + markers []string + expectedError error + expectedOutput map[string]string + }{ + { + name: "positive example", + markers: []string{"IMAGE", "VERSION"}, + pattern: "something/IMAGE::VERSION/otherthing/IMAGE::VERSION/", + fieldValue: "something/nginx::0.1.0/otherthing/nginx::0.1.0/", + expectedOutput: map[string]string{"IMAGE": "nginx", "VERSION": "0.1.0"}, + }, + { + name: "marker with different values", + markers: []string{"IMAGE", "VERSION"}, + pattern: "something/IMAGE:VERSION/IMAGE", + fieldValue: "something/nginx:0.1.0/ubuntu", + expectedError: errors.Errorf("marker IMAGE is found to have different values nginx and ubuntu"), + }, + { + name: "unmatched pattern", + markers: []string{"IMAGE", "VERSION"}, + pattern: "something/IMAGE:VERSION", + fieldValue: "otherthing/nginx:0.1.0", + expectedError: errors.Errorf("unable to derive values for markers"), + }, + { + name: "unmatched pattern at the end", + markers: []string{"IMAGE", "VERSION"}, + pattern: "something/IMAGE:VERSION/abc", + fieldValue: "something/nginx:0.1.0/abcd", + expectedError: errors.Errorf("unable to derive values for markers"), + }, + { + name: "substring markers", + markers: []string{"IMAGE", "VERSION", "MAGE"}, + pattern: "something/IMAGE:VERSION/abc/MAGE", + fieldValue: "something/nginx:0.1.0/abc/ubuntu", + expectedError: errors.Errorf("no marker should be substring of other"), + }, + { + name: "markers with no delimiters", + markers: []string{"IMAGE", "VERSION"}, + pattern: "something/IMAGEVERSION/", + fieldValue: "something/nginx0.1.0/", + expectedError: errors.Errorf("no delimiters between them"), + }, + { + name: "unmatched delimiter", + markers: []string{"IMAGE", "VERSION"}, + pattern: "something/IMAGE:^VERSION/otherthing/IMAGE::VERSION/", + fieldValue: "something/nginx::0.1.0/otherthing/nginx::0.1.0/", + expectedError: errors.Errorf("unable to derive values for markers"), + }, } + for i := range tests { + test := tests[i] + t.Run(test.name, func(t *testing.T) { + values := []setters2.Value{} + for _, marker := range test.markers { + value := setters2.Value{ + Marker: marker, + } + values = append(values, value) + } - m, err := c.GetValuesForMarkers() + sc := SubstitutionCreator{ + Pattern: test.pattern, + Values: values, + FieldValue: test.fieldValue, + } - if !assert.NoError(t, err) { - t.FailNow() - } + m, err := sc.GetValuesForMarkers() - assert.Equal(t, m["IMAGE"], "nginx") - assert.Equal(t, m["VERSION"], "0.1.0") -} - -func TestGetValuesForMarkersDiffMarkerValues(t *testing.T) { - c := SubstitutionCreator{ - Pattern: "something/IMAGE:VERSION/IMAGE", - Values: Values(), - FieldValue: "something/nginx:0.1.0/ubuntu", - } - - _, err := c.GetValuesForMarkers() - - if !assert.Error(t, err) { - t.FailNow() - } - - if !assert.Equal(t, err.Error(), "same marker is found to have different values in field value") { - t.FailNow() + if test.expectedError == nil { + // fail if expectedError is nil but actual error is not + if !assert.NoError(t, err) { + t.FailNow() + } + // check if all the expected markers and values are present in actual map + for k, v := range test.expectedOutput { + if val, ok := m[k]; ok { + assert.Equal(t, v, val) + } else { + t.FailNow() + } + } + } else { + //if expectedError is not nil, check for correctness of error message + assert.Contains(t, err.Error(), test.expectedError.Error()) + } + }) } } - -func TestGetValuesForMarkersNoMatch(t *testing.T) { - c := SubstitutionCreator{ - Pattern: "something/IMAGE:VERSION", - Values: Values(), - FieldValue: "otherthing/nginx:0.1.0", - } - - _, err := c.GetValuesForMarkers() - - if !assert.Error(t, err) { - t.FailNow() - } - - if !assert.Equal(t, err.Error(), "unable to derive values for markers, create setters for all markers and then try again") { - t.FailNow() - } -} - -func TestGetValuesForMarkersNoMatch2(t *testing.T) { - c := SubstitutionCreator{ - Pattern: "something/IMAGE:VERSION/abc", - Values: Values(), - FieldValue: "something/nginx:0.1.0/abcd", - } - - _, err := c.GetValuesForMarkers() - - if !assert.Error(t, err) { - t.FailNow() - } - - if !assert.Equal(t, err.Error(), "unable to derive values for markers, create setters for all markers and then try again") { - t.FailNow() - } -} - -func TestGetValuesForMarkersSubStngMarkers(t *testing.T) { - value3 := setters2.Value{ - Marker: "MAGE", - } - - c := SubstitutionCreator{ - Pattern: "something/IMAGE:VERSION/abc/MAGE", - Values: append(Values(), value3), - FieldValue: "something/nginx:0.1.0/abc/ubuntu", - } - - m, err := c.GetValuesForMarkers() - - if !assert.NoError(t, err) { - t.FailNow() - } - - assert.Equal(t, m["IMAGE"], "nginx") - assert.Equal(t, m["VERSION"], "0.1.0") - assert.Equal(t, m["MAGE"], "ubuntu") -} - -func Values() []setters2.Value { - value1 := setters2.Value{ - Marker: "IMAGE", - } - - value2 := setters2.Value{ - Marker: "VERSION", - } - - return []setters2.Value{value1, value2} -} From 573d7b7234223c5b86bc30fc9a07f7fb7c2e60a7 Mon Sep 17 00:00:00 2001 From: Phillip Wittrock Date: Wed, 26 Feb 2020 13:53:47 -0800 Subject: [PATCH 08/12] Setters: clear set-by if unspecified when setting a value --- kyaml/setters2/set.go | 6 ++--- kyaml/setters2/set_test.go | 48 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 4 deletions(-) diff --git a/kyaml/setters2/set.go b/kyaml/setters2/set.go index ec242d805..e610c035e 100644 --- a/kyaml/setters2/set.go +++ b/kyaml/setters2/set.go @@ -150,10 +150,8 @@ func (s SetOpenAPI) Filter(object *yaml.RNode) (*yaml.RNode, error) { return nil, err } - if s.SetBy != "" { - if err := def.PipeE(&yaml.FieldSetter{Name: "setBy", StringValue: s.SetBy}); err != nil { - return nil, err - } + if err := def.PipeE(&yaml.FieldSetter{Name: "setBy", StringValue: s.SetBy}); err != nil { + return nil, err } if s.Description != "" { diff --git a/kyaml/setters2/set_test.go b/kyaml/setters2/set_test.go index 1550a6244..28bf43ac1 100644 --- a/kyaml/setters2/set_test.go +++ b/kyaml/setters2/set_test.go @@ -581,6 +581,54 @@ openAPI: setter: name: no-match-2 value: "2" +`, + }, + { + name: "set-replicas-set-by-empty", + setter: "replicas", + value: "3", + input: ` +openAPI: + definitions: + io.k8s.cli.setters.no-match-1': + x-k8s-cli: + setter: + name: no-match-1 + value: "1" + setBy: "package-default" + io.k8s.cli.setters.replicas: + x-k8s-cli: + setter: + name: replicas + value: "4" + setBy: "package-default" + io.k8s.cli.setters.no-match-2': + x-k8s-cli: + setter: + name: no-match-2 + value: "2" + setBy: "package-default" + `, + expected: ` +openAPI: + definitions: + io.k8s.cli.setters.no-match-1': + x-k8s-cli: + setter: + name: no-match-1 + value: "1" + setBy: "package-default" + io.k8s.cli.setters.replicas: + x-k8s-cli: + setter: + name: replicas + value: "3" + io.k8s.cli.setters.no-match-2': + x-k8s-cli: + setter: + name: no-match-2 + value: "2" + setBy: "package-default" `, }, { From cf61a360e05d3fa570e4cb311cc57ee1d5ad102f Mon Sep 17 00:00:00 2001 From: Phillip Wittrock Date: Wed, 26 Feb 2020 20:13:06 -0800 Subject: [PATCH 09/12] Support for enum mappings in setters --- kyaml/setters2/add.go | 4 + kyaml/setters2/set.go | 37 +++++- kyaml/setters2/set_test.go | 224 +++++++++++++++++++++++++++++++++++++ kyaml/setters2/types.go | 5 +- 4 files changed, 266 insertions(+), 4 deletions(-) diff --git a/kyaml/setters2/add.go b/kyaml/setters2/add.go index 8bf7cc49d..c5f3fb8e9 100644 --- a/kyaml/setters2/add.go +++ b/kyaml/setters2/add.go @@ -104,6 +104,10 @@ type SetterDefinition struct { // Count is the number of fields set by this setter. Count int `yaml:"count,omitempty"` + + // EnumValues is a map of possible setter values to actual field values. + // May be used for t-shirt sizing values -- e.g. set cpu to small, medium, large + EnumValues map[string]string `yaml:"enumValues,omitempty"` } func (sd SetterDefinition) AddToFile(path string) error { diff --git a/kyaml/setters2/set.go b/kyaml/setters2/set.go index e610c035e..a3bc305b1 100644 --- a/kyaml/setters2/set.go +++ b/kyaml/setters2/set.go @@ -86,8 +86,14 @@ func (s *Set) substitute(field *yaml.RNode, ext *cliExtension) (bool, error) { if err != nil { return false, errors.Wrap(err) } - // substitute the setters current value into the substitution pattern - p = strings.ReplaceAll(p, v.Marker, subSetter.Setter.Value) + + if val, found := subSetter.Setter.EnumValues[subSetter.Setter.Value]; found { + // resolve enum for substitution + p = strings.ReplaceAll(p, v.Marker, val) + } else { + // substitute the setters current value into the substitution pattern + p = strings.ReplaceAll(p, v.Marker, subSetter.Setter.Value) + } if subSetter.Setter.Name == s.Name { // the substitution depends on the specified setter @@ -114,6 +120,11 @@ func (s *Set) set(field *yaml.RNode, ext *cliExtension) bool { // TODO(pwittrock): validate the field value + if val, found := ext.Setter.EnumValues[ext.Setter.Value]; found { + field.YNode().Value = val + return true + } + // this has a full setter, set its value field.YNode().Value = ext.Setter.Value return true @@ -146,6 +157,28 @@ func (s SetOpenAPI) Filter(object *yaml.RNode) (*yaml.RNode, error) { if def == nil { return nil, errors.Errorf("no setter %s found", s.Name) } + + // if the setter is an enumeration, then make sure the value matches + if values, err := def.Pipe(yaml.Lookup("enumValues")); err != nil { + return nil, err + } else if values != nil { + fields, err := values.Fields() + if err != nil { + return nil, err + } + var match bool + for i := range fields { + if fields[i] == s.Value { + match = true + break + } + } + if !match { + return nil, errors.Errorf("%s does not match the possible values for %s: [%s]", + s.Value, s.Name, strings.Join(fields, ",")) + } + } + if err := def.PipeE(&yaml.FieldSetter{Name: "value", StringValue: s.Value}); err != nil { return nil, err } diff --git a/kyaml/setters2/set_test.go b/kyaml/setters2/set_test.go index 28bf43ac1..705df8c01 100644 --- a/kyaml/setters2/set_test.go +++ b/kyaml/setters2/set_test.go @@ -58,6 +58,92 @@ metadata: name: nginx-deployment spec: replicas: 4 # {"$ref": "#/definitions/io.k8s.cli.setters.replicas"} + `, + }, + { + name: "set-replicas-enum", + setter: "replicas", + openapi: ` +openAPI: + definitions: + io.k8s.cli.setters.no-match-1': + x-k8s-cli: + setter: + name: no-match-1 + value: "1" + io.k8s.cli.setters.replicas: + x-k8s-cli: + setter: + name: replicas + value: "medium" + enumValues: + small: "1" + medium: "5" + large: "50" + io.k8s.cli.setters.no-match-2': + x-k8s-cli: + setter: + name: no-match-2 + value: "2" + `, + input: ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment +spec: + replicas: 1 # {"$ref": "#/definitions/io.k8s.cli.setters.replicas"} + `, + expected: ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment +spec: + replicas: 5 # {"$ref": "#/definitions/io.k8s.cli.setters.replicas"} + `, + }, + { + name: "set-replicas-enum-large", + setter: "replicas", + openapi: ` +openAPI: + definitions: + io.k8s.cli.setters.no-match-1': + x-k8s-cli: + setter: + name: no-match-1 + value: "1" + io.k8s.cli.setters.replicas: + x-k8s-cli: + setter: + name: replicas + value: "large" + enumValues: + small: "1" + medium: "5" + large: "50" + io.k8s.cli.setters.no-match-2': + x-k8s-cli: + setter: + name: no-match-2 + value: "2" + `, + input: ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment +spec: + replicas: 1 # {"$ref": "#/definitions/io.k8s.cli.setters.replicas"} + `, + expected: ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment +spec: + replicas: 50 # {"$ref": "#/definitions/io.k8s.cli.setters.replicas"} `, }, { @@ -155,6 +241,61 @@ spec: containers: - name: nginx image: nginx:1.8.1 # {"$ref": "#/definitions/io.k8s.cli.substitutions.image"} + `, + }, + { + name: "substitute-image-name-enum", + setter: "image-tag", + openapi: ` +openAPI: + definitions: + io.k8s.cli.setters.image-name: + x-k8s-cli: + setter: + name: image-name + value: "helloworld" + enumValues: + nginx: gcr.io/nginx + helloworld: us.gcr.io/helloworld + io.k8s.cli.setters.image-tag: + x-k8s-cli: + setter: + name: image-tag + value: "1.8.1" + io.k8s.cli.substitutions.image: + x-k8s-cli: + substitution: + name: image + pattern: IMAGE_NAME:IMAGE_TAG + values: + - marker: "IMAGE_NAME" + ref: "#/definitions/io.k8s.cli.setters.image-name" + - marker: "IMAGE_TAG" + ref: "#/definitions/io.k8s.cli.setters.image-tag" + `, + input: ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment +spec: + template: + spec: + containers: + - name: nginx + image: nginx:1.7.9 # {"$ref": "#/definitions/io.k8s.cli.substitutions.image"} + `, + expected: ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment +spec: + template: + spec: + containers: + - name: nginx + image: us.gcr.io/helloworld:1.8.1 # {"$ref": "#/definitions/io.k8s.cli.substitutions.image"} `, }, { @@ -631,6 +772,89 @@ openAPI: setBy: "package-default" `, }, + { + name: "set-replicas-with-enum", + setter: "replicas", + value: "baz", + input: ` +openAPI: + definitions: + io.k8s.cli.setters.no-match-1': + x-k8s-cli: + setter: + name: no-match-1 + value: "1" + setBy: "package-default" + io.k8s.cli.setters.replicas: + x-k8s-cli: + setter: + name: replicas + value: "foo" + enumValues: + foo: bar + baz: biz + io.k8s.cli.setters.no-match-2': + x-k8s-cli: + setter: + name: no-match-2 + value: "2" + setBy: "package-default" + `, + expected: ` +openAPI: + definitions: + io.k8s.cli.setters.no-match-1': + x-k8s-cli: + setter: + name: no-match-1 + value: "1" + setBy: "package-default" + io.k8s.cli.setters.replicas: + x-k8s-cli: + setter: + name: replicas + value: "baz" + enumValues: + foo: bar + baz: biz + io.k8s.cli.setters.no-match-2': + x-k8s-cli: + setter: + name: no-match-2 + value: "2" + setBy: "package-default" +`, + }, + { + name: "set-replicas-fail", + setter: "replicas", + value: "hello", + input: ` +openAPI: + definitions: + io.k8s.cli.setters.no-match-1': + x-k8s-cli: + setter: + name: no-match-1 + value: "1" + setBy: "package-default" + io.k8s.cli.setters.replicas: + x-k8s-cli: + setter: + name: replicas + value: "foo" + enumValues: + foo: bar + baz: biz + io.k8s.cli.setters.no-match-2': + x-k8s-cli: + setter: + name: no-match-2 + value: "2" + setBy: "package-default" + `, + err: "hello does not match the possible values for replicas: [foo,baz]", + }, { name: "error", setter: "replicas", diff --git a/kyaml/setters2/types.go b/kyaml/setters2/types.go index bd8f62a53..933c5f58e 100644 --- a/kyaml/setters2/types.go +++ b/kyaml/setters2/types.go @@ -19,8 +19,9 @@ type cliExtension struct { } type setter struct { - Name string `yaml:"name,omitempty" json:"name,omitempty"` - Value string `yaml:"value,omitempty" json:"value,omitempty"` + Name string `yaml:"name,omitempty" json:"name,omitempty"` + Value string `yaml:"value,omitempty" json:"value,omitempty"` + EnumValues map[string]string `yaml:"enumValues,omitempty" json:"enumValues,omitempty"` } type substitution struct { From 3c776b34351630acba5cd804253107f13f32d826 Mon Sep 17 00:00:00 2001 From: Phillip Wittrock Date: Wed, 26 Feb 2020 20:17:54 -0800 Subject: [PATCH 10/12] fix setter clear set-by test --- cmd/config/internal/commands/cmdset_test.go | 1 - kyaml/setters2/add.go | 6 +++++- kyaml/setters2/set.go | 16 ++++++++++++++-- 3 files changed, 19 insertions(+), 4 deletions(-) diff --git a/cmd/config/internal/commands/cmdset_test.go b/cmd/config/internal/commands/cmdset_test.go index f617660b7..289bcfa95 100644 --- a/cmd/config/internal/commands/cmdset_test.go +++ b/cmd/config/internal/commands/cmdset_test.go @@ -109,7 +109,6 @@ openAPI: setter: name: replicas value: "4" - setBy: me `, expectedResources: ` apiVersion: apps/v1 diff --git a/kyaml/setters2/add.go b/kyaml/setters2/add.go index c5f3fb8e9..d082b5d58 100644 --- a/kyaml/setters2/add.go +++ b/kyaml/setters2/add.go @@ -106,7 +106,11 @@ type SetterDefinition struct { Count int `yaml:"count,omitempty"` // EnumValues is a map of possible setter values to actual field values. - // May be used for t-shirt sizing values -- e.g. set cpu to small, medium, large + // If EnumValues is specified, then the value set the by user 1) MUST + // be present in the enumValues map as a key, and 2) the map entry value + // MUST be used as the value to set in the configuration (rather than the key) + // Example -- may be used for t-shirt sizing values by allowing cpu to be + // set to small, medium or large, and then mapping these values to cpu values -- 0.5, 2, 8 EnumValues map[string]string `yaml:"enumValues,omitempty"` } diff --git a/kyaml/setters2/set.go b/kyaml/setters2/set.go index a3bc305b1..fdff66dfe 100644 --- a/kyaml/setters2/set.go +++ b/kyaml/setters2/set.go @@ -88,7 +88,8 @@ func (s *Set) substitute(field *yaml.RNode, ext *cliExtension) (bool, error) { } if val, found := subSetter.Setter.EnumValues[subSetter.Setter.Value]; found { - // resolve enum for substitution + // the setter has an enum-map. we should replace the marker with the + // enum value looked up from the map rather than the enum key p = strings.ReplaceAll(p, v.Marker, val) } else { // substitute the setters current value into the substitution pattern @@ -121,6 +122,8 @@ func (s *Set) set(field *yaml.RNode, ext *cliExtension) bool { // TODO(pwittrock): validate the field value if val, found := ext.Setter.EnumValues[ext.Setter.Value]; found { + // the setter has an enum-map. we should replace the marker with the + // enum value looked up from the map rather than the enum key field.YNode().Value = val return true } @@ -158,22 +161,31 @@ func (s SetOpenAPI) Filter(object *yaml.RNode) (*yaml.RNode, error) { return nil, errors.Errorf("no setter %s found", s.Name) } - // if the setter is an enumeration, then make sure the value matches + // if the setter contains an enumValues map, then ensure the set value appears + // as a key in the map if values, err := def.Pipe(yaml.Lookup("enumValues")); err != nil { + // error looking up the enumValues return nil, err } else if values != nil { + // contains enumValues map -- validate the set value against the map entries + + // get the enumValues keys fields, err := values.Fields() if err != nil { return nil, err } + + // search for the user provided value in the set of allowed values var match bool for i := range fields { if fields[i] == s.Value { + // found a match, we are good match = true break } } if !match { + // no match found -- provide an informative error to the user return nil, errors.Errorf("%s does not match the possible values for %s: [%s]", s.Value, s.Name, strings.Join(fields, ",")) } From da548f65ea93c3ce21fbecbe079918b565b4ffbb Mon Sep 17 00:00:00 2001 From: Phillip Wittrock Date: Thu, 27 Feb 2020 11:48:24 -0800 Subject: [PATCH 11/12] fixup go.sum --- cmd/config/go.sum | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/cmd/config/go.sum b/cmd/config/go.sum index a767e7439..a499dd06c 100644 --- a/cmd/config/go.sum +++ b/cmd/config/go.sum @@ -156,8 +156,10 @@ gopkg.in/yaml.v2 v2.2.1/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= gopkg.in/yaml.v2 v2.2.2/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= gopkg.in/yaml.v2 v2.2.4 h1:/eiJrUcujPVeJ3xlSWaiNi3uSVmDGBK1pDHUHAnao1I= gopkg.in/yaml.v2 v2.2.4/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= -gopkg.in/yaml.v3 v3.0.0-20191026110619-0b21df46bc1d h1:LCPbGQ34PMrwad11aMZ+dbz5SAsq/0ySjRwQ8I9Qwd8= -gopkg.in/yaml.v3 v3.0.0-20191026110619-0b21df46bc1d/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= +gopkg.in/yaml.v2 v2.2.7 h1:VUgggvou5XRW9mHwD/yXxIYSMtY0zoKQf/v226p2nyo= +gopkg.in/yaml.v2 v2.2.7/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= +gopkg.in/yaml.v3 v3.0.0-20191120175047-4206685974f2 h1:XZx7nhd5GMaZpmDaEHFVafUZC7ya0fuo7cSJ3UCKYmM= +gopkg.in/yaml.v3 v3.0.0-20191120175047-4206685974f2/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= k8s.io/apimachinery v0.17.0 h1:xRBnuie9rXcPxUkDizUsGvPf1cnlZCFu210op7J7LJo= k8s.io/apimachinery v0.17.0/go.mod h1:b9qmWdKlLuU9EBh+06BtLcSf/Mu89rWL33naRxs1uZg= k8s.io/gengo v0.0.0-20190128074634-0689ccc1d7d6/go.mod h1:ezvh/TsK7cY6rbqRK0oQQ8IAqLxYwwyPxAX1Pzy0ii0= From fa507f782f4e331e16b6987afd2b1d64d6fd094c Mon Sep 17 00:00:00 2001 From: Phillip Wittrock Date: Thu, 27 Feb 2020 11:49:59 -0800 Subject: [PATCH 12/12] Setters: support for explicit setter typing - ensure OpenAPI definitions always uses strings for setter values - allow the field type to be defined -- integer,boolean,string - format values using yaml 1.1 compatibility --- .../internal/commands/cmdcreatesetter.go | 4 +- kyaml/setters2/add.go | 12 ++ kyaml/setters2/set.go | 26 +++- kyaml/setters2/set_test.go | 126 +++++++++++++++++- kyaml/setters2/settersutil/settercreator.go | 6 +- kyaml/setters2/types.go | 14 +- 6 files changed, 167 insertions(+), 21 deletions(-) diff --git a/cmd/config/internal/commands/cmdcreatesetter.go b/cmd/config/internal/commands/cmdcreatesetter.go index 9bcbe03d0..ec4297eb3 100644 --- a/cmd/config/internal/commands/cmdcreatesetter.go +++ b/cmd/config/internal/commands/cmdcreatesetter.go @@ -38,8 +38,7 @@ func NewCreateSetterRunner(parent string) *CreateSetterRunner { "kind of the Resource on which to create the setter.") set.Flags().MarkHidden("kind") set.Flags().StringVar(&r.Set.SetPartialField.Type, "type", "", - "valid OpenAPI field type -- e.g. integer,boolean,string.") - set.Flags().MarkHidden("type") + "OpenAPI field type for the setter -- e.g. integer,boolean,string.") set.Flags().BoolVar(&r.Set.SetPartialField.Partial, "partial", false, "create a partial setter for only part of the field value.") set.Flags().MarkHidden("partial") @@ -89,6 +88,7 @@ func (r *CreateSetterRunner) preRunE(c *cobra.Command, args []string) error { r.OpenAPIFile, err = ext.GetOpenAPIFile(args) r.CreateSetter.Description = r.Set.SetPartialField.Description r.CreateSetter.SetBy = r.Set.SetPartialField.SetBy + r.CreateSetter.Type = r.Set.SetPartialField.Type if err != nil { return err } diff --git a/kyaml/setters2/add.go b/kyaml/setters2/add.go index d082b5d58..83575c070 100644 --- a/kyaml/setters2/add.go +++ b/kyaml/setters2/add.go @@ -105,6 +105,9 @@ type SetterDefinition struct { // Count is the number of fields set by this setter. Count int `yaml:"count,omitempty"` + // Type is the type of the setter value. + Type string `yaml:"type,omitempty"` + // EnumValues is a map of possible setter values to actual field values. // If EnumValues is specified, then the value set the by user 1) MUST // be present in the enumValues map as a key, and 2) the map entry value @@ -135,6 +138,15 @@ func (sd SetterDefinition) Filter(object *yaml.RNode) (*yaml.RNode, error) { sd.Description = "" } + if sd.Type != "" { + err = def.PipeE(yaml.FieldSetter{Name: "type", StringValue: sd.Type}) + if err != nil { + return nil, err + } + // don't write the type to the extension + sd.Type = "" + } + ext, err := def.Pipe(yaml.LookupCreate(yaml.MappingNode, K8sCliExtensionKey)) if err != nil { return nil, err diff --git a/kyaml/setters2/set.go b/kyaml/setters2/set.go index fdff66dfe..b75f61d56 100644 --- a/kyaml/setters2/set.go +++ b/kyaml/setters2/set.go @@ -30,7 +30,7 @@ func (s *Set) Filter(object *yaml.RNode) (*yaml.RNode, error) { // visitScalar func (s *Set) visitScalar(object *yaml.RNode, p string) error { // get the openAPI for this field describing how to apply the setter - ext, err := getExtFromComment(object) + ext, sch, err := getExtFromComment(object) if err != nil { return err } @@ -39,13 +39,13 @@ func (s *Set) visitScalar(object *yaml.RNode, p string) error { } // perform a direct set of the field if it matches - if s.set(object, ext) { + if s.set(object, ext, sch) { s.Count++ return nil } // perform a substitution of the field if it matches - sub, err := s.substitute(object, ext) + sub, err := s.substitute(object, ext, sch) if err != nil { return err } @@ -57,7 +57,7 @@ func (s *Set) visitScalar(object *yaml.RNode, p string) error { // substitute updates the value of field from ext if ext contains a substitution that // depends on a setter whose name matches s.Name. -func (s *Set) substitute(field *yaml.RNode, ext *cliExtension) (bool, error) { +func (s *Set) substitute(field *yaml.RNode, ext *cliExtension, _ *spec.Schema) (bool, error) { nameMatch := false // check partial setters to see if they contain the setter as part of a @@ -109,11 +109,15 @@ func (s *Set) substitute(field *yaml.RNode, ext *cliExtension) (bool, error) { // TODO(pwittrock): validate the field value field.YNode().Value = p + + // substitutions are always strings + field.YNode().Tag = "!!str" + return true, nil } // set applies the value from ext to field if its name matches s.Name -func (s *Set) set(field *yaml.RNode, ext *cliExtension) bool { +func (s *Set) set(field *yaml.RNode, ext *cliExtension, sch *spec.Schema) bool { // check full setter if ext.Setter == nil || ext.Setter.Name != s.Name { return false @@ -130,6 +134,9 @@ func (s *Set) set(field *yaml.RNode, ext *cliExtension) bool { // this has a full setter, set its value field.YNode().Value = ext.Setter.Value + + // format the node so it is quoted if it is a string + yaml.FormatNonStringStyle(field.YNode(), *sch) return true } @@ -191,7 +198,14 @@ func (s SetOpenAPI) Filter(object *yaml.RNode) (*yaml.RNode, error) { } } - if err := def.PipeE(&yaml.FieldSetter{Name: "value", StringValue: s.Value}); err != nil { + v := yaml.NewScalarRNode(s.Value) + // values are always represented as strings the OpenAPI + // since the are unmarshalled into strings. Use double quote style to + // ensure this consistently. + v.YNode().Tag = "!!str" + v.YNode().Style = yaml.DoubleQuotedStyle + + if err := def.PipeE(&yaml.FieldSetter{Name: "value", Value: v}); err != nil { return nil, err } diff --git a/kyaml/setters2/set_test.go b/kyaml/setters2/set_test.go index 705df8c01..3414cf10d 100644 --- a/kyaml/setters2/set_test.go +++ b/kyaml/setters2/set_test.go @@ -15,11 +15,12 @@ import ( func TestSet_Filter(t *testing.T) { var tests = []struct { - name string - setter string - openapi string - input string - expected string + name string + description string + setter string + openapi string + input string + expected string }{ { name: "set-replicas", @@ -58,6 +59,98 @@ metadata: name: nginx-deployment spec: replicas: 4 # {"$ref": "#/definitions/io.k8s.cli.setters.replicas"} + `, + }, + { + name: "set-foo-type", + description: "if a type is specified for a setter, ensure the field is properly quoted", + setter: "foo", + openapi: ` +openAPI: + definitions: + io.k8s.cli.setters.foo: + x-k8s-cli: + setter: + name: foo + value: "4" + type: string + `, + input: ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment + annotations: + foo: 3 # {"$ref": "#/definitions/io.k8s.cli.setters.foo"} + `, + expected: ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment + annotations: + foo: "4" # {"$ref": "#/definitions/io.k8s.cli.setters.foo"} + `, + }, + { + name: "set-foo-type-wrong", + description: "if a type is specified for a setter, for the field to the type", + setter: "foo", + openapi: ` +openAPI: + definitions: + io.k8s.cli.setters.foo: + x-k8s-cli: + setter: + name: foo + value: "4" + type: boolean + `, + input: ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment + annotations: + foo: 3 # {"$ref": "#/definitions/io.k8s.cli.setters.foo"} + `, + expected: ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment + annotations: + foo: !!bool 4 # {"$ref": "#/definitions/io.k8s.cli.setters.foo"} + `, + }, + { + name: "set-foo-no-type", + description: "if a type is not specified for a setter, keep the existing quoting", + setter: "foo", + openapi: ` +openAPI: + definitions: + io.k8s.cli.setters.foo: + x-k8s-cli: + setter: + name: foo + value: "4" + `, + input: ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment + annotations: + foo: 3 # {"$ref": "#/definitions/io.k8s.cli.setters.foo"} + `, + expected: ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment + annotations: + foo: 4 # {"$ref": "#/definitions/io.k8s.cli.setters.foo"} `, }, { @@ -632,6 +725,29 @@ openAPI: setter: name: no-match-2 value: "2" +`, + }, + { + name: "set-annotation-quoted", + setter: "replicas", + value: "3", + input: ` +openAPI: + definitions: + io.k8s.cli.setters.replicas: + x-k8s-cli: + setter: + name: replicas + value: 4 + `, + expected: ` +openAPI: + definitions: + io.k8s.cli.setters.replicas: + x-k8s-cli: + setter: + name: replicas + value: "3" `, }, { diff --git a/kyaml/setters2/settersutil/settercreator.go b/kyaml/setters2/settersutil/settercreator.go index 121be30ce..8b6221335 100644 --- a/kyaml/setters2/settersutil/settercreator.go +++ b/kyaml/setters2/settersutil/settercreator.go @@ -19,6 +19,8 @@ type SetterCreator struct { SetBy string + Type string + // FieldName if set will add the OpenAPI reference to fields with this name or path // FieldName may be the full name of the field, full path to the field, or the path suffix. // e.g. all of the following would match spec.template.spec.containers.image -- @@ -35,7 +37,9 @@ type SetterCreator struct { func (c SetterCreator) Create(openAPIPath, resourcesPath string) error { // Update the OpenAPI definitions to hace the setter sd := setters2.SetterDefinition{ - Name: c.Name, Value: c.FieldValue, Description: c.Description, SetBy: c.SetBy} + Name: c.Name, Value: c.FieldValue, Description: c.Description, SetBy: c.SetBy, + Type: c.Type, + } if err := sd.AddToFile(openAPIPath); err != nil { return err } diff --git a/kyaml/setters2/types.go b/kyaml/setters2/types.go index 933c5f58e..b896a2cd7 100644 --- a/kyaml/setters2/types.go +++ b/kyaml/setters2/types.go @@ -57,32 +57,32 @@ func getExtFromSchema(schema *spec.Schema) (*cliExtension, error) { // getExtFromComment returns the cliExtension openAPI extension if it is present as // a comment on the field. -func getExtFromComment(object *yaml.RNode) (*cliExtension, error) { +func getExtFromComment(object *yaml.RNode) (*cliExtension, *spec.Schema, error) { // TODO(pwittrock): also use path to the field to get openapi, not just comments // parse comment containing the extended openapi for this field fm := fieldmeta.FieldMeta{} if err := fm.Read(object); err != nil { - return nil, errors.Wrap(err) + return nil, nil, errors.Wrap(err) } if fm.Schema.Ref.String() == "" { - return nil, nil + return nil, nil, nil } // resolve the comment reference to the extended openapi definitions r, err := openapi.Resolve(&fm.Schema.Ref) if err != nil { - return nil, errors.Wrap(err) + return nil, nil, errors.Wrap(err) } if r == nil { // no schema found // TODO(pwittrock): should this be an error if it doesn't resolve? - return nil, nil + return nil, nil, nil } // get the cli extension from the openapi (contains setter information) ext, err := getExtFromSchema(r) if err != nil { - return nil, errors.Wrap(err) + return nil, nil, errors.Wrap(err) } - return ext, nil + return ext, r, nil }