From 15b4e2a291192294e0b2307973406a5038cf8d80 Mon Sep 17 00:00:00 2001 From: Quan Zhang Date: Sun, 14 Aug 2022 18:34:02 -0400 Subject: [PATCH] Convert step onError from string to "OnErrorType" type Prior to this commit, the step onError field is defined as normal string and the 2 supported string values "continue" and "stopAndFail" are directly used across the codebase. This is error-prone and it introduces maintenance difficulty. This commit updates the onError field to be typed string "OnErrorType", with constants defined for the 2 supported values, and updates all the related references --- cmd/entrypoint/main.go | 7 ++++--- pkg/apis/pipeline/v1/container_types.go | 9 ++++++++- pkg/apis/pipeline/v1/task_validation.go | 2 +- pkg/apis/pipeline/v1/task_validation_test.go | 4 ++-- .../pipeline/v1beta1/container_conversion.go | 4 ++-- pkg/apis/pipeline/v1beta1/container_types.go | 9 ++++++++- .../pipeline/v1beta1/task_conversion_test.go | 2 +- pkg/apis/pipeline/v1beta1/task_validation.go | 5 ++--- .../pipeline/v1beta1/task_validation_test.go | 6 +++--- pkg/container/step_replacements.go | 2 +- pkg/entrypoint/entrypointer.go | 8 +++----- pkg/entrypoint/entrypointer_test.go | 19 ++++++++++--------- pkg/pod/entrypoint.go | 7 +++---- pkg/pod/entrypoint_test.go | 5 ++--- 14 files changed, 50 insertions(+), 39 deletions(-) diff --git a/cmd/entrypoint/main.go b/cmd/entrypoint/main.go index 445452f7464..65c46705b4e 100644 --- a/cmd/entrypoint/main.go +++ b/cmd/entrypoint/main.go @@ -30,6 +30,7 @@ import ( "github.com/containerd/containerd/platforms" "github.com/tektoncd/pipeline/cmd/entrypoint/subcommands" "github.com/tektoncd/pipeline/pkg/apis/pipeline" + "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" "github.com/tektoncd/pipeline/pkg/credentials" "github.com/tektoncd/pipeline/pkg/credentials/dockercreds" "github.com/tektoncd/pipeline/pkg/credentials/gitcreds" @@ -146,7 +147,7 @@ func main() { Results: strings.Split(*results, ","), Timeout: timeout, BreakpointOnFailure: *breakpointOnFailure, - OnError: *onError, + OnError: v1beta1.OnErrorType(*onError), StepMetadataDir: *stepMetadataDir, } @@ -175,12 +176,12 @@ func main() { if status, ok := t.Sys().(syscall.WaitStatus); ok { checkForBreakpointOnFailure(e, breakpointExitPostFile) // ignore a step error i.e. do not exit if a container terminates with a non-zero exit code when onError is set to "continue" - if e.OnError != entrypoint.ContinueOnError { + if e.OnError != v1beta1.StopAndFail { os.Exit(status.ExitStatus()) } } // log and exit only if a step error must cause run failure - if e.OnError != entrypoint.ContinueOnError { + if e.OnError != v1beta1.Continue { log.Fatalf("Error executing command (ExitError): %v", err) } default: diff --git a/pkg/apis/pipeline/v1/container_types.go b/pkg/apis/pipeline/v1/container_types.go index c2e041faa04..672bf650484 100644 --- a/pkg/apis/pipeline/v1/container_types.go +++ b/pkg/apis/pipeline/v1/container_types.go @@ -131,7 +131,7 @@ type Step struct { // can be set to [ continue | stopAndFail ] // stopAndFail indicates exit the taskRun if the container exits with non-zero exit code // continue indicates continue executing the rest of the steps irrespective of the container exit code - OnError string `json:"onError,omitempty"` + OnError OnErrorType `json:"onError,omitempty"` // Stores configuration for the stdout stream of the step. // +optional StdoutConfig *StepOutputConfig `json:"stdoutConfig,omitempty"` @@ -140,6 +140,13 @@ type Step struct { StderrConfig *StepOutputConfig `json:"stderrConfig,omitempty"` } +type OnErrorType string + +const ( + StopAndFail OnErrorType = "stopAndFail" + Continue OnErrorType = "continue" +) + // StepOutputConfig stores configuration for a step output stream. type StepOutputConfig struct { // Path to duplicate stdout stream to on container's local filesystem. diff --git a/pkg/apis/pipeline/v1/task_validation.go b/pkg/apis/pipeline/v1/task_validation.go index 8133330c950..fe44cc859c7 100644 --- a/pkg/apis/pipeline/v1/task_validation.go +++ b/pkg/apis/pipeline/v1/task_validation.go @@ -242,7 +242,7 @@ func validateStep(ctx context.Context, s Step, names sets.String) (errs *apis.Fi } if s.OnError != "" { - if s.OnError != "continue" && s.OnError != "stopAndFail" { + if s.OnError != Continue && s.OnError != StopAndFail { errs = errs.Also(&apis.FieldError{ Message: fmt.Sprintf("invalid value: %v", s.OnError), Paths: []string{"onError"}, diff --git a/pkg/apis/pipeline/v1/task_validation_test.go b/pkg/apis/pipeline/v1/task_validation_test.go index fee9ec14d45..be234561e7c 100644 --- a/pkg/apis/pipeline/v1/task_validation_test.go +++ b/pkg/apis/pipeline/v1/task_validation_test.go @@ -1389,14 +1389,14 @@ func TestStepOnError(t *testing.T) { }{{ name: "valid step - valid onError usage - set to continue - alpha API", steps: []v1.Step{{ - OnError: "continue", + OnError: v1.Continue, Image: "image", Args: []string{"arg"}, }}, }, { name: "valid step - valid onError usage - set to stopAndFail - alpha API", steps: []v1.Step{{ - OnError: "stopAndFail", + OnError: v1.StopAndFail, Image: "image", Args: []string{"arg"}, }}, diff --git a/pkg/apis/pipeline/v1beta1/container_conversion.go b/pkg/apis/pipeline/v1beta1/container_conversion.go index d6ed1f60cb4..51826057c54 100644 --- a/pkg/apis/pipeline/v1beta1/container_conversion.go +++ b/pkg/apis/pipeline/v1beta1/container_conversion.go @@ -28,7 +28,7 @@ func (s Step) convertTo(ctx context.Context, sink *v1.Step) { w.convertTo(ctx, &new) sink.Workspaces = append(sink.Workspaces, new) } - sink.OnError = s.OnError + sink.OnError = (v1.OnErrorType)(s.OnError) sink.StdoutConfig = (*v1.StepOutputConfig)(s.StdoutConfig) sink.StderrConfig = (*v1.StepOutputConfig)(s.StderrConfig) @@ -59,7 +59,7 @@ func (s *Step) convertFrom(ctx context.Context, source v1.Step) { new.convertFrom(ctx, w) s.Workspaces = append(s.Workspaces, new) } - s.OnError = source.OnError + s.OnError = (OnErrorType)(source.OnError) s.StdoutConfig = (*StepOutputConfig)(source.StdoutConfig) s.StderrConfig = (*StepOutputConfig)(source.StderrConfig) } diff --git a/pkg/apis/pipeline/v1beta1/container_types.go b/pkg/apis/pipeline/v1beta1/container_types.go index a01a2a9abee..59acf5cc86f 100644 --- a/pkg/apis/pipeline/v1beta1/container_types.go +++ b/pkg/apis/pipeline/v1beta1/container_types.go @@ -190,7 +190,7 @@ type Step struct { // can be set to [ continue | stopAndFail ] // stopAndFail indicates exit the taskRun if the container exits with non-zero exit code // continue indicates continue executing the rest of the steps irrespective of the container exit code - OnError string `json:"onError,omitempty"` + OnError OnErrorType `json:"onError,omitempty"` // Stores configuration for the stdout stream of the step. // +optional @@ -200,6 +200,13 @@ type Step struct { StderrConfig *StepOutputConfig `json:"stderrConfig,omitempty"` } +type OnErrorType string + +const ( + StopAndFail OnErrorType = "stopAndFail" + Continue OnErrorType = "continue" +) + // StepOutputConfig stores configuration for a step output stream. type StepOutputConfig struct { // Path to duplicate stdout stream to on container's local filesystem. diff --git a/pkg/apis/pipeline/v1beta1/task_conversion_test.go b/pkg/apis/pipeline/v1beta1/task_conversion_test.go index 0613683a67f..5b8c5a79ee0 100644 --- a/pkg/apis/pipeline/v1beta1/task_conversion_test.go +++ b/pkg/apis/pipeline/v1beta1/task_conversion_test.go @@ -95,7 +95,7 @@ func TestTaskConversion(t *testing.T) { Script: "echo hello", Timeout: &metav1.Duration{Duration: time.Hour}, Workspaces: []v1beta1.WorkspaceUsage{{Name: "workspace"}}, - OnError: "continue", + OnError: v1beta1.Continue, StdoutConfig: &v1beta1.StepOutputConfig{Path: "/path"}, StderrConfig: &v1beta1.StepOutputConfig{Path: "/another-path"}, }}, diff --git a/pkg/apis/pipeline/v1beta1/task_validation.go b/pkg/apis/pipeline/v1beta1/task_validation.go index f669c8ffe68..065b5404e6c 100644 --- a/pkg/apis/pipeline/v1beta1/task_validation.go +++ b/pkg/apis/pipeline/v1beta1/task_validation.go @@ -243,9 +243,8 @@ func validateStep(ctx context.Context, s Step, names sets.String) (errs *apis.Fi } } - // validate static values in onError if specified - onError can only be set to continue or stopAndFail if s.OnError != "" { - if !isParamRefs(s.OnError) && s.OnError != "continue" && s.OnError != "stopAndFail" { + if !isParamRefs(string(s.OnError)) && s.OnError != Continue && s.OnError != StopAndFail { errs = errs.Also(&apis.FieldError{ Message: fmt.Sprintf("invalid value: %v", s.OnError), Paths: []string{"onError"}, @@ -603,7 +602,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")) + errs = errs.Also(validateTaskVariable(string(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 index 9231840b9e6..93325ecf95b 100644 --- a/pkg/apis/pipeline/v1beta1/task_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/task_validation_test.go @@ -1501,14 +1501,14 @@ func TestStepOnError(t *testing.T) { }{{ name: "valid step - valid onError usage - set to continue", steps: []v1beta1.Step{{ - OnError: "continue", + OnError: v1beta1.Continue, Image: "image", Args: []string{"arg"}, }}, }, { name: "valid step - valid onError usage - set to stopAndFail", steps: []v1beta1.Step{{ - OnError: "stopAndFail", + OnError: v1beta1.StopAndFail, Image: "image", Args: []string{"arg"}, }}, @@ -1516,7 +1516,7 @@ func TestStepOnError(t *testing.T) { name: "valid step - valid onError usage - set to a task parameter", params: []v1beta1.ParamSpec{{ Name: "CONTINUE", - Default: &v1beta1.ArrayOrString{Type: v1beta1.ParamTypeString, StringVal: "continue"}, + Default: &v1beta1.ArrayOrString{Type: v1beta1.ParamTypeString, StringVal: string(v1beta1.Continue)}, }}, steps: []v1beta1.Step{{ OnError: "$(params.CONTINUE)", diff --git a/pkg/container/step_replacements.go b/pkg/container/step_replacements.go index 846b12b00fe..1640e749ce9 100644 --- a/pkg/container/step_replacements.go +++ b/pkg/container/step_replacements.go @@ -24,7 +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) + step.OnError = (v1beta1.OnErrorType)(substitution.ApplyReplacements(string(step.OnError), stringReplacements)) if step.StdoutConfig != nil { step.StdoutConfig.Path = substitution.ApplyReplacements(step.StdoutConfig.Path, stringReplacements) } diff --git a/pkg/entrypoint/entrypointer.go b/pkg/entrypoint/entrypointer.go index 92a13219a21..e80ecac1226 100644 --- a/pkg/entrypoint/entrypointer.go +++ b/pkg/entrypoint/entrypointer.go @@ -37,9 +37,7 @@ import ( // RFC3339 with millisecond const ( - timeFormat = "2006-01-02T15:04:05.000Z07:00" - ContinueOnError = "continue" - FailOnError = "stopAndFail" + timeFormat = "2006-01-02T15:04:05.000Z07:00" ) // Entrypointer holds fields for running commands with redirected @@ -77,7 +75,7 @@ type Entrypointer struct { // OnError defines exiting behavior of the entrypoint // set it to "stopAndFail" to indicate the entrypoint to exit the taskRun if the container exits with non zero exit code // set it to "continue" to indicate the entrypoint to continue executing the rest of the steps irrespective of the container exit code - OnError string + OnError v1beta1.OnErrorType // StepMetadataDir is the directory for a step where the step related metadata can be stored StepMetadataDir string } @@ -162,7 +160,7 @@ func (e Entrypointer) Go() error { switch { case err != nil && e.BreakpointOnFailure: logger.Info("Skipping writing to PostFile") - case e.OnError == ContinueOnError && errors.As(err, &ee): + case e.OnError == v1beta1.Continue && errors.As(err, &ee): // with continue on error and an ExitError, write non-zero exit code and a post file exitCode := strconv.Itoa(ee.ExitCode()) output = append(output, v1beta1.PipelineResourceResult{ diff --git a/pkg/entrypoint/entrypointer_test.go b/pkg/entrypoint/entrypointer_test.go index cb9abe44979..faa365a670a 100644 --- a/pkg/entrypoint/entrypointer_test.go +++ b/pkg/entrypoint/entrypointer_test.go @@ -355,32 +355,33 @@ func TestEntrypointer_ReadBreakpointExitCodeFromDisk(t *testing.T) { func TestEntrypointer_OnError(t *testing.T) { for _, c := range []struct { - desc, postFile, onError string - runner Runner - expectedError bool + desc, postFile string + onError v1beta1.OnErrorType + runner Runner + expectedError bool }{{ desc: "the step is exiting with 1, ignore the step error when onError is set to continue", runner: &fakeExitErrorRunner{}, postFile: "step-one", - onError: ContinueOnError, + onError: v1beta1.Continue, expectedError: true, }, { desc: "the step is exiting with 0, ignore the step error irrespective of no error with onError set to continue", runner: &fakeRunner{}, postFile: "step-one", - onError: ContinueOnError, + onError: v1beta1.Continue, expectedError: false, }, { desc: "the step is exiting with 1, treat the step error as failure with onError set to stopAndFail", runner: &fakeExitErrorRunner{}, expectedError: true, postFile: "step-one", - onError: FailOnError, + onError: v1beta1.StopAndFail, }, { desc: "the step is exiting with 0, treat the step error (but there is none) as failure with onError set to stopAndFail", runner: &fakeRunner{}, postFile: "step-one", - onError: FailOnError, + onError: v1beta1.StopAndFail, expectedError: false, }} { t.Run(c.desc, func(t *testing.T) { @@ -407,7 +408,7 @@ func TestEntrypointer_OnError(t *testing.T) { t.Fatalf("Entrypointer didn't fail") } - if c.onError == ContinueOnError { + if c.onError == v1beta1.Continue { switch { case fpw.wrote == nil: t.Error("Wanted post file written, got nil") @@ -422,7 +423,7 @@ func TestEntrypointer_OnError(t *testing.T) { } } - if c.onError == FailOnError { + if c.onError == v1beta1.StopAndFail { switch { case fpw.wrote == nil: t.Error("Wanted post file written, got nil") diff --git a/pkg/pod/entrypoint.go b/pkg/pod/entrypoint.go index e1e57d4ad98..7be9863f54e 100644 --- a/pkg/pod/entrypoint.go +++ b/pkg/pod/entrypoint.go @@ -28,7 +28,6 @@ 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" @@ -138,11 +137,11 @@ 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 { + if taskSpec.Steps[i].OnError != v1beta1.Continue && taskSpec.Steps[i].OnError != v1beta1.StopAndFail { return nil, fmt.Errorf("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) + v1beta1.Continue, v1beta1.StopAndFail, taskSpec.Steps[i].OnError) } - argsForEntrypoint = append(argsForEntrypoint, "-on_error", taskSpec.Steps[i].OnError) + argsForEntrypoint = append(argsForEntrypoint, "-on_error", string(taskSpec.Steps[i].OnError)) } if taskSpec.Steps[i].Timeout != nil { argsForEntrypoint = append(argsForEntrypoint, "-timeout", taskSpec.Steps[i].Timeout.Duration.String()) diff --git a/pkg/pod/entrypoint_test.go b/pkg/pod/entrypoint_test.go index fd6a8370287..c1309f637d8 100644 --- a/pkg/pod/entrypoint_test.go +++ b/pkg/pod/entrypoint_test.go @@ -24,7 +24,6 @@ import ( "github.com/google/go-cmp/cmp" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" - "github.com/tektoncd/pipeline/pkg/entrypoint" "github.com/tektoncd/pipeline/test/diff" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -358,9 +357,9 @@ func TestEntryPointOnError(t *testing.T) { }{{ taskSpec: v1beta1.TaskSpec{ Steps: []v1beta1.Step{{ - OnError: entrypoint.ContinueOnError, + OnError: v1beta1.Continue, }, { - OnError: entrypoint.FailOnError, + OnError: v1beta1.StopAndFail, }}, }, wantContainers: []corev1.Container{{