From c4e32f3715608d678297c28eaffceae5dead89b9 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 +++++++++++++++++- 4 files changed, 24 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{""}, }