Merge pull request #2236 from pwittrock/parse

Setters type support

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.v2 v2.2.7 h1:VUgggvou5XRW9mHwD/yXxIYSMtY0zoKQf/v226p2nyo=
-gopkg.in/yaml.v3 v3.0.0-20191026110619-0b21df46bc1d/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
+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=

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.")
+		"OpenAPI field type for the setter -- e.g. integer,boolean,string.")
-	set.Flags().MarkHidden("type")
 	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
 		}

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

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
 	}
 

kyaml/setters2/set_test.go

@@ -15,11 +15,12 @@ import (
 
 func TestSet_Filter(t *testing.T) {
 	var tests = []struct {
-		name     string
+		name        string
-		setter   string
+		description string
-		openapi  string
+		setter      string
-		input    string
+		openapi     string
-		expected 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"
 `,
 		},
 		{

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
 	}

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
 }