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

[QuanZhang-William]

Changes

Prior to this commit, the step.OnError field is defined as 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

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

• Has Docs included if any changes are user facing
• Has Tests included if any functionality added or changed
• Follows the commit message standard
• Meets the Tekton contributor standards (including
  functionality, content, code)
• Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
• Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)
• Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

Convert step.OnError from string to type: OnErrorType

[tekton-robot]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[QuanZhang-William]

/kind cleanup

[QuanZhang-William]

/test all

[QuanZhang-William]

/test all

[QuanZhang-William]

/retest

[QuanZhang-William]

/retest

[QuanZhang-William]

/retest

[QuanZhang-William]

/retest pull-tekton-pipeline-unit-tests

[tekton-robot]

@QuanZhang-William: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

• /test pull-tekton-pipeline-alpha-integration-tests
• /test pull-tekton-pipeline-build-tests
• /test pull-tekton-pipeline-integration-tests
• /test tekton-pipeline-unit-tests

The following commands are available to trigger optional jobs:

• /test pull-tekton-pipeline-go-coverage

Use /test all to run all jobs.

In response to this:

/retest pull-tekton-pipeline-unit-tests

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[QuanZhang-William]

/retest

[QuanZhang-William]

/retest

[wlynch]

Mostly LGTM! Just one change I think we should make.

[wlynch]

/lgtm

[Yongxuanzhang]

Great work!
Maybe adding a release note?

[QuanZhang-William]

Great work! Maybe adding a release note?

My understanding of a Release Note is to notify users about a behaviour change. There is actually no change from the user's perspective and there is nothing to "notify" (since we already have the OnError input validation before this PR). This is more like a "clean up " work. I don't think a Release Note is necessary in this case?

[wlynch]

My understanding of a Release Note is to notify users about a behaviour change. There is actually no change from the user's perspective and there is nothing to "notify" (since we already have the OnError input validation before this PR). This is more like a "clean up " work. I don't think a Release Note is necessary in this case?

Good catch @Yongxuanzhang! - This should be called out as a breaking change to the API - this will cause build failures for clients if they were previously using the string based type, i.e.

s := "asdf"
v1beta1.Step{
	OnError: s,
}

[pritidesai]

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

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

[QuanZhang-William]

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.

[pritidesai]

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.

[QuanZhang-William]

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.

[pritidesai]

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!

[vdemeester]

@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.

[QuanZhang-William]

My understanding of a Release Note is to notify users about a behaviour change. There is actually no change from the user's perspective and there is nothing to "notify" (since we already have the OnError input validation before this PR). This is more like a "clean up " work. I don't think a Release Note is necessary in this case?

Good catch @Yongxuanzhang! - This should be called out as a breaking change to the API - this will cause build failures for clients if they were previously using the string based type, i.e.

s := "asdf"
v1beta1.Step{
	OnError: s,
}

Ohh, got what you mean. I didn't think about that Tekton can be used in this way programmatically. Will update the Release Note in this case

[vdemeester]

Ohh, got what you mean. I didn't think about that Tekton can be used in this way programmatically. Will update the Release Note in this case

Mainly the cli or go client might be affected 🙃

[vdemeester]

@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.

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester, wlynch

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [vdemeester]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[QuanZhang-William]

/retest

[pritidesai]

@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.

I was a little surprised to discover this was merged while going over the release notes for 0.39 🤔

step.onError was recently changed to support params in addition to constants. step.onError is something user specified through params unlike other types in status which are mostly set by the controller or specified as static values by the users (unless that's not true) in their YAML specifications.

OnErrorType represents a false impression that it only accepts static values (continue and stopAndFail). Why unnecessary creating such type and then converting it back to string in many places?

[vdemeester]

OnErrorType represents a false impression that it only accepts static values (continue and stopAndFail). Why unnecessary creating such type and then converting it back to string in many places?

From a API consumer point of view, nothing changes, it's still "just a string" nothing else. 99% of users of Tekton won't read code, just docs, and OnErrorType doesn't bubble up anywhere in the API or the doc. This is a pure "implementation detail" change that can help developer code overall when working on the controller's code.

[pritidesai]

thank you @vdemeester for being patient here, appreciate it 🙏

I don't know how are these interpreted - https://tekton.dev/docs/pipelines/pipeline-api/#tekton.dev/v1.Step This API doc has listed onError as OnErrorType.

The source for this doc is here - https://github.com/tektoncd/pipeline/blob/main/docs/pipeline-api.md#tekton.dev/v1.Step

[QuanZhang-William]

thank you @vdemeester for being patient here, appreciate it 🙏

I don't know how are these interpreted - https://tekton.dev/docs/pipelines/pipeline-api/#tekton.dev/v1.Step This API doc has listed onError as OnErrorType.

The source for this doc is here - https://github.com/tektoncd/pipeline/blob/main/docs/pipeline-api.md#tekton.dev/v1.Step

Thanks for your feedback @pritidesai! The OnErrorType is documented as (string Alias) here: https://tekton.dev/docs/pipelines/pipeline-api/#tekton.dev/v1.OnErrorType. I think the question is essentially how does a user interpret a string alias in GO. My understanding is that a string alias is not a enum and there is no real enum in GO: golang/go#19814.

The string value "continue" and "stopAndFail" are being directly used across the codebase and it is in general not a good practice. We can also benefit a better compile time check from such typed string, for example, pipelineTask.OnError = step.OnError (after TEP-0050) cannot happen as they are different things semantically.

I think we can keep the OnErrorType for a better practice. And I can update the API doc explicitly saying something like: The value can be set dynamically from task parameter for example, onError: $(params. CONTINUE) to resolve user's confusion if any. What do you think?

/cc @jerop, @dibyom

[dibyom]

I think we can keep the OnErrorType for a better practice. And I can update the API doc explicitly saying something like: The value can be set dynamically from task parameter for example, onError: $(params. CONTINUE) to resolve user's confusion if any. What do you think?

https://tekton.dev/docs/pipelines/variables/#fields-that-accept-variable-substitutions already lists onError - I think that's sufficient? (There are other non "string" string types that also accept variable substitutions e.g. imagePullPolicy)

[QuanZhang-William]

I think we can keep the OnErrorType for a better practice. And I can update the API doc explicitly saying something like: The value can be set dynamically from task parameter for example, onError: $(params. CONTINUE) to resolve user's confusion if any. What do you think?

https://tekton.dev/docs/pipelines/variables/#fields-that-accept-variable-substitutions already lists onError - I think that's sufficient? (There are other non "string" string types that also accept variable substitutions e.g. imagePullPolicy)

Yeah, I agree it should be sufficient in this case :)