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

…3034) * Lint for parameters being used in actions to which they don't apply Signed-off-by: Kim Christensen <[email protected]> --------- Signed-off-by: Kim Christensen <[email protected]>
getporter · Apr 9, 2024 · 4f540b8 · 4f540b8
1 parent e896bc3
commit 4f540b8
Show file tree

Hide file tree

Showing 6 changed files with 311 additions and 2 deletions.
diff --git a/docs/content/docs/references/linter.md b/docs/content/docs/references/linter.md
@@ -6,6 +6,8 @@ weight: 9
 
 - [exec-100](#exec-100)
 - [porter-100](#porter-100)
+- [porter-101](#porter-101)
+- [porter-102](#porter-102)
 
 ## exec-100
 
@@ -31,6 +33,33 @@ To fix the problem indicated by the porter-100 error, you should replace the pre
 
 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.
+
 ## porter-102
 
 The porter-102 error is a message generated by the porter lint command when it detects that multiple dependencies are defined with the same
@@ -41,3 +70,4 @@ Multiple dependencies with the same name results in undefined behaviour.
 To fix the problem, you should name ensure all dependencies have different names.
 
 You can find more information about dependencies in [Dependencies](/docs/development/authoring-a-bundle/working-with-dependencies/).
+
diff --git a/pkg/linter/linter.go b/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...)
+  }
+
 	deps := make(map[string]interface{}, len(m.Dependencies.Requires))
 	for _, dep := range m.Dependencies.Requires {
 		if _, exists := deps[dep.Name]; exists {
@@ -235,3 +261,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
+}
diff --git a/pkg/linter/linter_test.go b/pkg/linter/linter_test.go
@@ -2,6 +2,7 @@ package linter
 
 import (
 	"context"
+	"fmt"
 	"testing"
 
 	"get.porter.sh/porter/pkg/manifest"
@@ -174,6 +175,128 @@ func TestLinter_Lint(t *testing.T) {
 	})
 }
 
+func TestLinter_Lint_ParameterDoesNotApplyTo(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")
+		})
+	}
+}
+
 func TestLinter_DependencyMultipleTimes(t *testing.T) {
 	t.Run("dependency defined multiple times", func(t *testing.T) {
 		cxt := portercontext.NewTestContext(t)

diff --git a/pkg/manifest/manifest.go b/pkg/manifest/manifest.go
@@ -282,6 +282,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 {
@@ -315,6 +328,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 {
@@ -1258,7 +1288,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
 	}
@@ -1294,7 +1324,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)

diff --git a/tests/integration/lint_test.go b/tests/integration/lint_test.go
@@ -34,7 +34,18 @@ 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")
 }
 
 func TestLint_DependenciesSameName(t *testing.T) {