diff --git a/docs/content/docs/references/linter.md b/docs/content/docs/references/linter.md index 51e59550e..408e75ecf 100644 --- 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 index f96870415..f5e67f8c3 100644 --- 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 index c8f3afe6b..b26239636 100644 --- 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 index d36184096..690565a7b 100644 --- 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 index 4d676110d..b06888298 100644 --- 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) { diff --git a/tests/integration/testdata/bundles/bundle-with-param-apply-lint-error/porter.yaml b/tests/integration/testdata/bundles/bundle-with-param-apply-lint-error/porter.yaml new file mode 100644 index 000000000..b8ed6a64f --- /dev/null +++ b/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