feat: improve step.Script variables references validation message

Signed-off-by: chengjoey <[email protected]>
tektoncd · Oct 2, 2024 · c4e32f3 · c4e32f3
1 parent c07e353
commit c4e32f3
Show file tree

Hide file tree

Showing 4 changed files with 24 additions and 8 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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{""},
 				}