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-0103: Skipping Reason #671

Merged
merged 1 commit into from
Apr 18, 2022
Merged

TEP-0103: Skipping Reason #671

merged 1 commit into from
Apr 18, 2022

Conversation

jerop
Copy link
Member

@jerop jerop commented Apr 6, 2022

Today, users only know that a PipelineTask was skipped, but they don't know for which exact reason. In this TEP, we propose adding the reason for skipping to SkippedTasks field in PipelineRunStatus to improve usability and debuggability.

References:

@tekton-robot tekton-robot requested review from imjasonh and wlynch April 6, 2022 11:17
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 6, 2022
@lbernick
Copy link
Member

lbernick commented Apr 7, 2022

/assign

Copy link
Member

@lbernick lbernick left a comment

Choose a reason for hiding this comment

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

I think this will be really useful! :)

[skipped-tasks]: https://github.com/tektoncd/pipeline/blob/053833cb10f3829d5a366daa1f431b293dcf3285/pkg/apis/pipeline/v1beta1/pipelinerun_types.go#L466-L476
[issue-4738]: https://github.com/tektoncd/pipeline/issues/4738
[issue-4571]: https://github.com/tektoncd/pipeline/issues/4571
[slack]: https://tektoncd.slack.com/archives/CK3HBG7CM/p1642349040014100
Copy link
Member

Choose a reason for hiding this comment

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

just fyi I don't think this slack link is available any longer, do you have a screenshot or text snippet of what was said? if not it's not a big deal

Copy link
Member Author

Choose a reason for hiding this comment

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

we lose threads because we don't have a premium account so it's capped at 10K messages - added an item to the community meeting to discuss getting a premium subscription because it's our main means of communication

unfortunately, I didn't take a screenshot of that particular thread - hopefully we can recover it and others if we get a paid subscription 🤞🏾

Name string `json:"name"`

// Reason is the cause of the PipelineTask being skipped
Reason SkippingReason `json:"reason"`
Copy link
Member

Choose a reason for hiding this comment

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

any reason not to put this info into TaskRun.Status.Condition.Reason instead of creating a separate field? what is in the Condition.Reason when a TaskRun is skipped?

Copy link
Member Author

@jerop jerop Apr 8, 2022

Choose a reason for hiding this comment

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

any reason not to put this info into TaskRun.Status.Condition.Reason instead of creating a separate field? what is in the Condition.Reason when a TaskRun is skipped?

We do not create a TaskRun for a skipped PipelineTask so there's no TaskRun.Status.Condition.Reason field for a skipped PipelineTask -- this is why we added the SkippedTasks field to the PipelineRun to list all the skipped PipelineTasks

Copy link
Member

Choose a reason for hiding this comment

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

ah whoops 😬

@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 8, 2022
Today, users only know that a `PipelineTask` was skipped, but they
don't know for which exact reason.

In this TEP, we propose adding the reason for skipping to `SkippedTasks`
field in `PipelineRunStatus` to improve usability and debuggability.
@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 11, 2022
@jerop
Copy link
Member Author

jerop commented Apr 11, 2022

/kind tep

@tekton-robot tekton-robot added the kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). label Apr 11, 2022
@bobcatfish
Copy link
Contributor

/assign vdemeester


There are many [reasons][reasons] why a `PipelineTask` could be skipped, including:
* at least one of its `when` expressions evaluated to false
* at least one of its `Conditions` failed
Copy link
Member

Choose a reason for hiding this comment

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

As Conditions are deprecated and will soon be removed, should we even try to support that ? 😛

@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 15, 2022
@dibyom
Copy link
Member

dibyom commented Apr 18, 2022

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 18, 2022
@tekton-robot tekton-robot merged commit 58f0551 into tektoncd:main Apr 18, 2022
@jerop jerop deleted the tep-0103 branch June 11, 2022 01:57
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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Status: UnAssigned
Development

Successfully merging this pull request may close these issues.

6 participants