Merge branch 'refactor-param-resolution2' into dependencies-v2-rebase

Signed-off-by: Carolyn Van Slyck <[email protected]>

Author: carolynvs

cmd/porter/bundle_test.go

@@ -22,7 +22,6 @@ func TestValidateInstallCommand(t *testing.T) {
 		wantOut   string
 	}{
 		{"no args", "install -r ghcr.io/getporter/examples/porter-hello:v0.2.0", "", ""},
-		{"invalid param", "install --param A:B -r ghcr.io/getporter/examples/porter-hello:v0.2.0", "invalid parameter (A:B), must be in name=value format", ""},
 		// --cred should still work, but print a message
 		{"old cred flag", "install --cred mycreds -r ghcr.io/getporter/examples/porter-hello:v0.2.0", "", "Flag --cred has been deprecated, please use credential-set instead"},
 	}
@@ -59,14 +58,19 @@ func TestValidateUninstallCommand(t *testing.T) {
 		name      string
 		args      string
 		wantError string
+		wantOut   string
 	}{
-		{"no args", "uninstall mybuns", ""},
-		{"invalid param", "uninstall mybuns --param A:B", "invalid parameter (A:B), must be in name=value format"},
+		{"no args", "uninstall mybuns", "", ""},
+		// --cred should still work, but print a message
+		{"old cred flag", "install --cred mycreds -r ghcr.io/getporter/examples/porter-hello:v0.2.0", "", "Flag --cred has been deprecated, please use credential-set instead"},
 	}
 
 	for _, tc := range testcases {
 		t.Run(tc.name, func(t *testing.T) {
+			var outBuf bytes.Buffer
 			p := buildRootCommand()
+			p.SetOut(&outBuf)
+			p.SetErr(&outBuf)
 			osargs := strings.Split(tc.args, " ")
 			cmd, args, err := p.Find(osargs)
 			require.NoError(t, err)
@@ -80,6 +84,10 @@ func TestValidateUninstallCommand(t *testing.T) {
 			} else {
 				require.EqualError(t, err, tc.wantError)
 			}
+
+			if tc.wantOut != "" {
+				tests.RequireOutputContains(t, outBuf.String(), tc.wantOut)
+			}
 		})
 	}
 }

pkg/porter/dependencies.go

