parameterize steps[].onError

In addition to the static values supported for onError, allow specifying valid values as a task parameter for example, onError: $(params.CONTINUE). Also, validate the specified value is supported otherwise return an error for an invalid value.
tektoncd · Aug 11, 2022 · f25eb35 · f25eb35
1 parent 94a1ba6
commit f25eb35
Show file tree

Hide file tree

Showing 8 changed files with 88 additions and 101 deletions.
diff --git a/docs/variables.md b/docs/variables.md
@@ -160,6 +160,7 @@ variable via `resources.inputs.<resourceName>.<variableName>` or
 | `Task` | `spec.steps[].command` |
 | `Task` | `spec.steps[].args` |
 | `Task` | `spec.steps[].script` |
+| `Task` | `spec.steps[].onError` |
 | `Task` | `spec.steps[].env.value` |
 | `Task` | `spec.steps[].env.valuefrom.secretkeyref.name` |
 | `Task` | `spec.steps[].env.valuefrom.secretkeyref.key` |

diff --git a/examples/v1beta1/pipelineruns/ignore-step-error.yaml b/examples/v1beta1/pipelineruns/ignore-step-error.yaml
@@ -4,14 +4,24 @@ metadata:
   generateName: pipelinerun-with-failing-step-
 spec:
   serviceAccountName: 'default'
+  params:
+    - name: CONTINUE
+      value: "continue"
   pipelineSpec:
+    params:
+      - name: CONTINUE
     tasks:
       - name: task1
+        params:
+          - name: CONTINUE
+            value: "$(params.CONTINUE)"
         taskSpec:
+          params:
+          - name: CONTINUE
           steps:
             # not really doing anything here, just a hurdle to test the "ignore step error"
             - image: alpine
-              onError: continue
+              onError: $(params.CONTINUE)
               name: exit-with-1
               script: |
                 exit 1

diff --git a/pkg/apis/pipeline/v1beta1/task_validation.go b/pkg/apis/pipeline/v1beta1/task_validation.go
@@ -242,16 +242,6 @@ func validateStep(ctx context.Context, s Step, names sets.String) (errs *apis.Fi
 		}
 	}
 
-	if s.OnError != "" {
-		if s.OnError != "continue" && s.OnError != "stopAndFail" {
-			errs = errs.Also(&apis.FieldError{
-				Message: fmt.Sprintf("invalid value: %v", s.OnError),
-				Paths:   []string{"onError"},
-				Details: "Task step onError must be either continue or stopAndFail",
-			})
-		}
-	}
-
 	if s.Script != "" {
 		cleaned := strings.TrimSpace(s.Script)
 		if strings.HasPrefix(cleaned, "#!win") {
@@ -602,6 +592,7 @@ func validateStepVariables(ctx context.Context, step Step, prefix string, vars s
 		errs = errs.Also(validateTaskVariable(v.MountPath, prefix, vars).ViaField("MountPath").ViaFieldIndex("volumeMount", i))
 		errs = errs.Also(validateTaskVariable(v.SubPath, prefix, vars).ViaField("SubPath").ViaFieldIndex("volumeMount", i))
 	}
+	errs = errs.Also(validateTaskVariable(step.OnError, prefix, vars).ViaField("onError"))
 	return errs
 }
 

diff --git a/pkg/apis/pipeline/v1beta1/task_validation_test.go b/pkg/apis/pipeline/v1beta1/task_validation_test.go
@@ -1421,56 +1421,6 @@ func TestStepAndSidecarWorkspacesErrors(t *testing.T) {
 	}
 }
 
