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.

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

[TEP-0050] Simplify continueAndFail to continue #1075

Merged
merged 1 commit into from
Sep 29, 2023

Conversation

QuanZhang-William
Copy link
Member

@QuanZhang-William QuanZhang-William commented Sep 28, 2023

Prior to this commit, the proposal 2 valid values for PipelineTask OnError field: continueAndFail and StopAndFail. continueAndFail indicates to fail the PipelineTask, continue to execute the DAG, and DO NOT fail the PipelineRun. However, the name continueAndFail is very confusing since

  • continue and Fail sounds like a conflict (see comment)
  • Fail could be interpreted as failing the whole pipelinerun, which is not the expected behavior

This commit simplifies the value to continue. This is more straightforward and more consistent with step.OnError.

/kind tep

Prior to this commit, the proposal 2 valid values for `PipelineTask` `OnError` field: `continueAndFail` and `StopAndFail`.
`continueAndFail` indicates to fail the `PipelineTask`, continue to execute the DAG, and DO NOT fail the `PipelineRun`.
However, the name `continueAndFail` is very confusing since

- 'continue' and 'Fail' sounds like a conflict (see [comment](tektoncd#785 (comment)))
- 'Fail' could be intepreted as failing the whole `pipelinerun`, which is not the expected behavior

This commit simplifies the value to `continue`. This is more straightforward and more consistent with `step.OnError`.

/kind tep
@tekton-robot tekton-robot added the kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). label Sep 28, 2023
@tekton-robot tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 28, 2023
@dibyom
Copy link
Member

dibyom commented Sep 28, 2023

Makes sense to me.
/approve

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 28, 2023
Copy link
Member

@pritidesai pritidesai left a comment

Choose a reason for hiding this comment

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

thanks a bunch @QuanZhang-William for your patience and excited to have this feature 🎉

@pritidesai
Copy link
Member

Since its a small change - renaming one of the options - I think we can merge this

Copy link
Member

@jerop jerop left a comment

Choose a reason for hiding this comment

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

/lgtm

@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom, jerop, pritidesai

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:

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

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 29, 2023
@tekton-robot tekton-robot merged commit b88d0f1 into tektoncd:main Sep 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants