New schema for params operator file (#1020)

Co-authored-by: alenkacz <[email protected]>

pkg/apis/kudo/v1beta1/operatorversion_types.go

@@ -82,7 +82,7 @@ type Parameter struct {
 	Description string `json:"description,omitempty"`
 
 	// Required specifies if the parameter is required to be provided by all instances, or whether a default can suffice.
-	Required bool `json:"required,omitempty"`
+	Required *bool `json:"required,omitempty"`
 
 	// Default is a default value if no parameter is provided by the instance.
 	Default *string `json:"default,omitempty"`

pkg/kudoctl/cmd/testdata/index.yaml.golden

@@ -2,7 +2,7 @@ apiVersion: v1
 entries:
   zookeeper:
   - appVersion: 3.4.10
-    digest: 958eeb121727a8df63a37781af4cc9168c812d3f121657fabef9a14e486df106
+    digest: cbacc2e23ef1accc9308acbeff92f1aa8d2ddf712c76f50c114e817fb1a2a166
     maintainers:
     - email: [email protected]
       name: Alena Varkockova

pkg/kudoctl/packages/package.go

@@ -7,7 +7,6 @@ import (
 	"io/ioutil"
 	"log"
 	"regexp"
-	"strconv"
 	"strings"
 
 	"github.com/kudobuilder/kudo/pkg/apis/kudo/v1beta1"
@@ -44,6 +43,10 @@ type PackageFiles struct {
 	Params    []v1beta1.Parameter
 }
 
+type ParametersFile struct {
+	Params []v1beta1.Parameter `json:"parameters"`
+}
+
 // Operator is a representation of the KEP-9 Operator YAML
 type Operator struct {
 	APIVersion        string                  `json:"apiVersion,omitempty"`
@@ -98,44 +101,33 @@ func parsePackageFile(filePath string, fileBytes []byte, currentPackage *Package
 		name := pathParts[len(pathParts)-1]
 		currentPackage.Templates[name] = string(fileBytes)
 	case isParametersFile(filePath):
-		var params map[string]map[string]string
-		if err := yaml.Unmarshal(fileBytes, &params); err != nil {
+		paramsFile, err := readParametersFile(fileBytes)
+		if err != nil {
 			return errors.Wrapf(err, "failed to unmarshal parameters file: %s", filePath)
 		}
-		paramsStruct := make([]v1beta1.Parameter, 0)
-		for paramName, param := range params {
-			required := true // defaults to true
-			if _, ok := param["required"]; ok {
-				parsed, err := strconv.ParseBool(param["required"])
-				if err != nil {
-					// ideally this should never happen and be already caught by some kind of linter
-					return errors.Wrapf(err, "failed parsing required field from parameter %s. cannot convert %s to bool", paramName, param["required"])
-				}
-
-				required = parsed
-			}
-			var defaultValue *string
-			if val, ok := param["default"]; ok {
-				defaultValue = kudo.String(val)
+		currentPackage.Params = make([]v1beta1.Parameter, 0)
+		defaultRequired := true
+		for _, param := range paramsFile.Params {
+			if param.Required == nil {
+				// applying default value of required for all params where not specified
+				param.Required = &defaultRequired
 			}
-
-			r := v1beta1.Parameter{
-				Name:        paramName,
-				Description: param["description"],
-				Default:     defaultValue,
-				Trigger:     param["trigger"],
-				Required:    required,
-				DisplayName: param["displayName"],
-			}
-			paramsStruct = append(paramsStruct, r)
+			currentPackage.Params = append(currentPackage.Params, param)
 		}
-		currentPackage.Params = paramsStruct
 	default:
 		return fmt.Errorf("unexpected file when reading package from filesystem: %s", filePath)
 	}
 	return nil
 }
 
+func readParametersFile(fileBytes []byte) (ParametersFile, error) {
+	paramsFile := ParametersFile{}
+	if err := yaml.Unmarshal(fileBytes, &paramsFile); err != nil {
+		return paramsFile, err
+	}
+	return paramsFile, nil
+}
+
 func newPackageFiles() PackageFiles {
 	return PackageFiles{
 		Templates: make(map[string]string),

pkg/kudoctl/packages/package_test.go

@@ -9,6 +9,8 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
+
 	"github.com/go-test/deep"
 	"github.com/kudobuilder/kudo/pkg/apis/kudo/v1beta1"
 	"github.com/pkg/errors"
@@ -115,3 +117,38 @@ func loadResourcesFromPath(goldenPath string) (*Resources, error) {
 	}
 	return result, nil
 }
+
+func Test_readParametersFile(t *testing.T) {
+	noParams := `
+parameters:
+`
+	param := `
+parameters:
+  - name: example
+`
+	example := make([]v1beta1.Parameter, 1)
+	example[0] = v1beta1.Parameter{Name: "example"}
+
+	bad := `
+parameters:
+	- oops:
+`
+	tests := []struct {
+		name      string
+		fileBytes []byte
+		want      ParametersFile
+		wantErr   bool
+	}{
+		{"no data", []byte{}, ParametersFile{}, false},
+		{"no parameters", []byte(noParams), ParametersFile{}, false},
+		{"parameters", []byte(param), ParametersFile{example}, false},
+		{"bad data", []byte(bad), ParametersFile{}, true},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := readParametersFile(tt.fileBytes)
+			assert.Equal(t, tt.wantErr, err != nil, "readParametersFile() error = %v, wantErr %v", err, tt.wantErr)
+			assert.Equal(t, tt.want, got)
+		})
+	}
+}

pkg/kudoctl/packages/testdata/zk/params.yaml

@@ -1,6 +1,7 @@
-memory:
-  description: Amount of memory to provide to Zookeeper pods
-  default: "1Gi"
-cpus:
-  description: Amount of cpu to provide to Zookeeper pods
-  default: "0.25"
+parameters:
+  - name: memory
+    description: Amount of memory to provide to Zookeeper pods
+    default: "1Gi"
+  - name: cpus
+    description: Amount of cpu to provide to Zookeeper pods
+    default: "0.25"

pkg/kudoctl/util/kudo/install.go

@@ -89,7 +89,7 @@ func validate(resources *packages.Resources, skipInstance bool) error {
 	parameters := resources.OperatorVersion.Spec.Parameters
 	missingParameters := []string{}
 	for _, p := range parameters {
-		if p.Required && p.Default == nil {
+		if *p.Required && p.Default == nil {
 			_, ok := resources.Instance.Spec.Parameters[p.Name]
 			if !ok {
 				missingParameters = append(missingParameters, p.Name)

pkg/kudoctl/util/kudo/install_test.go

@@ -67,19 +67,19 @@ func Test_InstallPackage(t *testing.T) {
 			},
 		},
 	}
-
+	tv := true
 	tests := []struct {
 		name              string
 		parameters        []v1beta1.Parameter
 		installParameters map[string]string
 		skipInstance      bool
 		err               string
 	}{
-		{"all parameters with defaults", []v1beta1.Parameter{{Name: "param", Required: true, Default: util.String("aaa")}}, map[string]string{}, false, ""},
-		{"missing parameter provided", []v1beta1.Parameter{{Name: "param", Required: true}}, map[string]string{"param": "value"}, false, ""},
-		{"missing parameter", []v1beta1.Parameter{{Name: "param", Required: true, Default: nil}}, map[string]string{}, false, "missing required parameters during installation: param"},
-		{"multiple missing parameter", []v1beta1.Parameter{{Name: "param", Required: true}, {Name: "param2", Required: true}}, map[string]string{}, false, "missing required parameters during installation: param,param2"},
-		{"skip instance ignores missing parameter", []v1beta1.Parameter{{Name: "param", Required: true}}, map[string]string{}, true, ""},
+		{"all parameters with defaults", []v1beta1.Parameter{{Name: "param", Required: &tv, Default: util.String("aaa")}}, map[string]string{}, false, ""},
+		{"missing parameter provided", []v1beta1.Parameter{{Name: "param", Required: &tv}}, map[string]string{"param": "value"}, false, ""},
+		{"missing parameter", []v1beta1.Parameter{{Name: "param", Required: &tv, Default: nil}}, map[string]string{}, false, "missing required parameters during installation: param"},
+		{"multiple missing parameter", []v1beta1.Parameter{{Name: "param", Required: &tv}, {Name: "param2", Required: &tv}}, map[string]string{}, false, "missing required parameters during installation: param,param2"},
+		{"skip instance ignores missing parameter", []v1beta1.Parameter{{Name: "param", Required: &tv}}, map[string]string{}, true, ""},
 	}
 
 	for _, tt := range tests {

test/e2e/cli-install-uninstall/first-operator/params.yaml

@@ -1,3 +1,4 @@
-replicas:
-  description: Number of replicas that should be run as part of the deployment
-  default: 2
+parameters:
+  - name: replicas
+    description: Number of replicas that should be run as part of the deployment
+    default: 2

test/integration/cli-install/cli-install-operator/params.yaml

@@ -1,6 +1,7 @@
-memory:
-  description: Amount of memory to provide to Zookeeper pods
-  default: "1Gi"
-cpus:
-  description: Amount of cpu to provide to Zookeeper pods
-  default: "0.25"
+parameters:
+  - name: memory
+    description: Amount of memory to provide to Zookeeper pods
+    default: "1Gi"
+  - name: cpus
+    description: Amount of cpu to provide to Zookeeper pods
+    default: "0.25"

test/integration/first-operator-test/first-operator/params.yaml

@@ -1,3 +1,4 @@
-replicas:
-  description: Number of replicas that should be run as part of the deployment
-  default: 2
+parameters:
+  - name: replicas
+    description: Number of replicas that should be run as part of the deployment
+    default: 2

test/integration/install-existing-crds-in-operator/crd-operator/params.yaml

@@ -1,3 +1,4 @@
-version:
-  description: Version of image
-  default: 1.7.9
+parameters:
+  - name: version
+    description: Version of image
+    default: 1.7.9

test/integration/operator-with-custom-crd/operator/params.yaml

@@ -0,0 +1 @@
+parameters:

test/integration/upgrade-command/first-operator/params.yaml

@@ -1,3 +1,4 @@
-version:
-  description: Version of image
-  default: 1.7.9
+parameters:
+  - name: version
+    description: Version of image
+    default: 1.7.9

test/integration/upgrade-command/second-operator/params.yaml

@@ -1,3 +1,4 @@
-version:
-  description: Version of image
-  default: 1.7.9
+parameters:
+  - name: version
+    description: Version of image
+    default: 1.7.9