From fa507f782f4e331e16b6987afd2b1d64d6fd094c Mon Sep 17 00:00:00 2001 From: Phillip Wittrock Date: Thu, 27 Feb 2020 11:49:59 -0800 Subject: [PATCH] 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 }