Function configs must have dedicated file (#434)

* Function configs must have dedicated file

* Suggested changes

* Fix empty value cases and old setter comments for array nodes

Author: phanimarupaka

examples/fix-simple/.expected/diff.patch

@@ -1,5 +1,5 @@
 diff --git a/Kptfile b/Kptfile
-index a8e7f27..0d91da2 100644
+index a8e7f27..cccb738 100644
 --- a/Kptfile
 +++ b/Kptfile
 @@ -1,20 +1,24 @@
@@ -37,7 +37,7 @@ index a8e7f27..0d91da2 100644
 +pipeline:
 +  mutators:
 +    - image: gcr.io/kpt-fn/apply-setters:v0.1
-+      configPath: setters-config.yaml
++      configPath: setters.yaml
 diff --git a/resources.yaml b/resources.yaml
 index 9e30767..dae3005 100644
 --- a/resources.yaml
@@ -50,15 +50,15 @@ index 9e30767..dae3005 100644
 +  name: the-map # kpt-set: ${name}
  data:
    some-key: some-value
-diff --git a/setters-config.yaml b/setters-config.yaml
+diff --git a/setters.yaml b/setters.yaml
 new file mode 100644
-index 0000000..f39bb00
+index 0000000..b67bc21
 --- /dev/null
-+++ b/setters-config.yaml
++++ b/setters.yaml
 @@ -0,0 +1,6 @@
 +apiVersion: v1
 +kind: ConfigMap
 +metadata:
-+  name: setters-config
++  name: setters
 +data:
 +  name: the-map

examples/fix-simple/README.md

@@ -83,7 +83,7 @@ info:
 pipeline:
   mutators:
     - image: gcr.io/kpt-fn/apply-setters:v0.1
-      configPath: setters-config.yaml
+      configPath: setters.yaml
 ```
 
 The transformed package is compatible with kpt 1.0 binary.

functions/go/fix/fixpkg/fix.go

@@ -14,6 +14,7 @@ import (
 	"sigs.k8s.io/kustomize/kyaml/fieldmeta"
 	"sigs.k8s.io/kustomize/kyaml/fn/runtime/runtimeutil"
 	"sigs.k8s.io/kustomize/kyaml/kio"
+	"sigs.k8s.io/kustomize/kyaml/kio/filters"
 	"sigs.k8s.io/kustomize/kyaml/kio/kioutil"
 	"sigs.k8s.io/kustomize/kyaml/openapi"
 	"sigs.k8s.io/kustomize/kyaml/sets"
@@ -55,7 +56,7 @@ type Result struct {
 	Message string
 }
 
-const SettersConfigFileName = "setters-config.yaml"
+const SettersConfigFileName = "setters.yaml"
 
 // Filter implements Fix as a yaml.Filter
 func (s *Fix) Filter(nodes []*yaml.RNode) ([]*yaml.RNode, error) {
@@ -230,15 +231,18 @@ func (s *Fix) FunctionsInPkg(nodes []*yaml.RNode, pkgPath string) []v1.Function
 		}
 		fnSpec := runtimeutil.GetFunctionSpec(node)
 		if fnSpec != nil {
+			// in order to make sure there is only one function config per file,
+			// rename the file with the name(and hash(name)) of the function config resource
+			fnFileName := fmt.Sprintf("%s.yaml", node.GetName())
 			// in v1, fn-config must be present in the package directory
 			// so configPath must be just the file name
-			fnFileName := filepath.Base(meta.Annotations[kioutil.PathAnnotation])
+			fnFilePath := filepath.Join(pkgPath, fnFileName)
 			res = append(res, v1.Function{
 				Image:      fnSpec.Container.Image,
 				ConfigPath: fnFileName,
 			})
 			// move the fn-config to the top level directory of the package
-			meta.Annotations[kioutil.PathAnnotation] = filepath.Join(pkgPath, fnFileName)
+			meta.Annotations[kioutil.PathAnnotation] = fnFilePath
 			delete(meta.Annotations, runtimeutil.FunctionAnnotationKey)
 			delete(meta.Annotations, "config.k8s.io/function")
 			err = node.SetAnnotations(meta.Annotations)
@@ -278,7 +282,7 @@ func (s *Fix) FixKptfile(node *yaml.RNode, functions []v1.Function) (*yaml.RNode
 		if kf.Pipeline != nil {
 			for i, fn := range kf.Pipeline.Mutators {
 				if strings.Contains(fn.Image, "apply-setters:v0.1") && len(fn.ConfigMap) > 0 {
-					settersConfig, err := ConfigFromSetters(kf.Pipeline.Mutators[0].ConfigMap, settersConfigFilePath)
+					settersConfig, err := SettersNodeFromSetters(kf.Pipeline.Mutators[0].ConfigMap, settersConfigFilePath)
 					if err != nil {
 						return node, err
 					}
@@ -392,9 +396,9 @@ func (s *Fix) FixKptfile(node *yaml.RNode, functions []v1.Function) (*yaml.RNode
 	if len(setters) > 0 {
 		fn := v1.Function{
 			Image:      "gcr.io/kpt-fn/apply-setters:v0.1",
-			ConfigPath: "setters-config.yaml",
+			ConfigPath: SettersConfigFileName,
 		}
-		settersConfig, err := ConfigFromSetters(setters, settersConfigFilePath)
+		settersConfig, err := SettersNodeFromSetters(setters, settersConfigFilePath)
 		if err != nil {
 			return node, err
 		}
@@ -431,12 +435,12 @@ func (s *Fix) FixKptfile(node *yaml.RNode, functions []v1.Function) (*yaml.RNode
 	return kNode, err
 }
 
-// ConfigFromSetters returns the ConfigMap node with input setters in data field
-func ConfigFromSetters(setters map[string]string, path string) (*yaml.RNode, error) {
+// SettersNodeFromSetters returns the ConfigMap node with input setters in data field
+func SettersNodeFromSetters(setters map[string]string, path string) (*yaml.RNode, error) {
 	configNode, err := yaml.Parse(fmt.Sprintf(`apiVersion: v1
 kind: ConfigMap
 metadata:
-  name: setters-config
+  name: setters
   annotations:
     %s: %s
     %s: "0"
@@ -452,28 +456,48 @@ metadata:
 	sort.Strings(keys)
 
 	for _, k := range keys {
-		v := setters[k]
-		vNode, err := yaml.Parse(v)
+		v, err := standardizeArrayValue(setters[k])
 		if err != nil {
 			return nil, err
 		}
-		if vNode.YNode().Kind == yaml.SequenceNode {
-			// standardize array values to folded style
-			vNode.YNode().Style = yaml.FoldedStyle
-			v, err = vNode.String()
-			if err != nil {
-				return nil, err
-			}
+		vNode := yaml.NewScalarRNode(v)
+		if v == "" {
+			vNode.YNode().Style = yaml.DoubleQuotedStyle
 		}
-
 		err = configNode.PipeE(
 			yaml.LookupCreate(yaml.ScalarNode, "data", k),
-			yaml.FieldSetter{Value: yaml.NewScalarRNode(v)})
+			yaml.FieldSetter{Value: vNode, OverrideStyle: true})
 		if err != nil {
 			return nil, err
 		}
 	}
-	return configNode, nil
+	// format the created resource
+	_, err = filters.FormatFilter{UseSchema: true}.Filter([]*yaml.RNode{configNode})
+	return configNode, err
+}
+
+// standardizeArrayValue returns the folded style array node string
+// e.g. for input value [foo, bar], it returns
+// - foo
+// - bar
+func standardizeArrayValue(val string) (string, error) {
+	if !strings.HasPrefix(val, "[") {
+		// the value is either standardized, or is not a sequence node value
+		return val, nil
+	}
+	vNode, err := yaml.Parse(val)
+	if err != nil {
+		return val, fmt.Errorf("failed to parse the array node value %q with error %q", val, err.Error())
+	}
+	if vNode.YNode().Kind == yaml.SequenceNode {
+		// standardize array values to folded style
+		vNode.YNode().Style = yaml.FoldedStyle
+		val, err = vNode.String()
+		if err != nil {
+			return val, fmt.Errorf("failed to serialize the array node value %q with error %q", val, err.Error())
+		}
+	}
+	return val, nil
 }
 
 // getPkgPathToSettersSchema returns the, map of pkgPath to the setters schema in Kptfile
