[TEP-0050] Add OnError field by QuanZhang-William · Pull Request #7162 · tektoncd/pipeline

QuanZhang-William · 2023-09-28T22:36:22Z

Changes

Part of #7165. In TEP-0050, we proposed to add an OnError API field under PipelineTask to configure error handling strategy.

This commits add the new OnError API field and the related documentations. The validation and business logic will be added in the follow-up PRs.

Note: OnError is in preview mode and not yet supported.

/kind feature

Submitter Checklist

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

Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
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). See some examples of good release notes.
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

None

tekton-robot · 2023-09-28T22:36:35Z

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

chitrangpatel · 2023-09-29T17:01:50Z

docs/pipelines.md

+> :seedling: This feature is in **Preview Only** mode and not yet supported/implemented.
+
+When a `PipelineTask` fails, the rest of the `PipelineTasks` are skipped and the `PipelineRun` is declared a failure. If you would like to
+ignore such `PipelineTask` failure and continue executing the rest of the `PipelineTasks`, you can specify `onError` for such a `PipelineTask`.


What about the case when the Task produces a result that may be needed downstream or by the pipeline?

Wouldn't setting it to just "continue" without providing a value to the outputs of the task make it fail?

Sorry! I spoke too soon. You already go over that use case below.

chitrangpatel · 2023-09-29T17:09:29Z

docs/pipelines.md