@@ -248,7 +248,7 @@ func (e *dependencyExecutioner) prepareDependency(ctx context.Context, dep *queu
 
 	// Handle any parameter overrides for the dependency defined on the command line
 	// --param DEP#PARAM=VALUE
-	for key, value := range e.parentOpts.combinedParameters {
+	for key, value := range e.parentOpts.depParams {
 		parts := strings.Split(key, "#")
 		if len(parts) > 1 && parts[0] == dep.Alias {
 			paramName := parts[1]
@@ -293,7 +293,7 @@ func (e *dependencyExecutioner) executeDependency(ctx context.Context, dep *queu
 		}
 	}
 
-	resolvedParameters, err := e.porter.resolveParameters(ctx, depInstallation, dep.BundleReference.Definition, e.parentArgs.Action, dep.Parameters)
+	finalParams, err := e.porter.finalizeParameters(ctx, depInstallation, dep.BundleReference.Definition, e.parentArgs.Action, dep.Parameters)
 	if err != nil {
 		return span.Error(fmt.Errorf("error resolving parameters for dependency %s: %w", dep.Alias, err))
 	}
@@ -304,7 +304,7 @@ func (e *dependencyExecutioner) executeDependency(ctx context.Context, dep *queu
 		Installation:          depInstallation,
 		Driver:                e.parentArgs.Driver,
 		AllowDockerHostAccess: e.parentOpts.AllowDockerHostAccess,
-		Params:                resolvedParameters,
+		Params:                finalParams,
 		PersistLogs:           e.parentArgs.PersistLogs,
 	}
 

pkg/porter/install.go

@@ -81,12 +81,19 @@ func (p *Porter) InstallBundle(ctx context.Context, opts InstallOptions) error {
 		return log.Error(err)
 	}
 
-	err = p.applyActionOptionsToInstallation(ctx, &i, opts.BundleExecutionOptions)
+	_, err = p.applyActionToInstallation(ctx, opts, &i)
 	if err != nil {
 		return err
 	}
+
+	err = p.sanitizeInstallation(ctx, &i, opts.bundleRef.Definition)
+	if err != nil {
+		return err
+	}
+
 	i.TrackBundle(bundleRef.Reference)
 	i.Labels = opts.ParseLabels()
+
 	err = p.Installations.UpsertInstallation(ctx, i)
 	if err != nil {
 		return fmt.Errorf("error saving installation record: %w", err)
@@ -96,29 +103,12 @@ func (p *Porter) InstallBundle(ctx context.Context, opts InstallOptions) error {
 	return p.ExecuteAction(ctx, i, opts)
 }
 
-// Remember the parameters and credentials used with the bundle last.
-// Appends any newly specified parameters, parameter/credential sets to the installation record.
-// Users are expected to edit the installation record if they don't want that behavior.
-func (p *Porter) applyActionOptionsToInstallation(ctx context.Context, i *storage.Installation, opts *BundleExecutionOptions) error {
-	// Record the parameters specified by the user, with flags taking precedence over parameter set values
-	err := opts.LoadParameters(ctx, p, opts.bundleRef.Definition)
+func (p *Porter) sanitizeInstallation(ctx context.Context, inst *storage.Installation, bun cnab.ExtendedBundle) error {
+	strategies, err := p.Sanitizer.CleanParameters(ctx, inst.Parameters.Parameters, bun, inst.ID)
 	if err != nil {
 		return err
 	}
-	// Record the user-specified parameter values
-	err = opts.populateInternalParameterSet(ctx, p, opts.bundleRef.Definition, i)
-	if err != nil {
-		return err
-	}
-
-	// Record the names of the parameter and credential sets used if specified. Otherwise, reuse the previously specified sets.
-	// This should replace previously specified sets so that only what was just specified is used.
-	if len(opts.ParameterSets) > 0 {
-		i.ParameterSets = opts.ParameterSets
-	}
-	if len(opts.CredentialIdentifiers) > 0 {
-		i.CredentialSets = opts.CredentialIdentifiers
-	}
 
+	inst.Parameters = inst.NewInternalParameterSet(strategies...)
 	return nil
 }

pkg/porter/install_test.go

@@ -70,11 +70,20 @@ func TestInstallOptions_validateDriver(t *testing.T) {
 	}
 }
 
-func TestPorter_applyActionOptionsToInstallation(t *testing.T) {
+func TestPorter_ApplyParametersToInstallation(t *testing.T) {
 	setup := func() (context.Context, *TestPorter, *storage.Installation) {
 		ctx := context.Background()
 		p := NewTestPorter(t)
 
+		p.TestParameters.InsertParameterSet(ctx, storage.ParameterSet{
+			ParameterSetSpec: storage.ParameterSetSpec{
+				Name: "oldps1",
+				Parameters: []secrets.Strategy{
+					{Name: "logLevel", Source: secrets.Source{Key: "value", Value: "2"}},
+				},
+			},
+		})
+
 		p.TestParameters.InsertParameterSet(ctx, storage.ParameterSet{
 			ParameterSetSpec: storage.ParameterSetSpec{
 				Name: "newps1",
@@ -98,7 +107,7 @@ func TestPorter_applyActionOptionsToInstallation(t *testing.T) {
 		ctx, p, inst := setup()
 
 		// We should replace the previously used sets since we specified different ones
-		opts := NewBundleExecutionOptions()
+		opts := NewInstallOptions()
 		opts.Reference = kahnlatest.String()
 		opts.bundleRef = &cnab.BundleReference{
 			Reference: kahnlatest,
@@ -119,8 +128,8 @@ func TestPorter_applyActionOptionsToInstallation(t *testing.T) {
 		opts.CredentialIdentifiers = []string{"newcs1"}
 
 		require.NoError(t, opts.Validate(ctx, nil, p.Porter))
-		err := p.applyActionOptionsToInstallation(ctx, inst, opts)
-		require.NoError(t, err, "applyActionOptionsToInstallation failed")
+		_, err := p.applyActionToInstallation(ctx, opts, inst)
+		require.NoError(t, err, "applyActionToInstallation failed")
 
 		require.Equal(t, opts.ParameterSets, inst.ParameterSets, "expected the installation to replace the credential sets with those specified")
 		require.Equal(t, opts.CredentialIdentifiers, inst.CredentialSets, "expected the installation to replace the credential sets with those specified")
@@ -130,7 +139,7 @@ func TestPorter_applyActionOptionsToInstallation(t *testing.T) {
 		ctx, p, inst := setup()
 
 		// We should reuse the previously used sets since we specified different ones
-		opts := NewBundleExecutionOptions()
+		opts := NewInstallOptions()
 		opts.Reference = kahnlatest.String()
 		opts.bundleRef = &cnab.BundleReference{
 			Reference: kahnlatest,
@@ -150,7 +159,7 @@ func TestPorter_applyActionOptionsToInstallation(t *testing.T) {
 		opts.CredentialIdentifiers = []string{}
 
 		require.NoError(t, opts.Validate(ctx, nil, p.Porter))
-		err := p.applyActionOptionsToInstallation(ctx, inst, opts)
+		_, err := p.applyActionToInstallation(ctx, opts, inst)
 		require.NoError(t, err, "applyActionOptionsToInstallation failed")
 
 		require.Equal(t, []string{"oldps1"}, inst.ParameterSets, "expected the installation to reuse the previous credential sets")

pkg/porter/invoke.go

@@ -65,9 +65,16 @@ func (p *Porter) InvokeBundle(ctx context.Context, opts InvokeOptions) error {
 		// Create an ephemeral installation just for this run
 		installation = storage.Installation{InstallationSpec: storage.InstallationSpec{Namespace: opts.Namespace, Name: opts.Name}}
 	}
-	err = p.applyActionOptionsToInstallation(ctx, &installation, opts.BundleExecutionOptions)
+
+	_, err = p.applyActionToInstallation(ctx, opts, &installation)
+	if err != nil {
+		return err
+	}
+
+	err = p.sanitizeInstallation(ctx, &installation, bundleRef.Definition)
 	if err != nil {
 		return err
 	}
+
 	return p.ExecuteAction(ctx, installation, opts)
 }

pkg/porter/lifecycle.go

@@ -4,15 +4,13 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"strings"
 
 	"get.porter.sh/porter/pkg/cache"
 	"get.porter.sh/porter/pkg/cnab"
 	"get.porter.sh/porter/pkg/cnab/bundleruntime"
 	"get.porter.sh/porter/pkg/cnab/drivers"
 	"get.porter.sh/porter/pkg/encoding"
 	"get.porter.sh/porter/pkg/portercontext"
-	"get.porter.sh/porter/pkg/secrets"
 	"get.porter.sh/porter/pkg/storage"
 	"get.porter.sh/porter/pkg/tracing"
 	"github.com/opencontainers/go-digest"
@@ -61,14 +59,8 @@ type BundleExecutionOptions struct {
 	// Driver is the CNAB-compliant driver used to run bundle actions.
 	Driver string
 
-	// parsedParams is the parsed set of parameters from Params.
-	parsedParams map[string]string
-
-	// parsedParamSets is the parsed set of parameter from ParameterSets
-	parsedParamSets map[string]string
-
-	// combinedParameters is parsedParams merged on top of parsedParamSets.
-	combinedParameters map[string]string
+	// parameters that are intended for dependencies
+	depParams map[string]string
 }
 
 func NewBundleExecutionOptions() *BundleExecutionOptions {
@@ -86,12 +78,6 @@ func (o *BundleExecutionOptions) Validate(ctx context.Context, args []string, p
 		return err
 	}
 
-	// Only validate the syntax of the --param flags
-	// We will validate the parameter sets later once we have the bundle loaded.
-	if err := o.parseParams(); err != nil {
-		return err
-	}
-
 	o.defaultDriver(p)
 	if err := o.validateDriver(p.Context); err != nil {
 		return err
@@ -100,101 +86,6 @@ func (o *BundleExecutionOptions) Validate(ctx context.Context, args []string, p
 	return nil
 }
 
-// LoadParameters validates and resolves the parameters and sets. It must be
-// called after porter has loaded the bundle definition.
-func (o *BundleExecutionOptions) LoadParameters(ctx context.Context, p *Porter, bun cnab.ExtendedBundle) error {
-	// This is called in multiple code paths, so exit early if
-	// we have already loaded the parameters into combinedParameters
-	if o.combinedParameters != nil {
-		return nil
-	}
-
-	err := o.parseParams()
-	if err != nil {
-		return err
-	}
-
-	err = o.parseParamSets(ctx, p, bun)
-	if err != nil {
-		return err
-	}
-
-	o.combinedParameters = o.combineParameters(p.Context)
-
-	return nil
-}
-
-// parsedParams parses the variable assignments in Params.
-func (o *BundleExecutionOptions) parseParams() error {
-	p, err := storage.ParseVariableAssignments(o.Params)
-	if err != nil {
-		return err
-	}
-
-	o.parsedParams = p
-	return nil
-}
-
-func (o *BundleExecutionOptions) populateInternalParameterSet(ctx context.Context, p *Porter, bun cnab.ExtendedBundle, i *storage.Installation) error {
-	strategies := make([]secrets.Strategy, 0, len(o.parsedParams))
-	for name, value := range o.parsedParams {
-		strategies = append(strategies, storage.ValueStrategy(name, value))
-	}
-
-	strategies, err := p.Sanitizer.CleanParameters(ctx, strategies, bun, i.ID)
-	if err != nil {
-		return err
-	}
-
-	if len(strategies) == 0 {
-		// if no override is specified, clear out the old parameters on the
-		// installation record
-		i.Parameters.Parameters = nil
-		return nil
-	}
-
-	i.Parameters = i.NewInternalParameterSet(strategies...)
-
-	return nil
-}
-
-// parseParamSets parses the variable assignments in ParameterSets.
-func (o *BundleExecutionOptions) parseParamSets(ctx context.Context, p *Porter, bun cnab.ExtendedBundle) error {
-	if len(o.ParameterSets) > 0 {
-		parsed, err := p.loadParameterSets(ctx, bun, o.Namespace, o.ParameterSets)
-		if err != nil {
-			return fmt.Errorf("unable to process provided parameter sets: %w", err)
-		}
-		o.parsedParamSets = parsed
-	}
-	return nil
-}
-
-// Combine the parameters into a single map
-// The params set on the command line take precedence over the params set in
-// parameter set files
-// Anything set multiple times, is decided by "last one set wins"
-func (o *BundleExecutionOptions) combineParameters(c *portercontext.Context) map[string]string {
-	final := make(map[string]string)
-
-	for k, v := range o.parsedParamSets {
-		final[k] = v
-	}
-
-	for k, v := range o.parsedParams {
-		final[k] = v
-	}
-
-	//
-	// Default the porter-debug param to --debug
-	//
-	if o.DebugMode {
-		final["porter-debug"] = "true"
-	}
-
-	return final
-}
-
 // defaultDriver supplies the default driver if none is specified
 func (o *BundleExecutionOptions) defaultDriver(p *Porter) {
 	//
@@ -358,23 +249,10 @@ func (p *Porter) BuildActionArgs(ctx context.Context, installation storage.Insta
 
 	// Resolve the final set of typed parameters, taking into account the user overrides, parameter sources
 	// and defaults
-	err = opts.LoadParameters(ctx, p, opts.bundleRef.Definition)
-	if err != nil {
-		return bundleruntime.ActionArguments{}, err
-	}
-
 	log.Debugf("resolving parameters for installation %s", installation)
 
-	// Do not resolve parameters from dependencies
-	params := make(map[string]string, len(opts.combinedParameters))
-	for k, v := range opts.combinedParameters {
-		if strings.Contains(k, "#") {
-			continue
-		}
-		params[k] = v
-	}
-
-	resolvedParams, err := p.resolveParameters(ctx, installation, bundleRef.Definition, action.GetAction(), params)
+	// TODO: I think this gets called twice sometimes, maybe have a short circuit check that we've done it already? Can we remove it from here and assume it was done elsewhere?
+	resolvedParams, err := p.applyActionToInstallation(ctx, action, &installation)
 	if err != nil {
 		return bundleruntime.ActionArguments{}, log.Error(err)
 	}