@@ -507,13 +531,16 @@ func (s *Fix) visitMapping(object *yaml.RNode) error {
 			// return if it is not a sequence node
 			return nil
 		}
-
+		oldCommentPrefixes := []string{`# {"$kpt-set":"`, `# {"$ref":"#/definitions/io.k8s.cli.`}
 		comment := node.Key.YNode().LineComment
-		// # {"$kpt-set":"foo"} must be converted to # kpt-set: ${foo}
-		if strings.Contains(comment, "$kpt-set") {
-			comment := strings.TrimPrefix(comment, `# {"$kpt-set":"`)
-			comment = strings.TrimSuffix(comment, `"}`)
-			node.Key.YNode().LineComment = fmt.Sprintf("kpt-set: ${%s}", comment)
+		for _, cp := range oldCommentPrefixes {
+			// # {"$kpt-set":"foo"} must be converted to # kpt-set: ${foo}
+			// # {"$ref":"#/definitions/io.k8s.cli.list"} must be converted to # kpt-set: ${list}
+			if strings.Contains(comment, cp) {
+				comment := strings.TrimPrefix(comment, cp)
+				comment = strings.TrimSuffix(comment, `"}`)
+				node.Key.YNode().LineComment = fmt.Sprintf("kpt-set: ${%s}", comment)
+			}
 		}
 		return nil
 	})

