By default print a warning rather than failing with an error when a parameter is passed to docker app and is not defined in the bundle.

Add --strict flag to throw an error rather displaying a warning

Signed-off-by: Jean-Christophe Sirot <jean-christophe.sirot@docker.com>

Author: Jean-Christophe Sirot

internal/commands/cnab.go

@@ -368,6 +368,7 @@ func prepareCustomAction(actionName string, dockerCli command.Cli, appname strin
 	if err := mergeBundleParameters(installation,
 		withFileParameters(paramsOpts.parametersFiles),
 		withCommandLineParameters(paramsOpts.overrides),
+		withStrictMode(paramsOpts.strictMode),
 	); err != nil {
 		return nil, nil, nil, err
 	}

internal/commands/install.go

@@ -117,6 +117,7 @@ func runInstall(dockerCli command.Cli, appname string, opts installOptions) erro
 		withCommandLineParameters(opts.overrides),
 		withOrchestratorParameters(opts.orchestrator, opts.kubeNamespace),
 		withSendRegistryAuth(opts.sendRegistryAuth),
+		withStrictMode(opts.strictMode),
 	); err != nil {
 		return err
 	}

internal/commands/parameters.go

@@ -2,6 +2,8 @@ package commands
 
 import (
 	"fmt"
+	"io"
+	"os"
 	"strings"
 
 	"github.com/deislabs/cnab-go/bundle"
@@ -13,80 +15,109 @@ import (
 	"github.com/pkg/errors"
 )
 
-type mergeBundleOpt func(bndl *bundle.Bundle, params map[string]string) error
+type mergeBundleConfig struct {
+	bundle     *bundle.Bundle
+	params     map[string]string
+	strictMode bool
+	stderr     io.Writer
+}
+
+type mergeBundleOpt func(c *mergeBundleConfig) error
 
 func withFileParameters(parametersFiles []string) mergeBundleOpt {
-	return func(bndl *bundle.Bundle, params map[string]string) error {
+	return func(c *mergeBundleConfig) error {
 		p, err := parameters.LoadFiles(parametersFiles)
 		if err != nil {
 			return err
 		}
 		for k, v := range p.Flatten() {
-			params[k] = v
+			c.params[k] = v
 		}
 		return nil
 	}
 }
 
 func withCommandLineParameters(overrides []string) mergeBundleOpt {
-	return func(bndl *bundle.Bundle, params map[string]string) error {
+	return func(c *mergeBundleConfig) error {
 		d := cliopts.ConvertKVStringsToMap(overrides)
 		for k, v := range d {
-			params[k] = v
+			c.params[k] = v
 		}
 		return nil
 	}
 }
 
 func withSendRegistryAuth(sendRegistryAuth bool) mergeBundleOpt {
-	return func(bndl *bundle.Bundle, params map[string]string) error {
-		if _, ok := bndl.Definitions[internal.ParameterShareRegistryCredsName]; ok {
+	return func(c *mergeBundleConfig) error {
+		if _, ok := c.bundle.Definitions[internal.ParameterShareRegistryCredsName]; ok {
 			val := "false"
 			if sendRegistryAuth {
 				val = "true"
 			}
-			params[internal.ParameterShareRegistryCredsName] = val
+			c.params[internal.ParameterShareRegistryCredsName] = val
 		}
 		return nil
 	}
 }
 
 func withOrchestratorParameters(orchestrator string, kubeNamespace string) mergeBundleOpt {
-	return func(bndl *bundle.Bundle, params map[string]string) error {
-		if _, ok := bndl.Definitions[internal.ParameterOrchestratorName]; ok {
-			params[internal.ParameterOrchestratorName] = orchestrator
+	return func(c *mergeBundleConfig) error {
+		if _, ok := c.bundle.Definitions[internal.ParameterOrchestratorName]; ok {
+			c.params[internal.ParameterOrchestratorName] = orchestrator
 		}
-		if _, ok := bndl.Definitions[internal.ParameterKubernetesNamespaceName]; ok {
-			params[internal.ParameterKubernetesNamespaceName] = kubeNamespace
+		if _, ok := c.bundle.Definitions[internal.ParameterKubernetesNamespaceName]; ok {
+			c.params[internal.ParameterKubernetesNamespaceName] = kubeNamespace
 		}
 		return nil
 	}
 }
 
+func withErrorWriter(w io.Writer) mergeBundleOpt {
+	return func(c *mergeBundleConfig) error {
+		c.stderr = w
+		return nil
+	}
+}
+
+func withStrictMode(strict bool) mergeBundleOpt {
+	return func(c *mergeBundleConfig) error {
+		c.strictMode = strict
+		return nil
+	}
+}
 func mergeBundleParameters(installation *store.Installation, ops ...mergeBundleOpt) error {
 	bndl := installation.Bundle
 	if installation.Parameters == nil {
 		installation.Parameters = make(map[string]interface{})
 	}
 	userParams := map[string]string{}
+	cfg := &mergeBundleConfig{
+		bundle: bndl,
+		params: userParams,
+		stderr: os.Stderr,
+	}
 	for _, op := range ops {
-		if err := op(bndl, userParams); err != nil {
+		if err := op(cfg); err != nil {
 			return err
 		}
 	}
-	if err := matchAndMergeParametersDefinition(installation.Parameters, userParams, bndl.Definitions); err != nil {
+	if err := matchAndMergeParametersDefinition(installation.Parameters, cfg.params, bndl.Definitions, cfg.strictMode, cfg.stderr); err != nil {
 		return err
 	}
 	var err error
 	installation.Parameters, err = bundle.ValuesOrDefaults(installation.Parameters, bndl)
 	return err
 }
 
-func matchAndMergeParametersDefinition(currentValues map[string]interface{}, parameterValues map[string]string, parameterDefinitions definition.Definitions) error {
+func matchAndMergeParametersDefinition(currentValues map[string]interface{}, parameterValues map[string]string, parameterDefinitions definition.Definitions, strictMode bool, stderr io.Writer) error {
 	for k, v := range parameterValues {
 		definition, ok := parameterDefinitions[k]
 		if !ok {
-			return fmt.Errorf("parameter %q is not defined in the bundle", k)
+			if strictMode {
+				return fmt.Errorf("parameter %q is not defined in the bundle", k)
+			}
+			fmt.Fprintf(stderr, "Warning: parameter %q is not defined in the bundle\n", k)
+			continue
 		}
 		value, err := definition.ConvertValue(v)
 		if err != nil {

internal/commands/parameters_test.go

@@ -1,6 +1,8 @@
 package commands
 
 import (
+	"bytes"
+	"strings"
 	"testing"
 
 	"github.com/deislabs/cnab-go/bundle"
@@ -26,7 +28,11 @@ overridden: bar`))
 	actual := map[string]string{
 		"overridden": "foo",
 	}
-	err := withFileParameters([]string{tmpDir.Join("params.yaml")})(bundle, actual)
+	err := withFileParameters([]string{tmpDir.Join("params.yaml")})(
+		&mergeBundleConfig{
+			bundle: bundle,
+			params: actual,
+		})
 	assert.NilError(t, err)
 	expected := map[string]string{
 		"param1.param2": "value1",
@@ -42,7 +48,11 @@ func TestWithCommandLineParameters(t *testing.T) {
 		"overridden": "foo",
 	}
 
-	err := withCommandLineParameters([]string{"param1.param2=value1", "param3=3", "overridden=bar"})(bundle, actual)
+	err := withCommandLineParameters([]string{"param1.param2=value1", "param3=3", "overridden=bar"})(
+		&mergeBundleConfig{
+			bundle: bundle,
+			params: actual,
+		})
 	assert.NilError(t, err)
 	expected := map[string]string{
 		"param1.param2": "value1",
@@ -135,7 +145,10 @@ func TestWithOrchestratorParameters(t *testing.T) {
 	for _, testCase := range testCases {
 		t.Run(testCase.name, func(t *testing.T) {
 			actual := map[string]string{}
-			err := withOrchestratorParameters("kubernetes", "my-namespace")(testCase.bundle, actual)
+			err := withOrchestratorParameters("kubernetes", "my-namespace")(&mergeBundleConfig{
+				bundle: testCase.bundle,
+				params: actual,
+			})
 			assert.NilError(t, err)
 			assert.Assert(t, cmp.DeepEqual(actual, testCase.expected))
 		})
@@ -144,12 +157,12 @@ func TestWithOrchestratorParameters(t *testing.T) {
 
 func TestMergeBundleParameters(t *testing.T) {
 	t.Run("Override Order", func(t *testing.T) {
-		first := func(b *bundle.Bundle, params map[string]string) error {
-			params["param"] = "first"
+		first := func(c *mergeBundleConfig) error {
+			c.params["param"] = "first"
 			return nil
 		}
-		second := func(b *bundle.Bundle, params map[string]string) error {
-			params["param"] = "second"
+		second := func(c *mergeBundleConfig) error {
+			c.params["param"] = "second"
 			return nil
 		}
 		bundle := prepareBundle(withParameterAndDefault("param", "string", "default"))
@@ -177,8 +190,8 @@ func TestMergeBundleParameters(t *testing.T) {
 	})
 
 	t.Run("Converting values", func(t *testing.T) {
-		withIntValue := func(b *bundle.Bundle, params map[string]string) error {
-			params["param"] = "1"
+		withIntValue := func(c *mergeBundleConfig) error {
+			c.params["param"] = "1"
 			return nil
 		}
 		bundle := prepareBundle(withParameter("param", "integer"))
@@ -202,20 +215,33 @@ func TestMergeBundleParameters(t *testing.T) {
 		assert.Assert(t, cmp.DeepEqual(i.Parameters, expected))
 	})
 
-	t.Run("Undefined parameter is rejected", func(t *testing.T) {
-		withUndefined := func(b *bundle.Bundle, params map[string]string) error {
-			params["param"] = "1"
+	t.Run("Undefined parameter throws warning", func(t *testing.T) {
+		withUndefined := func(c *mergeBundleConfig) error {
+			c.params["param"] = "1"
+			return nil
+		}
+		bundle := prepareBundle()
+		i := &store.Installation{Claim: claim.Claim{Bundle: bundle}}
+		buf := new(bytes.Buffer)
+		err := mergeBundleParameters(i, withUndefined, withErrorWriter(buf))
+		assert.NilError(t, err)
+		assert.Assert(t, strings.Contains(buf.String(), "is not defined in the bundle"))
+	})
+
+	t.Run("Undefined parameter with strict mode is rejected", func(t *testing.T) {
+		withUndefined := func(c *mergeBundleConfig) error {
+			c.params["param"] = "1"
 			return nil
 		}
 		bundle := prepareBundle()
 		i := &store.Installation{Claim: claim.Claim{Bundle: bundle}}
-		err := mergeBundleParameters(i, withUndefined)
+		err := mergeBundleParameters(i, withUndefined, withStrictMode(true))
 		assert.ErrorContains(t, err, "is not defined in the bundle")
 	})
 
 	t.Run("Invalid type is rejected", func(t *testing.T) {
-		withIntValue := func(b *bundle.Bundle, params map[string]string) error {
-			params["param"] = "foo"
+		withIntValue := func(c *mergeBundleConfig) error {
+			c.params["param"] = "foo"
 			return nil
 		}
 		bundle := prepareBundle(withParameter("param", "integer"))
@@ -225,8 +251,8 @@ func TestMergeBundleParameters(t *testing.T) {
 	})
 
 	t.Run("Invalid value is rejected", func(t *testing.T) {
-		withInvalidValue := func(b *bundle.Bundle, params map[string]string) error {
-			params["param"] = "invalid"
+		withInvalidValue := func(c *mergeBundleConfig) error {
+			c.params["param"] = "invalid"
 			return nil
 		}
 		bundle := prepareBundle(withParameterAndValues("param", "string", []interface{}{"valid"}))

internal/commands/root.go

@@ -94,11 +94,13 @@ func prepareBundleStore() (store.BundleStore, error) {
 type parametersOptions struct {
 	parametersFiles []string
 	overrides       []string
+	strictMode      bool
 }
 
 func (o *parametersOptions) addFlags(flags *pflag.FlagSet) {
 	flags.StringArrayVar(&o.parametersFiles, "parameters-file", []string{}, "Override parameters file")
 	flags.StringArrayVarP(&o.overrides, "set", "s", []string{}, "Override parameter value")
+	flags.BoolVar(&o.strictMode, "strict", false, "Fail when a paramater is undefined instead of displaying a warning")
 }
 
 type credentialOptions struct {

internal/commands/upgrade.go

@@ -67,6 +67,7 @@ func runUpgrade(dockerCli command.Cli, installationName string, opts upgradeOpti
 		withFileParameters(opts.parametersFiles),
 		withCommandLineParameters(opts.overrides),
 		withSendRegistryAuth(opts.sendRegistryAuth),
+		withStrictMode(opts.strictMode),
 	); err != nil {
 		return err
 	}