Lint for parameters being used in actions to which they don't apply

docs/content/docs/references/linter.md

@@ -6,6 +6,7 @@ weight: 9
 
 - [exec-100](#exec-100)
 - [porter-100](#porter-100)
+- [porter-101](#porter-101)
 
 ## exec-100
 
@@ -30,3 +31,30 @@ Using a reserved prefix can be problematic as it can overwrite a predefined para
 To fix the problem indicated by the porter-100 error, you should replace the prefix of any newly defined parameters to not start with "porter".
 
 You can find more information about parameters in following URL: https://porter.sh/quickstart/parameters/.
+
+## porter-101
+
+The porter-101 error suggests that an action uses a parameter that is not available to it.
+
+This is an of a manifest triggering the error (shorten for brevity):
+
+```yaml
+parameters:
+  - name: uninstallParam
+    type: string
+    applyTo:
+      - uninstall # Notice the parameter only applies to the uninstall action
+
+install:
+  - exec:
+      description: "Install Hello World"
+      command: ./helpers.sh
+      arguments:
+        - install
+        - "${ bundle.parameters.uninstallParam }"
+```
+
+To fix the problem indicated by the porter-101 error, you should ensure that all parameters used in the action applies to the actions where
+it is referenced.
+
+You can find more information about applyTo in the following URL: https://porter.sh/docs/bundle/manifest/#parameter-types.

pkg/linter/linter.go

@@ -11,6 +11,7 @@ import (
 	"get.porter.sh/porter/pkg/pkgmgmt"
 	"get.porter.sh/porter/pkg/portercontext"
 	"get.porter.sh/porter/pkg/tracing"
+	"get.porter.sh/porter/pkg/yaml"
 	"github.com/dustin/go-humanize"
 )
 
@@ -154,6 +155,11 @@ func New(cxt *portercontext.Context, mixins pkgmgmt.PackageManager) *Linter {
 	}
 }
 
+type action struct {
+	name  string
+	steps manifest.Steps
+}
+
 func (l *Linter) Lint(ctx context.Context, m *manifest.Manifest) (Results, error) {
 	// Check for reserved porter prefix on parameter names
 	reservedPrefixes := []string{"porter-", "porter_"}
@@ -183,9 +189,29 @@ func (l *Linter) Lint(ctx context.Context, m *manifest.Manifest) (Results, error
 			}
 		}
 	}
+
+	// Check if parameters apply to the steps
 	ctx, span := tracing.StartSpan(ctx)
 	defer span.EndSpan()
 
+	span.Debug("Validating that parameters applies to the actions...")
+	tmplParams := m.GetTemplatedParameters()
+	actions := []action{
+		{"install", m.Install},
+		{"upgrade", m.Upgrade},
+		{"uninstall", m.Uninstall},
+	}
+	for actionName, steps := range m.CustomActions {
+		actions = append(actions, action{actionName, steps})
+	}
+	for _, action := range actions {
+		res, err := validateParamsAppliesToAction(m, action.steps, tmplParams, action.name)
+		if err != nil {
+			return nil, span.Error(fmt.Errorf("error validating action: %s", action.name))
+		}
+		results = append(results, res...)
+	}
+
 	span.Debug("Running linters for each mixin used in the manifest...")
 	q := query.New(l.Context, l.Mixins)
 	responses, err := q.Execute(ctx, "lint", query.NewManifestGenerator(m))
@@ -213,3 +239,53 @@ func (l *Linter) Lint(ctx context.Context, m *manifest.Manifest) (Results, error
 
 	return results, nil
 }
+
+func validateParamsAppliesToAction(m *manifest.Manifest, steps manifest.Steps, tmplParams manifest.ParameterDefinitions, actionName string) (Results, error) {
+	var results Results
+	for stepNumber, step := range steps {
+		data, err := yaml.Marshal(step.Data)
+		if err != nil {
+			return nil, fmt.Errorf("error during marshalling: %w", err)
+		}
+
+		tmplResult, err := m.ScanManifestTemplating(data)
+		if err != nil {
+			return nil, fmt.Errorf("error parsing templating: %w", err)
+		}
+
+		for _, variable := range tmplResult.Variables {
+			paramName, ok := m.GetTemplateParameterName(variable)
+			if !ok {
+				continue
+			}
+
+			for _, tmplParam := range tmplParams {
+				if tmplParam.Name != paramName {
+					continue
+				}
+				if !tmplParam.AppliesTo(actionName) {
+					description, err := step.GetDescription()
+					if err != nil {
+						return nil, fmt.Errorf("error getting step description: %w", err)
+					}
+					res := Result{
+						Level: LevelError,
+						Location: Location{
+							Action:          actionName,
+							Mixin:           step.GetMixinName(),
+							StepNumber:      stepNumber + 1,
+							StepDescription: description,
+						},
+						Code:    "porter-101",
+						Title:   "Parameter does not apply to action",
+						Message: fmt.Sprintf("Parameter %s does not apply to %s action", paramName, actionName),
+						URL:     "https://porter.sh/docs/references/linter/#porter-101",
+					}
+					results = append(results, res)
+				}
+			}
+		}
+	}
+
+	return results, nil
+}

pkg/linter/linter_test.go

@@ -2,6 +2,7 @@ package linter
 
 import (
 	"context"
+	"fmt"
 	"testing"
 
 	"get.porter.sh/porter/pkg/manifest"
@@ -173,3 +174,125 @@ func TestLinter_Lint(t *testing.T) {
 		require.NotContains(t, results[0].String(), ": 0th step in the mixin ()")
 	})
 }
