From 0f0efe2a4cb990dc60bfa8896a98bdaa67d370c0 Mon Sep 17 00:00:00 2001 From: Phani Teja Marupaka Date: Mon, 15 Jun 2020 17:18:38 -0700 Subject: [PATCH 1/2] Avoid manual step for creating array setters --- .../internal/commands/cmdcreatesetter.go | 10 +- .../internal/commands/cmdcreatesetter_test.go | 19 ++- .../internal/commands/cmdlistsetters.go | 15 ++- .../internal/commands/cmdlistsetters_test.go | 114 +++++++++++++++++- kyaml/setters2/add.go | 40 +++++- kyaml/setters2/set.go | 4 + kyaml/setters2/settersutil/settercreator.go | 32 +++-- kyaml/setters2/walk.go | 9 ++ kyaml/yaml/compatibility_test.go | 2 +- 9 files changed, 219 insertions(+), 26 deletions(-) diff --git a/cmd/config/internal/commands/cmdcreatesetter.go b/cmd/config/internal/commands/cmdcreatesetter.go index f2ea66ab3..b107adea6 100644 --- a/cmd/config/internal/commands/cmdcreatesetter.go +++ b/cmd/config/internal/commands/cmdcreatesetter.go @@ -91,7 +91,9 @@ func (r *CreateSetterRunner) preRunE(c *cobra.Command, args []string) error { } if setterVersion == "" { - if len(args) < 2 || !c.Flag("value").Changed && len(args) < 3 { + if len(args) == 2 && r.Set.SetPartialField.Type == "array" && c.Flag("field").Changed { + setterVersion = "v2" + } else if len(args) < 2 || !c.Flag("value").Changed && len(args) < 3 { setterVersion = "v1" } else if err := initSetterVersion(c, args); err != nil { return err @@ -124,6 +126,12 @@ func (r *CreateSetterRunner) preRunE(c *cobra.Command, args []string) error { r.CreateSetter.Description = r.Set.SetPartialField.Description r.CreateSetter.SetBy = r.Set.SetPartialField.SetBy r.CreateSetter.Type = r.Set.SetPartialField.Type + + if r.CreateSetter.Type == "array" { + // this is just a place holder value, we derive the actual value at + // later point from the field path + r.CreateSetter.FieldValue = "listValues" + } } return nil } diff --git a/cmd/config/internal/commands/cmdcreatesetter_test.go b/cmd/config/internal/commands/cmdcreatesetter_test.go index c584230c6..461a26db5 100644 --- a/cmd/config/internal/commands/cmdcreatesetter_test.go +++ b/cmd/config/internal/commands/cmdcreatesetter_test.go @@ -130,14 +130,16 @@ spec: { name: "add replicas with schema list values", - args: []string{"list", "a", "--description", "hello world", "--set-by", "me", "--type", "array"}, - schema: `{"maxItems": 2, "type": "array", "items": {"type": "string"}}`, + args: []string{"list", "a", "--description", "hello world", "--set-by", "me", "--type", "array", "--field", "spec.list"}, + schema: `{"maxItems": 3, "type": "array", "items": {"type": "string"}}`, input: ` apiVersion: example.com/v1beta1 kind: Example spec: list: - "a" + - "b" + - "c" `, inputOpenAPI: ` apiVersion: v1alpha1 @@ -151,21 +153,26 @@ openAPI: io.k8s.cli.setters.list: items: type: string - maxItems: 2 + maxItems: 3 type: array description: hello world x-k8s-cli: setter: name: list - value: a + listValues: + - a + - b + - c setBy: me `, expectedResources: ` apiVersion: example.com/v1beta1 kind: Example spec: - list: - - "a" # {"$openapi":"list"} + list: # {"$openapi":"list"} + - "a" + - "b" + - "c" `, }, { diff --git a/cmd/config/internal/commands/cmdlistsetters.go b/cmd/config/internal/commands/cmdlistsetters.go index 273d9c671..cb84a21a3 100644 --- a/cmd/config/internal/commands/cmdlistsetters.go +++ b/cmd/config/internal/commands/cmdlistsetters.go @@ -113,17 +113,20 @@ func (r *ListSettersRunner) ListSubstitutions(c *cobra.Command, args []string) e return err } table := newTable(c.OutOrStdout(), r.Markdown) - table.SetHeader([]string{"SUBSTITUTION", "PATTERN", "SETTERS"}) + table.SetHeader([]string{"SUBSTITUTION", "PATTERN", "REFERENCES"}) for i := range r.List.Substitutions { s := r.List.Substitutions[i] - setters := "" + refs := "" for _, value := range s.Values { - setter := strings.TrimPrefix(value.Ref, fieldmeta.DefinitionsPrefix+fieldmeta.SetterDefinitionPrefix) - setters = setters + "," + setter + // trim setter and substitution prefixes + ref := strings.TrimPrefix( + strings.TrimPrefix(value.Ref, fieldmeta.DefinitionsPrefix+fieldmeta.SetterDefinitionPrefix), + fieldmeta.DefinitionsPrefix+fieldmeta.SubstitutionDefinitionPrefix) + refs = refs + "," + ref } - setters = fmt.Sprintf("[%s]", strings.TrimPrefix(setters, ",")) + refs = fmt.Sprintf("[%s]", strings.TrimPrefix(refs, ",")) table.Append([]string{ - s.Name, s.Pattern, setters}) + s.Name, s.Pattern, refs}) } if len(r.List.Substitutions) == 0 { return nil diff --git a/cmd/config/internal/commands/cmdlistsetters_test.go b/cmd/config/internal/commands/cmdlistsetters_test.go index 77d475a73..c870f73ec 100644 --- a/cmd/config/internal/commands/cmdlistsetters_test.go +++ b/cmd/config/internal/commands/cmdlistsetters_test.go @@ -104,7 +104,7 @@ spec: image nginx me2 hello world 2 2 replicas 3 me1 hello world 1 1 tag 1.7.9 me3 hello world 3 1 - SUBSTITUTION PATTERN SETTERS + SUBSTITUTION PATTERN REFERENCES image IMAGE:TAG [image,tag] `, }, @@ -178,7 +178,7 @@ spec: image nginx me2 hello world 2 3 replicas 3 me1 hello world 1 2 tag 1.7.9 me3 hello world 3 2 - SUBSTITUTION PATTERN SETTERS + SUBSTITUTION PATTERN REFERENCES image IMAGE:TAG [image,tag] `, }, @@ -251,8 +251,116 @@ spec: `, expected: ` NAME VALUE SET BY DESCRIPTION COUNT image nginx me2 hello world 2 3 - SUBSTITUTION PATTERN SETTERS + SUBSTITUTION PATTERN REFERENCES image IMAGE:TAG [image,tag] +`, + }, + + { + name: "list array setter", + openapi: ` +apiVersion: v1alpha1 +kind: Example +openAPI: + definitions: + io.k8s.cli.setters.list: + items: + type: string + maxItems: 3 + type: array + description: hello world + x-k8s-cli: + setter: + name: list + value: List Values + listValues: + - a + - b + - c + setBy: me + `, + input: ` +apiVersion: example.com/v1beta1 +kind: Example +metadata: + annotations: + foo: bar +spec: + list: # {"$openapi":"list"} + - "a" + - "b" + - "c" +`, + expected: ` NAME VALUE SET BY DESCRIPTION COUNT + list [a,b,c] me hello world 1 +`, + }, + + { + name: "nested substitution", + input: ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment +spec: + replicas: 3 + template: + spec: + containers: + - name: nginx + image: something/nginx::1.7.9/nginxotherthing # {"$openapi":"my-nested-subst"} + - name: sidecar + image: nginx::1.7.9 # {"$openapi":"my-image-subst"} + `, + openapi: ` +apiVersion: v1alpha1 +kind: Example +openAPI: + definitions: + io.k8s.cli.setters.my-image-setter: + x-k8s-cli: + setter: + name: my-image-setter + value: nginx + io.k8s.cli.setters.my-tag-setter: + x-k8s-cli: + setter: + name: my-tag-setter + value: 1.7.9 + io.k8s.cli.substitutions.my-image-subst: + x-k8s-cli: + substitution: + name: my-image-subst + pattern: ${my-image-setter}::${my-tag-setter} + values: + - marker: ${my-image-setter} + ref: '#/definitions/io.k8s.cli.setters.my-image-setter' + - marker: ${my-tag-setter} + ref: '#/definitions/io.k8s.cli.setters.my-tag-setter' + io.k8s.cli.substitutions.my-nested-subst: + x-k8s-cli: + substitution: + name: my-nested-subst + pattern: something/${my-image-subst}/${my-other-setter} + values: + - marker: ${my-image-subst} + ref: '#/definitions/io.k8s.cli.substitutions.my-image-subst' + - marker: ${my-other-setter} + ref: '#/definitions/io.k8s.cli.setters.my-other-setter' + io.k8s.cli.setters.my-other-setter: + x-k8s-cli: + setter: + name: my-other-setter + value: nginxotherthing + `, + expected: ` NAME VALUE SET BY DESCRIPTION COUNT + my-image-setter nginx 2 + my-other-setter nginxotherthing 1 + my-tag-setter 1.7.9 2 + SUBSTITUTION PATTERN REFERENCES + my-image-subst ${my-image-setter}::${my-tag-setter} [my-image-setter,my-tag-setter] + my-nested-subst something/${my-image-subst}/${my-other-setter} [my-image-subst,my-other-setter] `, }, } diff --git a/kyaml/setters2/add.go b/kyaml/setters2/add.go index 8d2e2c940..b7a7bf5a5 100644 --- a/kyaml/setters2/add.go +++ b/kyaml/setters2/add.go @@ -30,6 +30,9 @@ type Add struct { // Ref is the OpenAPI reference to set on the matching fields as a comment. Ref string + + // ListValues are the value of a list setter. + ListValues []string } // Filter implements yaml.Filter @@ -48,6 +51,37 @@ func (a *Add) visitSequence(_ *yaml.RNode, _ string, _ *openapi.ResourceSchema) return nil } +// visitMapping implements visitor +// visitMapping visits the fields in input MappingNode and adds setter/subst ref +// if the path path spec matches with input FiledName +func (a *Add) visitMapping(object *yaml.RNode, p string, _ *openapi.ResourceSchema) error { + return object.VisitFields(func(node *yaml.MapNode) error { + if node.Value.YNode().Kind != yaml.SequenceNode { + return nil + } + + key, err := node.Key.String() + if err != nil { + return err + } + + // derive the list values for the sequence node to write it to openAPI definitions + var values []string + for _, sc := range node.Value.Content() { + values = append(values, sc.Value) + } + a.ListValues = values + + // pathToKey refers to the path address of the key node ex: metadata.annotations + // p is the path till parent node, pathToKey is obtained by appending child key + pathToKey := p + "." + strings.Trim(key, "\n") + if a.FieldName != "" && strings.HasSuffix(pathToKey, a.FieldName) { + return a.addRef(node.Key) + } + return nil + }) +} + // visitScalar implements visitor // visitScalar will set the field metadata on each scalar field whose name + value match func (a *Add) visitScalar(object *yaml.RNode, p string, _ *openapi.ResourceSchema) error { @@ -58,7 +92,11 @@ func (a *Add) visitScalar(object *yaml.RNode, p string, _ *openapi.ResourceSchem if a.FieldValue != "" && a.FieldValue != object.YNode().Value { return nil } + return a.addRef(object) +} +// addRef adds the setter/subst ref to the object node as a line comment +func (a *Add) addRef(object *yaml.RNode) error { // read the field metadata fm := fieldmeta.FieldMeta{} if err := fm.Read(object); err != nil { @@ -85,7 +123,7 @@ type SetterDefinition struct { Name string `yaml:"name"` // Value is the value of the setter. - Value string `yaml:"value"` + Value string `yaml:"value,omitempty"` // ListValues are the value of a list setter. ListValues []string `yaml:"listValues,omitempty"` diff --git a/kyaml/setters2/set.go b/kyaml/setters2/set.go index de8d85e74..ea624bfae 100644 --- a/kyaml/setters2/set.go +++ b/kyaml/setters2/set.go @@ -45,6 +45,10 @@ func (s *Set) isMatch(name string) bool { return s.SetAll || s.Name == name } +func (s *Set) visitMapping(object *yaml.RNode, p string, _ *openapi.ResourceSchema) error { + return nil +} + // visitSequence will perform setters for sequences func (s *Set) visitSequence(object *yaml.RNode, p string, schema *openapi.ResourceSchema) error { ext, err := getExtFromComment(schema) diff --git a/kyaml/setters2/settersutil/settercreator.go b/kyaml/setters2/settersutil/settercreator.go index 9cc12e787..200275a52 100644 --- a/kyaml/setters2/settersutil/settercreator.go +++ b/kyaml/setters2/settersutil/settercreator.go @@ -60,16 +60,32 @@ func (c SetterCreator) Create(openAPIPath, resourcesPath string) error { // Update the resources with the setter reference inout := &kio.LocalPackageReadWriter{PackagePath: resourcesPath} - return kio.Pipeline{ - Inputs: []kio.Reader{inout}, - Filters: []kio.Filter{kio.FilterAll( - &setters2.Add{ - FieldName: c.FieldName, - FieldValue: c.FieldValue, - Ref: fieldmeta.DefinitionsPrefix + fieldmeta.SetterDefinitionPrefix + c.Name, - })}, + a := &setters2.Add{ + FieldName: c.FieldName, + FieldValue: c.FieldValue, + Ref: fieldmeta.DefinitionsPrefix + fieldmeta.SetterDefinitionPrefix + c.Name, + } + err = kio.Pipeline{ + Inputs: []kio.Reader{inout}, + Filters: []kio.Filter{kio.FilterAll(a)}, Outputs: []kio.Writer{inout}, }.Execute() + + if err != nil { + return err + } + + // if the setter is of array type write the derived list values back to + // openAPI definitions + if len(a.ListValues) > 0 { + sd.ListValues = a.ListValues + sd.Value = "" + if err := sd.AddToFile(openAPIPath); err != nil { + return err + } + } + + return nil } // schemaFromFile reads the contents from schemaPath and returns schema diff --git a/kyaml/setters2/walk.go b/kyaml/setters2/walk.go index 786a5b815..6ca698336 100644 --- a/kyaml/setters2/walk.go +++ b/kyaml/setters2/walk.go @@ -23,6 +23,12 @@ type visitor interface { // path is the path to the field // oa is the OpenAPI schema for the field visitSequence(node *yaml.RNode, path string, oa *openapi.ResourceSchema) error + + // visitMapping is called for each Mapping field value on a resource + // node is the mapping field value + // path is the path to the field + // oa is the OpenAPI schema for the field + visitMapping(node *yaml.RNode, path string, oa *openapi.ResourceSchema) error } // accept invokes the appropriate function on v for each field in object @@ -39,6 +45,9 @@ func acceptImpl(v visitor, object *yaml.RNode, p string, oa *openapi.ResourceSch // Traverse the child of the document return accept(v, yaml.NewRNode(object.YNode())) case yaml.MappingNode: + if err := v.visitMapping(object, p, oa); err != nil { + return err + } return object.VisitFields(func(node *yaml.MapNode) error { // get the schema for the field and propagate it oa = getSchema(node.Key, oa, node.Key.YNode().Value) diff --git a/kyaml/yaml/compatibility_test.go b/kyaml/yaml/compatibility_test.go index c2b9aa77f..02e83cbe7 100644 --- a/kyaml/yaml/compatibility_test.go +++ b/kyaml/yaml/compatibility_test.go @@ -23,7 +23,7 @@ func TestIsYaml1_1NonString(t *testing.T) { {val: "2", expected: true}, {val: "true", expected: true}, {val: "1.0\nhello", expected: false}, // multiline strings should always be false - {val: "", expected: false}, // empty string should be considered a string + {val: "", expected: false}, // empty string should be considered a string } for k := range valueToTagMap { From dbb16dcb6d09fff475aa99b1f4607deb9200eea4 Mon Sep 17 00:00:00 2001 From: Phani Teja Marupaka Date: Tue, 16 Jun 2020 11:10:37 -0700 Subject: [PATCH 2/2] Suggested changes --- .../internal/commands/cmdcreatesetter.go | 6 +- .../internal/commands/cmdcreatesetter_test.go | 98 ++++++++++++++++++- .../internal/commands/cmdlistsetters_test.go | 1 - cmd/config/internal/commands/cmdset_test.go | 1 + kyaml/setters2/add.go | 15 ++- kyaml/setters2/settersutil/settercreator.go | 1 + 6 files changed, 115 insertions(+), 7 deletions(-) diff --git a/cmd/config/internal/commands/cmdcreatesetter.go b/cmd/config/internal/commands/cmdcreatesetter.go index b107adea6..04ed314f3 100644 --- a/cmd/config/internal/commands/cmdcreatesetter.go +++ b/cmd/config/internal/commands/cmdcreatesetter.go @@ -128,9 +128,9 @@ func (r *CreateSetterRunner) preRunE(c *cobra.Command, args []string) error { r.CreateSetter.Type = r.Set.SetPartialField.Type if r.CreateSetter.Type == "array" { - // this is just a place holder value, we derive the actual value at - // later point from the field path - r.CreateSetter.FieldValue = "listValues" + if !c.Flag("field").Changed { + return errors.Errorf("field flag must be set for array type setters") + } } } return nil diff --git a/cmd/config/internal/commands/cmdcreatesetter_test.go b/cmd/config/internal/commands/cmdcreatesetter_test.go index 461a26db5..8547fb56b 100644 --- a/cmd/config/internal/commands/cmdcreatesetter_test.go +++ b/cmd/config/internal/commands/cmdcreatesetter_test.go @@ -129,8 +129,101 @@ spec: }, { - name: "add replicas with schema list values", - args: []string{"list", "a", "--description", "hello world", "--set-by", "me", "--type", "array", "--field", "spec.list"}, + name: "list values with schema", + args: []string{"list", "--description", "hello world", "--set-by", "me", "--type", "array", "--field", "spec.list"}, + schema: `{"maxItems": 3, "type": "array", "items": {"type": "string"}}`, + input: ` +apiVersion: example.com/v1beta1 +kind: Example1 +spec: + list: + - "a" + - "b" + - "c" +--- +apiVersion: example.com/v1beta1 +kind: Example2 +spec: + list: + - "a" + - "b" + - "c" + `, + inputOpenAPI: ` +apiVersion: v1alpha1 +kind: Example +`, + expectedOpenAPI: ` +apiVersion: v1alpha1 +kind: Example +openAPI: + definitions: + io.k8s.cli.setters.list: + items: + type: string + maxItems: 3 + type: array + description: hello world + x-k8s-cli: + setter: + name: list + value: "" + listValues: + - a + - b + - c + setBy: me + `, + expectedResources: ` +apiVersion: example.com/v1beta1 +kind: Example1 +spec: + list: # {"$openapi":"list"} + - "a" + - "b" + - "c" +--- +apiVersion: example.com/v1beta1 +kind: Example2 +spec: + list: # {"$openapi":"list"} + - "a" + - "b" + - "c" + `, + }, + + { + name: "error list path with different values", + args: []string{"list", "--description", "hello world", "--set-by", "me", "--type", "array", "--field", "spec.list"}, + schema: `{"maxItems": 3, "type": "array", "items": {"type": "string"}}`, + input: ` +apiVersion: example.com/v1beta1 +kind: Example +spec: + list: + - "a" + - "b" + - "c" +--- +apiVersion: example.com/v1beta1 +kind: Example +spec: + list: + - "c" + - "d" + `, + inputOpenAPI: ` +apiVersion: v1alpha1 +kind: Example +`, + err: `setters can only be created for fields with same values, encountered different ` + + `array values for specified field path: [c d], [a b c]`, + }, + + { + name: "list values error if field not set", + args: []string{"list", "a", "--description", "hello world", "--set-by", "me", "--type", "array"}, schema: `{"maxItems": 3, "type": "array", "items": {"type": "string"}}`, input: ` apiVersion: example.com/v1beta1 @@ -174,6 +267,7 @@ spec: - "b" - "c" `, + err: `field flag must be set for array type setters`, }, { name: "add replicas with value set by flag", diff --git a/cmd/config/internal/commands/cmdlistsetters_test.go b/cmd/config/internal/commands/cmdlistsetters_test.go index c870f73ec..4a1fc20d6 100644 --- a/cmd/config/internal/commands/cmdlistsetters_test.go +++ b/cmd/config/internal/commands/cmdlistsetters_test.go @@ -272,7 +272,6 @@ openAPI: x-k8s-cli: setter: name: list - value: List Values listValues: - a - b diff --git a/cmd/config/internal/commands/cmdset_test.go b/cmd/config/internal/commands/cmdset_test.go index eda1bd982..1191cd392 100644 --- a/cmd/config/internal/commands/cmdset_test.go +++ b/cmd/config/internal/commands/cmdset_test.go @@ -168,6 +168,7 @@ spec: replicas: 4 # {"$ref":"#/definitions/io.k8s.cli.setters.replicas"} `, }, + { name: "set image with value", args: []string{"tag", "1.8.1"}, diff --git a/kyaml/setters2/add.go b/kyaml/setters2/add.go index b7a7bf5a5..678fcaa89 100644 --- a/kyaml/setters2/add.go +++ b/kyaml/setters2/add.go @@ -4,6 +4,7 @@ package setters2 import ( + "reflect" "strings" "github.com/go-openapi/spec" @@ -33,6 +34,9 @@ type Add struct { // ListValues are the value of a list setter. ListValues []string + + // Type is the type of the setter value + Type string } // Filter implements yaml.Filter @@ -70,6 +74,12 @@ func (a *Add) visitMapping(object *yaml.RNode, p string, _ *openapi.ResourceSche for _, sc := range node.Value.Content() { values = append(values, sc.Value) } + + // check if there are different values for field path + if len(a.ListValues) > 0 && !reflect.DeepEqual(values, a.ListValues) { + return errors.Errorf("setters can only be created for fields with same values, "+ + "encountered different array values for specified field path: %s, %s", values, a.ListValues) + } a.ListValues = values // pathToKey refers to the path address of the key node ex: metadata.annotations @@ -86,6 +96,9 @@ func (a *Add) visitMapping(object *yaml.RNode, p string, _ *openapi.ResourceSche // visitScalar will set the field metadata on each scalar field whose name + value match func (a *Add) visitScalar(object *yaml.RNode, p string, _ *openapi.ResourceSchema) error { // check if the field matches + if a.Type == "array" { + return nil + } if a.FieldName != "" && !strings.HasSuffix(p, a.FieldName) { return nil } @@ -123,7 +136,7 @@ type SetterDefinition struct { Name string `yaml:"name"` // Value is the value of the setter. - Value string `yaml:"value,omitempty"` + Value string `yaml:"value"` // ListValues are the value of a list setter. ListValues []string `yaml:"listValues,omitempty"` diff --git a/kyaml/setters2/settersutil/settercreator.go b/kyaml/setters2/settersutil/settercreator.go index 200275a52..e2468922b 100644 --- a/kyaml/setters2/settersutil/settercreator.go +++ b/kyaml/setters2/settersutil/settercreator.go @@ -64,6 +64,7 @@ func (c SetterCreator) Create(openAPIPath, resourcesPath string) error { FieldName: c.FieldName, FieldValue: c.FieldValue, Ref: fieldmeta.DefinitionsPrefix + fieldmeta.SetterDefinitionPrefix + c.Name, + Type: c.Type, } err = kio.Pipeline{ Inputs: []kio.Reader{inout},