Refactor required parameters

action/action.go

@@ -56,7 +56,7 @@ func getImageMap(b *bundle.Bundle) ([]byte, error) {
 	return json.Marshal(imgs)
 }
 
-func appliesToAction(action string, parameter bundle.ParameterDefinition) bool {
+func appliesToAction(action string, parameter bundle.Parameter) bool {
 	if len(parameter.ApplyTo) == 0 {
 		return true
 	}
@@ -76,15 +76,13 @@ func opFromClaim(action string, stateless bool, c *claim.Claim, ii bundle.Invoca
 
 	// Quick verification that no params were passed that are not actual legit params.
 	for key := range c.Parameters {
-		if _, ok := c.Bundle.Parameters.Fields[key]; !ok {
+		if _, ok := c.Bundle.Parameters[key]; !ok {
 			return nil, fmt.Errorf("undefined parameter %q", key)
 		}
 	}
 
-	if c.Bundle.Parameters != nil {
-		if err := injectParameters(action, c, env, files); err != nil {
-			return nil, err
-		}
+	if err := injectParameters(action, c, env, files); err != nil {
+		return nil, err
 	}
 
 	imgMap, err := getImageMap(c.Bundle)
@@ -112,15 +110,10 @@ func opFromClaim(action string, stateless bool, c *claim.Claim, ii bundle.Invoca
 }
 
 func injectParameters(action string, c *claim.Claim, env, files map[string]string) error {
-	requiredMap := map[string]struct{}{}
-	for _, key := range c.Bundle.Parameters.Required {
-		requiredMap[key] = struct{}{}
-	}
-	for k, param := range c.Bundle.Parameters.Fields {
+	for k, param := range c.Bundle.Parameters {
 		rawval, ok := c.Parameters[k]
 		if !ok {
-			_, required := requiredMap[k]
-			if required && appliesToAction(action, param) {
+			if param.Required && appliesToAction(action, param) {
 				return fmt.Errorf("missing required parameter %q for action %q", k, action)
 			}
 			continue

action/action_test.go

@@ -72,22 +72,20 @@ func mockBundle() *bundle.Bundle {
 				Default: "three",
 			},
 		},
-		Parameters: &bundle.ParametersDefinition{
-			Fields: map[string]bundle.ParameterDefinition{
-				"param_one": {
-					Definition: "ParamOne",
-				},
-				"param_two": {
-					Definition: "ParamTwo",
-					Destination: &bundle.Location{
-						EnvironmentVariable: "PARAM_TWO",
-					},
+		Parameters: map[string]bundle.Parameter{
+			"param_one": {
+				Definition: "ParamOne",
+			},
+			"param_two": {
+				Definition: "ParamTwo",
+				Destination: &bundle.Location{
+					EnvironmentVariable: "PARAM_TWO",
 				},
-				"param_three": {
-					Definition: "ParamThree",
-					Destination: &bundle.Location{
-						Path: "/param/three",
-					},
+			},
+			"param_three": {
+				Definition: "ParamThree",
+				Destination: &bundle.Location{
+					Path: "/param/three",
 				},
 			},
 		},
@@ -203,8 +201,9 @@ func TestOpFromClaim_UndefinedParams(t *testing.T) {
 func TestOpFromClaim_MissingRequiredParameter(t *testing.T) {
 	now := time.Now()
 	b := mockBundle()
-	b.Parameters.Required = []string{"param_one"}
-	b.Parameters.Fields["param_one"] = bundle.ParameterDefinition{}
+	b.Parameters["param_one"] = bundle.Parameter{
+		Required: true,
+	}
 
 	c := &claim.Claim{
 		Created:  now,
@@ -233,10 +232,10 @@ func TestOpFromClaim_MissingRequiredParamSpecificToAction(t *testing.T) {
 	now := time.Now()
 	b := mockBundle()
 	// Add a required parameter only defined for the test action
-	b.Parameters.Fields["param_test"] = bundle.ParameterDefinition{
-		ApplyTo: []string{"test"},
+	b.Parameters["param_test"] = bundle.Parameter{
+		ApplyTo:  []string{"test"},
+		Required: true,
 	}
-	b.Parameters.Required = []string{"param_test"}
 	c := &claim.Claim{
 		Created:  now,
 		Modified: now,

bundle/bundle.go

@@ -25,7 +25,7 @@ type Bundle struct {
 	InvocationImages   []InvocationImage      `json:"invocationImages" mapstructure:"invocationImages"`
 	Images             map[string]Image       `json:"images,omitempty" mapstructure:"images"`
 	Actions            map[string]Action      `json:"actions,omitempty" mapstructure:"actions"`
-	Parameters         *ParametersDefinition  `json:"parameters,omitempty" mapstructure:"parameters"`
+	Parameters         map[string]Parameter   `json:"parameters,omitempty" mapstructure:"parameters"`
 	Credentials        map[string]Credential  `json:"credentials,omitempty" mapstructure:"credentials"`
 	Outputs            *OutputsDefinition     `json:"outputs,omitempty" mapstructure:"outputs"`
 	Definitions        definition.Definitions `json:"definitions,omitempty" mapstructure:"definitions"`
@@ -98,7 +98,7 @@ type InvocationImage struct {
 	BaseImage `mapstructure:",squash"`
 }
 
-// Map that stores the relocated images
+// ImageRelocationMap stores the relocated images
 // The key is the Image in bundle.json and the value is the new Image
 // from the relocated registry
 type ImageRelocationMap map[string]string
@@ -139,22 +139,13 @@ type Action struct {
 func ValuesOrDefaults(vals map[string]interface{}, currentVals map[string]interface{}, b *Bundle) (map[string]interface{}, error) {
 	res := map[string]interface{}{}
 
-	if b.Parameters == nil {
-		return res, nil
-	}
-
-	requiredMap := map[string]struct{}{}
-	for _, key := range b.Parameters.Required {
-		requiredMap[key] = struct{}{}
-	}
-
-	for name, def := range b.Parameters.Fields {
-		s, ok := b.Definitions[def.Definition]
+	for name, param := range b.Parameters {
+		s, ok := b.Definitions[param.Definition]
 		if !ok {
 			return res, fmt.Errorf("unable to find definition for %s", name)
 		}
 		if val, ok := vals[name]; ok {
-			if currentVal, ok := currentVals[name]; def.Immutable && ok && currentVal != val {
+			if currentVal, ok := currentVals[name]; param.Immutable && ok && currentVal != val {
 				return res, fmt.Errorf("parameter %s is immutable and cannot be overridden with value %v", name, val)
 			}
 			valErrs, err := s.Validate(val)
@@ -170,7 +161,7 @@ func ValuesOrDefaults(vals map[string]interface{}, currentVals map[string]interf
 			typedVal := s.CoerceValue(val)
 			res[name] = typedVal
 			continue
-		} else if _, ok := requiredMap[name]; ok {
+		} else if param.Required {
 			return res, fmt.Errorf("parameter %q is required", name)
 		}
 		res[name] = s.Default

bundle/bundle_test.go

@@ -126,20 +126,18 @@ func TestValuesOrDefaults(t *testing.T) {
 				Default: false,
 			},
 		},
-		Parameters: &ParametersDefinition{
-			Fields: map[string]ParameterDefinition{
-				"port": {
-					Definition: "portType",
-				},
-				"host": {
-					Definition: "hostType",
-				},
-				"enabled": {
-					Definition: "enabledType",
-				},
-				"replicaCount": {
-					Definition: "replicaCountType",
-				},
+		Parameters: map[string]Parameter{
+			"port": {
+				Definition: "portType",
+			},
+			"host": {
+				Definition: "hostType",
+			},
+			"enabled": {
+				Definition: "enabledType",
+			},
+			"replicaCount": {
+				Definition: "replicaCountType",
 			},
 		},
 	}
@@ -156,6 +154,10 @@ func TestValuesOrDefaults(t *testing.T) {
 	vals["replicaCount"] = "banana"
 	_, err = ValuesOrDefaults(vals, currentVals, b)
 	is.Error(err)
+
+	// Check for panic when zero value Bundle is passed
+	_, err = ValuesOrDefaults(vals, currentVals, &Bundle{})
+	is.NoError(err)
 }
 
 func TestValuesOrDefaults_NoParameter(t *testing.T) {
@@ -184,16 +186,14 @@ func TestValuesOrDefaults_Required(t *testing.T) {
 				Default: false,
 			},
 		},
-		Parameters: &ParametersDefinition{
-			Fields: map[string]ParameterDefinition{
-				"minimum": {
-					Definition: "minType",
-				},
-				"enabled": {
-					Definition: "enabledType",
-				},
+		Parameters: map[string]Parameter{
+			"minimum": {
+				Definition: "minType",
+				Required:   true,
+			},
+			"enabled": {
+				Definition: "enabledType",
 			},
-			Required: []string{"minimum"},
 		},
 	}
 
@@ -233,15 +233,13 @@ func TestValuesOrDefaults_Immutable(t *testing.T) {
 				Default: false,
 			},
 		},
-		Parameters: &ParametersDefinition{
-			Fields: map[string]ParameterDefinition{
-				"namespace": {
-					Definition: "namespaceType",
-					Immutable:  true,
-				},
-				"enabled": {
-					Definition: "enabledType",
-				},
+		Parameters: map[string]Parameter{
+			"namespace": {
+				Definition: "namespaceType",
+				Immutable:  true,
+			},
+			"enabled": {
+				Definition: "enabledType",
 			},
 		},
 	}
@@ -523,41 +521,40 @@ func TestBundleMarshallAllThings(t *testing.T) {
 				Type: "string",
 			},
 		},
-		Parameters: &ParametersDefinition{
-			Fields: map[string]ParameterDefinition{
-				"port": {
-					Definition: "portType",
-					Destination: &Location{
-						EnvironmentVariable: "PORT",
-					},
+		Parameters: map[string]Parameter{
+			"port": {
+				Definition: "portType",
+				Destination: &Location{
+					EnvironmentVariable: "PORT",
 				},
-				"host": {
-					Definition: "hostType",
-					Destination: &Location{
-						EnvironmentVariable: "HOST",
-					},
+				Required: true,
+			},
+			"host": {
+				Definition: "hostType",
+				Destination: &Location{
+					EnvironmentVariable: "HOST",
 				},
-				"enabled": {
-					Definition: "enabledType",
-					Destination: &Location{
-						EnvironmentVariable: "ENABLED",
-					},
+				Required: true,
+			},
+			"enabled": {
+				Definition: "enabledType",
+				Destination: &Location{
+					EnvironmentVariable: "ENABLED",
 				},
-				"replicaCount": {
-					Definition: "replicaCountType",
-					Destination: &Location{
-						EnvironmentVariable: "REPLICA_COUNT",
-					},
+			},
+			"replicaCount": {
+				Definition: "replicaCountType",
+				Destination: &Location{
+					EnvironmentVariable: "REPLICA_COUNT",
 				},
-				"productKey": {
-					Definition: "productKeyType",
-					Destination: &Location{
-						EnvironmentVariable: "PRODUCT_KEY",
-					},
-					Immutable: true,
+			},
+			"productKey": {
+				Definition: "productKeyType",
+				Destination: &Location{
+					EnvironmentVariable: "PRODUCT_KEY",
 				},
+				Immutable: true,
 			},
-			Required: []string{"port", "host"},
 		},
 		Outputs: &OutputsDefinition{
 			Fields: map[string]OutputDefinition{
@@ -576,7 +573,7 @@ func TestBundleMarshallAllThings(t *testing.T) {
 
 	_, err = b.WriteTo(&buf)
 	require.NoError(t, err, "test requires output")
-	assert.Equal(t, []byte(expectedJSON), buf.Bytes(), "output should match expected canonical json")
+	assert.Equal(t, string(expectedJSON), buf.String(), "output should match expected canonical json")
 }
 
 func TestValidateABundleAndParams(t *testing.T) {

bundle/parameters.go

@@ -1,15 +1,11 @@
 package bundle
 
-type ParametersDefinition struct {
-	Fields   map[string]ParameterDefinition `json:"fields" mapstructure:"fields"`
-	Required []string                       `json:"required,omitempty" mapstructure:"required,omitempty"`
-}
-
-// ParameterDefinition defines a single parameter for a CNAB bundle
-type ParameterDefinition struct {
+// Parameter defines a single parameter for a CNAB bundle
+type Parameter struct {
 	Definition  string    `json:"definition" mapstructure:"definition"`
 	ApplyTo     []string  `json:"applyTo,omitempty" mapstructure:"applyTo,omitempty"`
 	Description string    `json:"description,omitempty" mapstructure:"description"`
 	Destination *Location `json:"destination,omitemtpty" mapstructure:"destination"`
 	Immutable   bool      `json:"immutable,omitempty" mapstructure:"immutable,omitempty"`
+	Required    bool      `json:"required,omitempty" mapstructure:"required,omitempty"`
 }