+
+func TestLinter_Lint_ParameterDeosNotApplyTo(t *testing.T) {
+	ctx := context.Background()
+	testCases := []struct {
+		action   string
+		setSteps func(*manifest.Manifest, manifest.Steps)
+	}{
+		{"install", func(m *manifest.Manifest, steps manifest.Steps) { m.Install = steps }},
+		{"upgrade", func(m *manifest.Manifest, steps manifest.Steps) { m.Upgrade = steps }},
+		{"uninstall", func(m *manifest.Manifest, steps manifest.Steps) { m.Uninstall = steps }},
+		{"customAction", func(m *manifest.Manifest, steps manifest.Steps) {
+			m.CustomActions = make(map[string]manifest.Steps)
+			m.CustomActions["customAction"] = steps
+		}},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.action, func(t *testing.T) {
+			cxt := portercontext.NewTestContext(t)
+			mixins := mixin.NewTestMixinProvider()
+			l := New(cxt.Context, mixins)
+
+			param := map[string]manifest.ParameterDefinition{
+				"doesNotApply": {
+					Name:    "doesNotApply",
+					ApplyTo: []string{"dummy"},
+				},
+			}
+			steps := manifest.Steps{
+				&manifest.Step{
+					Data: map[string]interface{}{
+						"exec": map[string]interface{}{
+							"description": "exec step",
+							"parameters": []string{
+								"\"${ bundle.parameters.doesNotApply }\"",
+							},
+						},
+					},
+				},
+			}
+			m := &manifest.Manifest{
+				SchemaVersion:     "1.0.1",
+				TemplateVariables: []string{"bundle.parameters.doesNotApply"},
+				Parameters:        param,
+			}
+			tc.setSteps(m, steps)
+
+			lintResults := Results{
+				{
+					Level: LevelError,
+					Location: Location{
+						Action:          tc.action,
+						Mixin:           "exec",
+						StepNumber:      1,
+						StepDescription: "exec step",
+					},
+					Code:    "porter-101",
+					Title:   "Parameter does not apply to action",
+					Message: fmt.Sprintf("Parameter doesNotApply does not apply to %s action", tc.action),
+					URL:     "https://porter.sh/docs/references/linter/#porter-101",
+				},
+			}
+			results, err := l.Lint(ctx, m)
+			require.NoError(t, err, "Lint failed")
+			require.Len(t, results, 1, "linter should have returned 1 result")
+			require.Equal(t, lintResults, results, "unexpected lint results")
+		})
+	}
+}
+
+func TestLinter_Lint_ParameterAppliesTo(t *testing.T) {
+	ctx := context.Background()
+	testCases := []struct {
+		action   string
+		setSteps func(*manifest.Manifest, manifest.Steps)
+	}{
+		{"install", func(m *manifest.Manifest, steps manifest.Steps) { m.Install = steps }},
+		{"upgrade", func(m *manifest.Manifest, steps manifest.Steps) { m.Upgrade = steps }},
+		{"uninstall", func(m *manifest.Manifest, steps manifest.Steps) { m.Uninstall = steps }},
+		{"customAction", func(m *manifest.Manifest, steps manifest.Steps) {
+			m.CustomActions = make(map[string]manifest.Steps)
+			m.CustomActions["customAction"] = steps
+		}},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.action, func(t *testing.T) {
+			cxt := portercontext.NewTestContext(t)
+			mixins := mixin.NewTestMixinProvider()
+			l := New(cxt.Context, mixins)
+
+			param := map[string]manifest.ParameterDefinition{
+				"appliesTo": {
+					Name:    "appliesTo",
+					ApplyTo: []string{tc.action},
+				},
+			}
+			steps := manifest.Steps{
+				&manifest.Step{
+					Data: map[string]interface{}{
+						"exec": map[string]interface{}{
+							"description": "exec step",
+							"parameters": []string{
+								"\"${ bundle.parameters.appliesTo }\"",
+							},
+						},
+					},
+				},
+			}
+			m := &manifest.Manifest{
+				SchemaVersion:     "1.0.1",
+				TemplateVariables: []string{"bundle.parameters.appliesTo"},
+				Parameters:        param,
+			}
+			tc.setSteps(m, steps)
+
+			results, err := l.Lint(ctx, m)
+			require.NoError(t, err, "Lint failed")
+			require.Len(t, results, 0, "linter should have returned 1 result")
+		})
+	}
+}