-func TestStepOnError(t *testing.T) {
-	tests := []struct {
-		name          string
-		steps         []v1beta1.Step
-		expectedError *apis.FieldError
-	}{{
-		name: "valid step - valid onError usage - set to continue - alpha API",
-		steps: []v1beta1.Step{{
-			OnError: "continue",
-			Image:   "image",
-			Args:    []string{"arg"},
-		}},
-	}, {
-		name: "valid step - valid onError usage - set to stopAndFail - alpha API",
-		steps: []v1beta1.Step{{
-			OnError: "stopAndFail",
-			Image:   "image",
-			Args:    []string{"arg"},
-		}},
-	}, {
-		name: "invalid step - onError set to invalid value - alpha API",
-		steps: []v1beta1.Step{{
-			OnError: "onError",
-			Image:   "image",
-			Args:    []string{"arg"},
-		}},
-		expectedError: &apis.FieldError{
-			Message: fmt.Sprintf("invalid value: onError"),
-			Paths:   []string{"onError"},
-			Details: "Task step onError must be either continue or stopAndFail",
-		},
-	}}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			ts := &v1beta1.TaskSpec{
-				Steps: tt.steps,
-			}
-			ctx := context.Background()
-			ts.SetDefaults(ctx)
-			err := ts.Validate(ctx)
-			if tt.expectedError == nil && err != nil {
-				t.Errorf("TaskSpec.Validate() = %v", err)
-			} else if tt.expectedError != nil && err == nil {
-				t.Errorf("TaskSpec.Validate() = %v", err)
-			}
-		})
-	}
-
-}
-
 // TestIncompatibleAPIVersions exercises validation of fields that
 // require a specific feature gate version in order to work.
 func TestIncompatibleAPIVersions(t *testing.T) {

diff --git a/pkg/container/step_replacements.go b/pkg/container/step_replacements.go
@@ -24,6 +24,7 @@ import (
 // ApplyStepReplacements applies variable interpolation on a Step.
 func ApplyStepReplacements(step *v1beta1.Step, stringReplacements map[string]string, arrayReplacements map[string][]string) {
 	step.Script = substitution.ApplyReplacements(step.Script, stringReplacements)
+	step.OnError = substitution.ApplyReplacements(step.OnError, stringReplacements)
 	if step.StdoutConfig != nil {
 		step.StdoutConfig.Path = substitution.ApplyReplacements(step.StdoutConfig.Path, stringReplacements)
 	}

diff --git a/pkg/container/step_replacements_test.go b/pkg/container/step_replacements_test.go
@@ -42,6 +42,7 @@ func TestApplyStepReplacements(t *testing.T) {
 		Command:    []string{"$(array.replace.me)"},
 		Args:       []string{"$(array.replace.me)"},
 		WorkingDir: "$(replace.me)",
+		OnError:    "$(replace.me)",
 		EnvFrom: []corev1.EnvFromSource{{
 			ConfigMapRef: &corev1.ConfigMapEnvSource{
 				LocalObjectReference: corev1.LocalObjectReference{
@@ -92,6 +93,7 @@ func TestApplyStepReplacements(t *testing.T) {
 		Command:    []string{"val1", "val2"},
 		Args:       []string{"val1", "val2"},
 		WorkingDir: "replaced!",
+		OnError:    "replaced!",
 		EnvFrom: []corev1.EnvFromSource{{
 			ConfigMapRef: &corev1.ConfigMapEnvSource{
 				LocalObjectReference: corev1.LocalObjectReference{

diff --git a/pkg/pod/entrypoint.go b/pkg/pod/entrypoint.go
@@ -28,6 +28,7 @@ import (
 
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline"
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
+	"github.com/tektoncd/pipeline/pkg/entrypoint"
 	"gomodules.xyz/jsonpatch/v2"
 	corev1 "k8s.io/api/core/v1"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
@@ -137,6 +138,10 @@ func orderContainers(commonExtraEntrypointArgs []string, steps []corev1.Containe
 		if taskSpec != nil {
 			if taskSpec.Steps != nil && len(taskSpec.Steps) >= i+1 {
 				if taskSpec.Steps[i].OnError != "" {
+					if taskSpec.Steps[i].OnError != entrypoint.ContinueOnError && taskSpec.Steps[i].OnError != entrypoint.FailOnError {
+						return nil, errors.New(fmt.Sprintf("task step onError must be either %s or %s but it is set to an invalid value %s",
+							entrypoint.ContinueOnError, entrypoint.FailOnError, taskSpec.Steps[i].OnError))
+					}
 					argsForEntrypoint = append(argsForEntrypoint, "-on_error", taskSpec.Steps[i].OnError)
 				}
 				if taskSpec.Steps[i].Timeout != nil {

diff --git a/pkg/pod/entrypoint_test.go b/pkg/pod/entrypoint_test.go
@@ -18,6 +18,7 @@ package pod
 
 import (
 	"context"
+	"errors"
 	"testing"
 	"time"
 
@@ -339,14 +340,6 @@ func TestEntryPointSingleResultsSingleStep(t *testing.T) {
 }
 
 func TestEntryPointOnError(t *testing.T) {
-	taskSpec := v1beta1.TaskSpec{
-		Steps: []v1beta1.Step{{
-			OnError: entrypoint.ContinueOnError,
-		}, {
-			OnError: entrypoint.FailOnError,
-		}},
-	}
-
 	steps := []corev1.Container{{
 		Name:    "failing-step",
 		Image:   "step-1",
@@ -357,42 +350,76 @@ func TestEntryPointOnError(t *testing.T) {
 		Command: []string{"cmd"},
 	}}
 
-	want := []corev1.Container{{
-		Name:    "failing-step",
-		Image:   "step-1",
-		Command: []string{entrypointBinary},
-		Args: []string{
-			"-wait_file", "/tekton/downward/ready",
-			"-wait_file_content",
-			"-post_file", "/tekton/run/0/out",
-			"-termination_path", "/tekton/termination",
-			"-step_metadata_dir", "/tekton/run/0/status",
-			"-on_error", "continue",
-			"-entrypoint", "cmd", "--",
+	for _, tc := range []struct {
+		desc           string
+		taskSpec       v1beta1.TaskSpec
+		wantContainers []corev1.Container
+		err            error
+	}{{
+		taskSpec: v1beta1.TaskSpec{
+			Steps: []v1beta1.Step{{
+				OnError: entrypoint.ContinueOnError,
+			}, {
+				OnError: entrypoint.FailOnError,
+			}},
 		},
-		VolumeMounts:           []corev1.VolumeMount{downwardMount},
-		TerminationMessagePath: "/tekton/termination",
+		wantContainers: []corev1.Container{{
+			Name:    "failing-step",
+			Image:   "step-1",
+			Command: []string{entrypointBinary},
+			Args: []string{
+				"-wait_file", "/tekton/downward/ready",
+				"-wait_file_content",
+				"-post_file", "/tekton/run/0/out",
+				"-termination_path", "/tekton/termination",
+				"-step_metadata_dir", "/tekton/run/0/status",
+				"-on_error", "continue",
+				"-entrypoint", "cmd", "--",
+			},
+			VolumeMounts:           []corev1.VolumeMount{downwardMount},
+			TerminationMessagePath: "/tekton/termination",
+		}, {
+			Name:    "passing-step",
+			Image:   "step-2",
+			Command: []string{entrypointBinary},
+			Args: []string{
+				"-wait_file", "/tekton/run/0/out",
+				"-post_file", "/tekton/run/1/out",
+				"-termination_path", "/tekton/termination",
+				"-step_metadata_dir", "/tekton/run/1/status",
+				"-on_error", "stopAndFail",
+				"-entrypoint", "cmd", "--",
+			},
+			TerminationMessagePath: "/tekton/termination",
+		}},
 	}, {
-		Name:    "passing-step",
-		Image:   "step-2",
-		Command: []string{entrypointBinary},
-		Args: []string{
-			"-wait_file", "/tekton/run/0/out",
-			"-post_file", "/tekton/run/1/out",
-			"-termination_path", "/tekton/termination",
-			"-step_metadata_dir", "/tekton/run/1/status",
-			"-on_error", "stopAndFail",
-			"-entrypoint", "cmd", "--",
+		taskSpec: v1beta1.TaskSpec{
+			Steps: []v1beta1.Step{{
+				OnError: "invalid-on-error",
+			}},
 		},
-		TerminationMessagePath: "/tekton/termination",
-	}}
-	got, err := orderContainers([]string{}, steps, &taskSpec, nil, true)
-	if err != nil {
-		t.Fatalf("orderContainers: %v", err)
-	}
-	if d := cmp.Diff(want, got); d != "" {
-		t.Errorf("Diff %s", diff.PrintWantGot(d))
+		err: errors.New("task step onError must be either continue or stopAndFail but it is set to an invalid value invalid-on-error"),
+	}} {
+		t.Run(tc.desc, func(t *testing.T) {
+			got, err := orderContainers([]string{}, steps, &tc.taskSpec, nil, true)
+			if len(tc.wantContainers) == 0 {
+				if err == nil {
+					t.Fatalf("expected an error for an invalid value for onError but received none")
+				}
+				if d := cmp.Diff(tc.err.Error(), err.Error()); d != "" {
+					t.Errorf("Diff %s", diff.PrintWantGot(d))
+				}
+			} else {
+				if err != nil {
+					t.Fatalf("orderContainers: %v", err)
+				}
+				if d := cmp.Diff(tc.wantContainers, got); d != "" {
+					t.Errorf("Diff %s", diff.PrintWantGot(d))
+				}
+			}
+		})
 	}
+
 }
 
 func TestEntryPointStepOutputConfigs(t *testing.T) {