functions/go/fix/fixpkg/fix_test.go

@@ -37,6 +37,8 @@ func TestFixV1alpha1ToV1(t *testing.T) {
   message: Transformed "packageMetadata" to "info"
 - filepath: Kptfile
   message: Transformed "upstream" to "upstream" and "upstreamLock"
+- filepath: Kptfile
+  message: Added "gcr.io/kpt-fn/set-annotations:v0.1" to mutators list, please move it to validators section if it is a validator function
 - filepath: Kptfile
   message: Added "gcr.io/kpt-fn/set-labels:v0.1" to mutators list, please move it to validators section if it is a validator function
 - filepath: Kptfile
@@ -78,11 +80,86 @@ func TestFixV1alpha2ToV1(t *testing.T) {
 	assert.NoError(t, err)
 	assert.Equal(t, `- filepath: Kptfile
   message: Updated apiVersion to kpt.dev/v1
-- filepath: setters-config.yaml
+- filepath: setters.yaml
   message: Moved setters from configMap to configPath
 - filepath: hello-world/Kptfile
   message: Updated apiVersion to kpt.dev/v1
-- filepath: hello-world/setters-config.yaml
+- filepath: hello-world/setters.yaml
   message: Moved setters from configMap to configPath
 `, string(results))
 }
+
+type settersNodeTest struct {
+	name     string
+	setters  map[string]string
+	path     string
+	expected string
+	errMsg   string
+}
+
+var settersNodeFromSettersCases = []settersNodeTest{
+	{
+		name: "Create setters file with all types",
+		path: `foo/bar`,
+		setters: map[string]string{
+			"environment":         "",
+			"integer":             "10",
+			"number":              "1.1",
+			"boolean":             "true",
+			"string":              "foo",
+			"region":              "us-east-1",
+			"flow-style-setter":   "[dev, prod]",
+			"folded-style-setter": "- hi\n- hello",
+		},
+		expected: `apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: setters
+  annotations:
+    config.kubernetes.io/index: "0"
+    config.kubernetes.io/path: foo/bar
+data:
+  boolean: "true"
+  environment: ""
+  flow-style-setter: |
+    - dev
+    - prod
+  folded-style-setter: |-
+    - hi
+    - hello
+  integer: "10"
+  number: "1.1"
+  region: us-east-1
+  string: foo
+`,
+	},
+	{
+		name: "invalid flow-style sequence node",
+		path: `foo/bar`,
+		setters: map[string]string{
+			"setter": "[dev, prod,",
+		},
+		errMsg: `failed to parse the array node value "[dev, prod," with error "yaml: line 1: did not find expected node content"`,
+	},
+}
+
+func TestSettersNodeFromSetters(t *testing.T) {
+	for i := range settersNodeFromSettersCases {
+		test := settersNodeFromSettersCases[i]
+		t.Run(test.name, func(t *testing.T) {
+			res, err := SettersNodeFromSetters(test.setters, test.path)
+			if test.errMsg == "" {
+				assert.NoError(t, err)
+			} else {
+				assert.Error(t, err)
+				assert.Equal(t, test.errMsg, err.Error())
+				return
+			}
+			actual, err := res.String()
+			assert.NoError(t, err)
+			if !assert.Equal(t, test.expected, actual) {
+				t.FailNow()
+			}
+		})
+	}
+}

testdata/fix/nginx-v1/Kptfile

@@ -29,9 +29,11 @@ info:
 pipeline:
   mutators:
     - image: gcr.io/kpt-fn/apply-setters:v0.1
-      configPath: setters-config.yaml
+      configPath: setters.yaml
+    - image: gcr.io/kpt-fn/set-annotations:v0.1
+      configPath: my-annotations.yaml
     - image: gcr.io/kpt-fn/set-labels:v0.1
-      configPath: fn-config.yaml
+      configPath: set-labels.yaml
 inventory:
   namespace: some-space
   name: inventory-00933591

testdata/fix/nginx-v1/deployment.yaml

@@ -18,3 +18,6 @@ spec:
       env: # kpt-set: ${list}
         - dev
         - stage
+      clusters: # kpt-set: ${clusters}
+        - cluster1
+        - cluster2

testdata/fix/nginx-v1/hello-world/Kptfile

@@ -21,8 +21,8 @@ info:
 pipeline:
   mutators:
     - image: gcr.io/kpt-fn/apply-setters:v0.1
-      configPath: setters-config.yaml
+      configPath: setters.yaml
     - image: gcr.io/kpt-fn/set-annotations:v0.1
-      configPath: fn-config.yaml
+      configPath: set-annotations.yaml
     - image: gcr.io/kpt-fn/set-namespace:v0.1
-      configPath: ns-config.yaml
+      configPath: set-namespace.yaml

testdata/fix/nginx-v1alpha2/hello-world/fn-config.yaml renamed to testdata/fix/nginx-v1/hello-world/set-annotations.yaml

@@ -1,7 +1,7 @@
 apiVersion: v1
 kind: ConfigMap
 metadata:
-  name: my-config
+  name: set-annotations
   annotations:
     config.kubernetes.io/local-config: "true"
 data:

testdata/fix/nginx-v1/hello-world/ns-config.yaml renamed to testdata/fix/nginx-v1/hello-world/set-namespace.yaml

@@ -1,7 +1,7 @@
 apiVersion: v1
 kind: ConfigMap
 metadata:
-  name: my-config
+  name: set-namespace
   annotations:
     config.kubernetes.io/local-config: "true"
 data:

testdata/fix/nginx-v1/hello-world/setters-config.yaml renamed to testdata/fix/nginx-v1/hello-world/setters.yaml

@@ -1,8 +1,8 @@
 apiVersion: v1
 kind: ConfigMap
 metadata:
-  name: setters-config
+  name: setters
 data:
-  http-port: 80
+  replicas: "5"
+  http-port: "80"
   image-tag: v0.3.0
-  replicas: 5