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

[TEP-0050] Convert Step OnError from string to "OnErrorType" type #5322

Merged
merged 1 commit into from
Aug 18, 2022
Merged

[TEP-0050] Convert Step OnError from string to "OnErrorType" type #5322

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
426 changes: 262 additions & 164 deletions docs/pipeline-api.md
View file
Open in desktop

Large diffs are not rendered by default.

14 changes: 11 additions & 3 deletions pkg/apis/pipeline/v1/container_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,9 +129,7 @@ type Step struct {

// OnError defines the exiting behavior of a container on error
// 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"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we added support for variables in addition to the two constants - #5307

steps[].onError: $(params.CONTINUE)
steps[].onError: continue
steps[].onError: stopAndFail

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @pritidesai, thanks for letting me know. Was wondering if there is any concern when using variables with "OnErrorType". We will eventually parse the variable to a real value here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How?

I haven't reviewed the code changes in this PR. I am not sure how can it be restricted to the type of enum (continue and stopAndFail) when it can have a parameter of type string.

Copy link
Member Author

@QuanZhang-William QuanZhang-William Aug 17, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also what I found interesting that enum is not that restricted in Go, even with a Typed string OnErrorType you can still do something like

// the value is properly set to step.OnError even if it is not defined in enum
step.OnError = "this is undefined value in enum OnErrorType"

So there is essentially no difference between a regular string or a typed string OnErrorTypewhen calling ApplyReplacement function other than some type casting.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you @QuanZhang-William for all the efforts but declaring onError to as enum but allowing it to specify variables sounds very confusing. If we are declaring a field as enum, it will be simpler and easier to follow if it's restricted to the specified enum values.

I suggest keeping the type to string, thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pritidesai I am not sure I follow here ? The type is still string, as OnErrorType is still just an alias to a string. There is no concept of enum in Go anyway 🙃. Also, this is the exact same thing with a lot of other types we are using (in status, …), it is mainly useful for developer and consumer of the go library to use a predefined set of values, it doesn't do much more than this.

// Stores configuration for the stdout stream of the step.
// +optional
StdoutConfig *StepOutputConfig `json:"stdoutConfig,omitempty"`
Expand All @@ -140,6 +138,16 @@ type Step struct {
StderrConfig *StepOutputConfig `json:"stderrConfig,omitempty"`
}

// OnErrorType defines a list of supported exiting behavior of a container on error
type OnErrorType string

const (
// StopAndFail indicates exit the taskRun if the container exits with non-zero exit code
StopAndFail OnErrorType = "stopAndFail"
// Continue indicates continue executing the rest of the steps irrespective of the container exit code
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.
Expand Down
2 changes: 1 addition & 1 deletion pkg/apis/pipeline/v1/openapi_generated.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion pkg/apis/pipeline/v1/swagger.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -872,7 +872,7 @@
"default": ""
},
"onError": {
"description": "OnError defines the exiting behavior of a container on error 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",
"description": "OnError defines the exiting behavior of a container on error can be set to [ continue | stopAndFail ]",
"type": "string"
},
"resources": {
Expand Down
2 changes: 1 addition & 1 deletion pkg/apis/pipeline/v1/task_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"},
Expand Down
4 changes: 2 additions & 2 deletions pkg/apis/pipeline/v1/task_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"},
}},
Expand Down
4 changes: 2 additions & 2 deletions pkg/apis/pipeline/v1beta1/container_conversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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)
}
Expand Down
14 changes: 11 additions & 3 deletions pkg/apis/pipeline/v1beta1/container_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,9 +188,7 @@ type Step struct {

// OnError defines the exiting behavior of a container on error
// 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
Expand All @@ -200,6 +198,16 @@ type Step struct {
StderrConfig *StepOutputConfig `json:"stderrConfig,omitempty"`
}

// OnErrorType defines a list of supported exiting behavior of a container on error
type OnErrorType string

QuanZhang-William marked this conversation as resolved.
Show resolved Hide resolved
const (
// StopAndFail indicates exit the taskRun if the container exits with non-zero exit code
StopAndFail OnErrorType = "stopAndFail"
// Continue indicates continue executing the rest of the steps irrespective of the container exit code
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.
Expand Down
2 changes: 1 addition & 1 deletion pkg/apis/pipeline/v1beta1/openapi_generated.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion pkg/apis/pipeline/v1beta1/swagger.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1822,7 +1822,7 @@
"default": ""
},
"onError": {
"description": "OnError defines the exiting behavior of a container on error 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",
"description": "OnError defines the exiting behavior of a container on error can be set to [ continue | stopAndFail ]",
"type": "string"
},
"ports": {
Expand Down
2 changes: 1 addition & 1 deletion pkg/apis/pipeline/v1beta1/task_conversion_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"},
}},
Expand Down
5 changes: 2 additions & 3 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 @@ -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"},
Expand Down Expand Up @@ -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
}

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 @@ -1501,22 +1501,22 @@ 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"},
}},
}, {
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)",
Expand Down
2 changes: 1 addition & 1 deletion pkg/container/step_replacements.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
7 changes: 3 additions & 4 deletions pkg/pod/entrypoint.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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())
Expand Down
5 changes: 2 additions & 3 deletions pkg/pod/entrypoint_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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{{
Expand Down