+```
+
+- If the consuming `PipelineTask` has `OnError:stopAndFail`, the `PipelineRun` will fail with `InvalidTaskResultReference`.
+- If the consuming `PipelineTask` has `OnError:continue`, the consuming `PipelineTask` will be skipped with reason `Results were missing`, 


Just confirming. This would ensure that all the subsequent Tasks that in-turn depend on this skipped Task would also be skipped right?

Yeah, and to be more specific:

all subsequent resource-dependent tasks (with onError:continue) would be skipped if the result is not emitted.

chitrangpatel · 2023-09-29T17:12:49Z

Thanks for the PR @QuanZhang-William. I think the v1beta1 types will also require relevant conversion functions. I suppose you are going to add that in a follow-up PR?
I would recommend adding the type completely in a single PR (i.e. conversion, defaults and empty validations ). And then in the followup PR, you can add actual validation followed by controller logic. WDYT?

Yongxuanzhang · 2023-09-29T17:51:27Z

why don't we add the docs when the feature is implemented?

QuanZhang-William · 2023-09-29T18:10:57Z

Thanks for the PR @QuanZhang-William. I think the v1beta1 types will also require relevant conversion functions. I suppose you are going to add that in a follow-up PR? I would recommend adding the type completely in a single PR (i.e. conversion, defaults and empty validations ). And then in the followup PR, you can add actual validation followed by controller logic. WDYT?

Hi @chitrangpatel, sure I can put them in a single PR. I don't think we need to apply default values for this field though (which is same for step.OnError).

QuanZhang-William · 2023-09-29T18:14:54Z

why don't we add the docs when the feature is implemented?

Hi @Yongxuanzhang. Seems like this the pattern we follow recently:

I feel users could be confused if we add an API field but has no documentation on it. I have called out in the doc that the feature is WIP and do not use it.

Yongxuanzhang

/lgtm

pkg/apis/pipeline/v1/pipeline_types.go

Yongxuanzhang · 2023-09-29T18:46:11Z

Could you also link the issue to track all the work plan? Thanks!

tekton-robot · 2023-09-29T20:58:24Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage
pkg/apis/pipeline/v1/pipeline_validation.go	99.1%	99.2%
pkg/apis/pipeline/v1beta1/pipeline_conversion.go	95.3%	95.3%
pkg/apis/pipeline/v1beta1/pipeline_validation.go	98.8%	98.8%

tekton-robot · 2023-09-29T21:00:17Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage
pkg/apis/pipeline/v1/pipeline_validation.go	99.1%	99.2%
pkg/apis/pipeline/v1beta1/pipeline_conversion.go	95.3%	95.3%
pkg/apis/pipeline/v1beta1/pipeline_validation.go	98.8%	98.8%

QuanZhang-William · 2023-09-29T21:02:33Z

Could you also link the issue to track all the work plan? Thanks!

Sure, tracking issue added: #7165

tekton-robot · 2023-10-11T21:00:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Yongxuanzhang

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [Yongxuanzhang]

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

tekton-robot · 2023-10-11T21:31:53Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage
pkg/apis/pipeline/v1/pipeline_validation.go	99.1%	99.2%
pkg/apis/pipeline/v1beta1/pipeline_conversion.go	95.3%	95.3%
pkg/apis/pipeline/v1beta1/pipeline_validation.go	98.8%	98.8%

tekton-robot · 2023-10-11T21:34:16Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage
pkg/apis/pipeline/v1/pipeline_validation.go	99.1%	99.2%
pkg/apis/pipeline/v1beta1/pipeline_conversion.go	95.3%	95.3%
pkg/apis/pipeline/v1beta1/pipeline_validation.go	98.8%	98.8%

docs/pipelines.md

tekton-robot · 2023-10-12T20:32:57Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage
pkg/apis/pipeline/v1/pipeline_validation.go	99.1%	99.2%
pkg/apis/pipeline/v1beta1/pipeline_conversion.go	95.3%	95.3%
pkg/apis/pipeline/v1beta1/pipeline_validation.go	98.8%	98.8%

tekton-robot · 2023-10-12T20:34:27Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage
pkg/apis/pipeline/v1/pipeline_validation.go	99.1%	99.2%
pkg/apis/pipeline/v1beta1/pipeline_conversion.go	95.3%	95.3%
pkg/apis/pipeline/v1beta1/pipeline_validation.go	98.8%	98.8%

dibyom · 2023-10-16T15:59:22Z

@QuanZhang-William I think we should remove the release note - release notes are aimed towards users and since the feature doesn't work yet, I think it is confusing to add it

otherwise, LGTM

dibyom · 2023-10-16T18:26:14Z

/lgtm

QuanZhang-William · 2023-10-17T13:40:36Z

/retest

In [TEP-0050][tep-0050], we proposed to add an `OnError` API field under `PipelineTask` to configure error handling strategy. This commits add the new `OnError` API field and the related validation, conversion and validation. The business logic will be added in the follow-up PRs. Note: OnError is in preview mode and not yet supported. /kind feature [tep-0050]: https://github.com/tektoncd/community/blob/main/teps/0050-ignore-task-failures.md

tekton-robot · 2023-10-17T14:26:48Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage
pkg/apis/pipeline/v1/pipeline_validation.go	99.1%	99.2%
pkg/apis/pipeline/v1beta1/pipeline_conversion.go	95.3%	95.3%
pkg/apis/pipeline/v1beta1/pipeline_validation.go	98.8%	98.8%

tekton-robot · 2023-10-17T14:28:08Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage
pkg/apis/pipeline/v1/pipeline_validation.go	99.1%	99.2%
pkg/apis/pipeline/v1beta1/pipeline_conversion.go	95.3%	95.3%
pkg/apis/pipeline/v1beta1/pipeline_validation.go	98.8%	98.8%

JeromeJu · 2023-10-17T14:31:14Z

/lgtm

QuanZhang-William · 2023-10-17T15:53:43Z

/test pull-tekton-pipeline-integration-tests
due to timeout

tekton-robot requested review from pritidesai and vdemeester September 28, 2023 22:36

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesnt merit a release note. labels Sep 29, 2023

QuanZhang-William marked this pull request as ready for review September 29, 2023 13:57

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 29, 2023

tekton-robot requested a review from dibyom September 29, 2023 13:57

tekton-robot assigned JeromeJu Sep 29, 2023

chitrangpatel reviewed Sep 29, 2023

View reviewed changes

Yongxuanzhang approved these changes Sep 29, 2023

View reviewed changes

pkg/apis/pipeline/v1/pipeline_types.go Outdated Show resolved Hide resolved

tekton-robot assigned Yongxuanzhang Sep 29, 2023

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 29, 2023

QuanZhang-William force-pushed the tep-0050-onerror-field branch from 9acd0bd to 8b55c08 Compare September 29, 2023 20:51

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 29, 2023

QuanZhang-William changed the title ~~[TEP-0050] Add OnError field and documentation~~ [TEP-0050] Add OnError field Sep 29, 2023

QuanZhang-William force-pushed the tep-0050-onerror-field branch from 8b55c08 to a97c1e8 Compare September 29, 2023 21:01

Yongxuanzhang approved these changes Oct 11, 2023

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 11, 2023

QuanZhang-William force-pushed the tep-0050-onerror-field branch from 7989360 to 3729c9a Compare October 11, 2023 21:26

pritidesai reviewed Oct 12, 2023

View reviewed changes

docs/pipelines.md Outdated Show resolved Hide resolved

QuanZhang-William force-pushed the tep-0050-onerror-field branch from 3729c9a to ec04b1d Compare October 12, 2023 20:26

QuanZhang-William requested review from dibyom and pritidesai October 16, 2023 14:21

JeromeJu mentioned this pull request Oct 16, 2023

[TEP0138] Add Per-feature Flag Struct for New Features #7090

Merged

7 tasks

tekton-robot assigned dibyom Oct 16, 2023

tekton-robot added lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesnt merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Oct 16, 2023

QuanZhang-William force-pushed the tep-0050-onerror-field branch from ec04b1d to c2185d9 Compare October 17, 2023 14:20

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 17, 2023

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 17, 2023

tekton-robot merged commit c9ff759 into tektoncd:main Oct 17, 2023

QuanZhang-William mentioned this pull request Nov 27, 2023

TEP-0050: Ignore Task Failure Implementation #7165

Closed

4 tasks

Conversation

QuanZhang-William commented Sep 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Submitter Checklist

Release Notes

Uh oh!

tekton-robot commented Sep 28, 2023

Uh oh!

chitrangpatel Sep 29, 2023

Choose a reason for hiding this comment

Uh oh!

chitrangpatel Sep 29, 2023

Choose a reason for hiding this comment

Uh oh!

chitrangpatel Sep 29, 2023

Choose a reason for hiding this comment

Uh oh!

chitrangpatel Sep 29, 2023

Choose a reason for hiding this comment

Uh oh!

QuanZhang-William Sep 29, 2023

Choose a reason for hiding this comment

Uh oh!

chitrangpatel commented Sep 29, 2023

Uh oh!

Yongxuanzhang commented Sep 29, 2023

Uh oh!

QuanZhang-William commented Sep 29, 2023

Uh oh!

QuanZhang-William commented Sep 29, 2023

Uh oh!

Yongxuanzhang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Yongxuanzhang commented Sep 29, 2023

Uh oh!

tekton-robot commented Sep 29, 2023

Uh oh!

tekton-robot commented Sep 29, 2023

Uh oh!

QuanZhang-William commented Sep 29, 2023

Uh oh!

tekton-robot commented Oct 11, 2023

Uh oh!

tekton-robot commented Oct 11, 2023

Uh oh!

tekton-robot commented Oct 11, 2023

Uh oh!

Uh oh!

tekton-robot commented Oct 12, 2023

Uh oh!

tekton-robot commented Oct 12, 2023

Uh oh!

dibyom commented Oct 16, 2023

Uh oh!

dibyom commented Oct 16, 2023

Uh oh!

QuanZhang-William commented Oct 17, 2023

Uh oh!

tekton-robot commented Oct 17, 2023

Uh oh!

tekton-robot commented Oct 17, 2023

Uh oh!

JeromeJu commented Oct 17, 2023

Uh oh!

QuanZhang-William commented Oct 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

QuanZhang-William commented Sep 28, 2023 •

edited

Loading