pkg/manifest/manifest.go

@@ -281,6 +281,19 @@ func (m *Manifest) getTemplateDependencyOutputName(value string) (string, string
 	return dependencyName, outputName, true
 }
 
+var templatedParameterRegex = regexp.MustCompile(`^bundle\.parameters\.(.+)$`)
+
+// GetTemplateParameterName returns the parameter name from the template variable.
+func (m *Manifest) GetTemplateParameterName(value string) (string, bool) {
+	matches := templatedParameterRegex.FindStringSubmatch(value)
+	if len(matches) < 2 {
+		return "", false
+	}
+
+	parameterName := matches[1]
+	return parameterName, true
+}
+
 // GetTemplatedOutputs returns the output definitions for any bundle level outputs
 // that have been templated, keyed by the output name.
 func (m *Manifest) GetTemplatedOutputs() OutputDefinitions {
@@ -314,6 +327,23 @@ func (m *Manifest) GetTemplatedDependencyOutputs() DependencyOutputReferences {
 	return outputs
 }
 
+// GetTemplatedParameters returns the output definitions for any bundle level outputs
+// that have been templated, keyed by the output name.
+func (m *Manifest) GetTemplatedParameters() ParameterDefinitions {
+	parameters := make(ParameterDefinitions, len(m.TemplateVariables))
+	for _, tmplVar := range m.TemplateVariables {
+		if name, ok := m.GetTemplateParameterName(tmplVar); ok {
+			parameterDef, ok := m.Parameters[name]
+			if !ok {
+				// Only return bundle level definitions
+				continue
+			}
+			parameters[name] = parameterDef
+		}
+	}
+	return parameters
+}
+
 // DetermineDependenciesExtensionUsed looks for how dependencies are used
 // by the bundle and which version of the dependency extension can be used.
 func (m *Manifest) DetermineDependenciesExtensionUsed() string {
@@ -1257,7 +1287,7 @@ func ReadManifest(cxt *portercontext.Context, path string) (*Manifest, error) {
 		return nil, fmt.Errorf("unsupported property set or a custom action is defined incorrectly: %w", err)
 	}
 
-	tmplResult, err := m.scanManifestTemplating(data)
+	tmplResult, err := m.ScanManifestTemplating(data)
 	if err != nil {
 		return nil, err
 	}
@@ -1293,7 +1323,7 @@ func (m *Manifest) GetTemplatePrefix() string {
 	return ""
 }
 
-func (m *Manifest) scanManifestTemplating(data []byte) (templateScanResult, error) {
+func (m *Manifest) ScanManifestTemplating(data []byte) (templateScanResult, error) {
 	const disableHtmlEscaping = true
 	templateSrc := m.GetTemplatePrefix() + string(data)
 	tmpl, err := mustache.ParseStringRaw(templateSrc, disableHtmlEscaping)

tests/integration/lint_test.go

@@ -34,5 +34,16 @@ func TestLint(t *testing.T) {
 		cmd.Env("PORTER_VERBOSITY=info")
 	})
 	require.NotContains(t, output, "unknown command", "an unsupported mixin command should not be printed to the console in info")
+}
+
+func TestLint_ApplyToParam(t *testing.T) {
+	test, err := tester.NewTest(t)
+	defer test.Close()
+	require.NoError(t, err, "test setup failed")
 
+	_, output, _ := test.RunPorterWith(func(cmd *shx.PreparedCommand) {
+		cmd.Args("lint")
+		cmd.In(filepath.Join(test.RepoRoot, "tests/integration/testdata/bundles/bundle-with-param-apply-lint-error"))
+	})
+	require.Contains(t, output, "error(porter-101) - Parameter does not apply to action", "parameters being used in actions to which they don't apply should be an error")
 }

tests/integration/testdata/bundles/bundle-with-param-apply-lint-error/porter.yaml

@@ -0,0 +1,39 @@
+# This bundle is designed to cause the porter lint/build commands to fail
+schemaType: Bundle
+schemaVersion: 1.0.1
+name: exec-mixin-lint-error
+version: 0.1.0
+description: "This bundle is designed to cause the porter lint/build commands to fail, use --no-lint to use it anyway"
+registry: "localhost:5000"
+
+mixins:
+  - exec
+
+parameters:
+  - name: onlyApplyToUninstall
+    description: Only applies to uninstall
+    type: string
+    default: appliesToUninstall
+    applyTo:
+      - uninstall
+
+install:
+  - exec:
+      description: trigger a lint error
+      command: echo
+      arguments:
+        - ${ bundle.parameters.onlyApplyToUninstall }
+
+upgrade:
+  - exec:
+      description: "World 2.0"
+      command: echo
+      arguments:
+        - upgrade
+
+uninstall:
+  - exec:
+      description: "Uninstall Hello World"
+      command: echo
+      arguments:
+        - uninstall