From b8bd1e2391f5e69b615ccbf57604746f5c894c68 Mon Sep 17 00:00:00 2001 From: joey Date: Wed, 2 Oct 2024 15:53:42 +0800 Subject: [PATCH] feat: improve step.Script variables references validation message Signed-off-by: chengjoey --- .../v1beta1/pipeline_validation_test.go | 4 +- pkg/apis/pipeline/v1beta1/task_validation.go | 4 +- .../pipeline/v1beta1/task_validation_test.go | 6 +-- pkg/substitution/substitution.go | 18 +++++++- pkg/substitution/substitution_test.go | 43 +++++++++++++++++++ 5 files changed, 67 insertions(+), 8 deletions(-) diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go b/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go index 8faf62c9182..234f7832676 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go @@ -272,7 +272,7 @@ func TestPipeline_Validate_Failure(t *testing.T) { }, }, expectedError: apis.FieldError{ - Message: `non-existent variable in "$(params.doesnotexist)"`, + Message: "non-existent variable `doesnotexist` in \"$(params.doesnotexist)\"", Paths: []string{"spec.tasks[0].steps[0].script"}, }, }, { @@ -303,7 +303,7 @@ func TestPipeline_Validate_Failure(t *testing.T) { }, }, expectedError: apis.FieldError{ - Message: `non-existent variable in "$(params.doesnotexist)"`, + Message: "non-existent variable `doesnotexist` in \"$(params.doesnotexist)\"", Paths: []string{"spec.finally[0].steps[0].script"}, }, }, { diff --git a/pkg/apis/pipeline/v1beta1/task_validation.go b/pkg/apis/pipeline/v1beta1/task_validation.go index 4d03a950125..a4c0725338a 100644 --- a/pkg/apis/pipeline/v1beta1/task_validation.go +++ b/pkg/apis/pipeline/v1beta1/task_validation.go @@ -640,7 +640,7 @@ func validateTaskResultsVariables(ctx context.Context, steps []Step, results []T resultsNames.Insert(r.Name) } for idx, step := range steps { - errs = errs.Also(substitution.ValidateNoReferencesToUnknownVariables(step.Script, "results", resultsNames).ViaField("script").ViaFieldIndex("steps", idx)) + errs = errs.Also(substitution.ValidateNoReferencesToUnknownVariablesWithDetail(step.Script, "results", resultsNames).ViaField("script").ViaFieldIndex("steps", idx)) } return errs } @@ -790,7 +790,7 @@ func validateStepVariables(ctx context.Context, step Step, prefix string, vars s errs := substitution.ValidateNoReferencesToUnknownVariables(step.Name, prefix, vars).ViaField("name") errs = errs.Also(substitution.ValidateNoReferencesToUnknownVariables(step.Image, prefix, vars).ViaField("image")) errs = errs.Also(substitution.ValidateNoReferencesToUnknownVariables(step.WorkingDir, prefix, vars).ViaField("workingDir")) - errs = errs.Also(substitution.ValidateNoReferencesToUnknownVariables(step.Script, prefix, vars).ViaField("script")) + errs = errs.Also(substitution.ValidateNoReferencesToUnknownVariablesWithDetail(step.Script, prefix, vars).ViaField("script")) for i, cmd := range step.Command { errs = errs.Also(substitution.ValidateNoReferencesToUnknownVariables(cmd, prefix, vars).ViaFieldIndex("command", i)) } diff --git a/pkg/apis/pipeline/v1beta1/task_validation_test.go b/pkg/apis/pipeline/v1beta1/task_validation_test.go index fec9351b99e..69bbd1ade2c 100644 --- a/pkg/apis/pipeline/v1beta1/task_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/task_validation_test.go @@ -858,7 +858,7 @@ func TestTaskSpecValidateError(t *testing.T) { Results: []v1beta1.TaskResult{{Name: "a-result"}}, }, expectedError: apis.FieldError{ - Message: `non-existent variable in "\n\t\t\t\t#!/usr/bin/env bash\n\t\t\t\tdate | tee $(results.non-exist.path)"`, + Message: `non-existent variable ` + "`non-exist`" + ` in "\n\t\t\t\t#!/usr/bin/env bash\n\t\t\t\tdate | tee $(results.non-exist.path)"`, Paths: []string{"steps[0].script"}, }, }, { @@ -1417,7 +1417,7 @@ func TestTaskSpecValidateError(t *testing.T) { }}, }, expectedError: apis.FieldError{ - Message: `non-existent variable in "\n\t\t\t\t#!/usr/bin/env bash\n\t\t\t\thello \"$(context.task.missing)\""`, + Message: `non-existent variable ` + "`missing`" + ` in "\n\t\t\t\t#!/usr/bin/env bash\n\t\t\t\thello \"$(context.task.missing)\""`, Paths: []string{"steps[0].script"}, }, }, { @@ -2566,7 +2566,7 @@ func TestTaskSpecValidate_StepResults_Error(t *testing.T) { Results: []v1.StepResult{{Name: "a-result"}}, }, expectedError: apis.FieldError{ - Message: `non-existent variable in "\n\t\t\t#!/usr/bin/env bash\n\t\t\tdate | tee $(results.non-exist.path)"`, + Message: "non-existent variable `non-exist` in \"\\n\\t\\t\\t#!/usr/bin/env bash\\n\\t\\t\\tdate | tee $(results.non-exist.path)\": steps[0].script\nnon-existent variable in \"\\n\\t\\t\\t#!/usr/bin/env bash\\n\\t\\t\\tdate | tee $(results.non-exist.path)\"", Paths: []string{"steps[0].script"}, }, enableStepActions: true, diff --git a/pkg/substitution/substitution.go b/pkg/substitution/substitution.go index 8e1acab2fe1..df428c31bf5 100644 --- a/pkg/substitution/substitution.go +++ b/pkg/substitution/substitution.go @@ -54,6 +54,16 @@ var intIndexRegex = regexp.MustCompile(intIndex) // - prefix: the prefix of the substitutable variable, e.g. "params" or "context.pipeline" // - vars: names of known variables func ValidateNoReferencesToUnknownVariables(value, prefix string, vars sets.String) *apis.FieldError { + return validateNoReferencesToUnknownVariables(value, prefix, vars, false) +} + +// ValidateNoReferencesToUnknownVariablesWithDetail same as ValidateNoReferencesToUnknownVariables +// but with more prefix detailed error message +func ValidateNoReferencesToUnknownVariablesWithDetail(value, prefix string, vars sets.String) *apis.FieldError { + return validateNoReferencesToUnknownVariables(value, prefix, vars, true) +} + +func validateNoReferencesToUnknownVariables(value, prefix string, vars sets.String, withDetail bool) *apis.FieldError { if vs, present, errString := ExtractVariablesFromString(value, prefix); present { if errString != "" { return &apis.FieldError{ @@ -64,8 +74,14 @@ func ValidateNoReferencesToUnknownVariables(value, prefix string, vars sets.Stri for _, v := range vs { v = TrimArrayIndex(v) if !vars.Has(v) { + var msg string + if withDetail { + msg = fmt.Sprintf("non-existent variable `%s` in %q", v, value) + } else { + msg = fmt.Sprintf("non-existent variable in %q", value) + } return &apis.FieldError{ - Message: fmt.Sprintf("non-existent variable in %q", value), + Message: msg, // Empty path is required to make the `ViaField`, … work Paths: []string{""}, } diff --git a/pkg/substitution/substitution_test.go b/pkg/substitution/substitution_test.go index c265510d195..0599008dfba 100644 --- a/pkg/substitution/substitution_test.go +++ b/pkg/substitution/substitution_test.go @@ -189,6 +189,49 @@ func TestValidateNoReferencesToUnknownVariables(t *testing.T) { } } +func TestValidateNoReferencesToUnknownVariablesWithDetail(t *testing.T) { + type args struct { + input string + prefix string + vars sets.String + } + for _, tc := range []struct { + name string + args args + expectedError *apis.FieldError + }{{ + name: "undefined variable", + args: args{ + input: "--flag=$(inputs.params.baz)", + prefix: "inputs.params", + vars: sets.NewString("foo"), + }, + expectedError: &apis.FieldError{ + Message: `non-existent variable ` + "`baz`" + ` in "--flag=$(inputs.params.baz)"`, + Paths: []string{""}, + }, + }, { + name: "undefined individual attributes of an object param", + args: args{ + input: "--flag=$(params.objectParam.key3)", + prefix: "params.objectParam", + vars: sets.NewString("key1", "key2"), + }, + expectedError: &apis.FieldError{ + Message: `non-existent variable ` + "`key3`" + ` in "--flag=$(params.objectParam.key3)"`, + Paths: []string{""}, + }, + }} { + t.Run(tc.name, func(t *testing.T) { + got := substitution.ValidateNoReferencesToUnknownVariablesWithDetail(tc.args.input, tc.args.prefix, tc.args.vars) + + if d := cmp.Diff(tc.expectedError, got, cmp.AllowUnexported(apis.FieldError{})); d != "" { + t.Errorf("ValidateVariableP() error did not match expected error %s", diff.PrintWantGot(d)) + } + }) + } +} + func TestValidateNoReferencesToProhibitedVariables(t *testing.T) { type args struct { input string