Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: improve step.Script variables references validation message #8312

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+67 −8
Open

feat: improve step.Script variables references validation message #8312

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions pkg/apis/pipeline/v1beta1/pipeline_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"},
},
}, {
Expand Down Expand Up @@ -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"},
},
}, {
Expand Down
4 changes: 2 additions & 2 deletions pkg/apis/pipeline/v1beta1/task_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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))
}
Expand Down
6 changes: 3 additions & 3 deletions pkg/apis/pipeline/v1beta1/task_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"},
},
}, {
Expand Down Expand Up @@ -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"},
},
}, {
Expand Down Expand Up @@ -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,
Expand Down
18 changes: 17 additions & 1 deletion pkg/substitution/substitution.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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{""},
}
Expand Down
43 changes: 43 additions & 0 deletions pkg/substitution/substitution_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down