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-0045: When Expressions in Finally Tasks #317

Merged
merged 1 commit into from
Jan 27, 2021

Conversation

jerop
Copy link
Member

@jerop jerop commented Jan 25, 2021

Users can guard execution of Tasks using WhenExpressions,
but that is not supported in Finally Tasks.

This TEP describes the need for supporting WhenExpressions in
Finally Tasks not only to provide efficient guarded execution
but also to improve the reusability of Tasks.

Given we've recently added support for Results and Status in
Finally Tasks, this is an opportune time to enable WhenExpressions
in Finally Tasks.

/kind tep
/cc @pritidesai @bobcatfish @sbwsg

@tekton-robot tekton-robot requested review from bobcatfish, pritidesai and a user January 25, 2021 16:00
@tekton-robot tekton-robot added kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 25, 2021
@bobcatfish
Copy link
Contributor

/assign @pritidesai
/assign @sbwsg

@tekton-robot tekton-robot assigned pritidesai and ghost Jan 25, 2021
@jerop jerop force-pushed the whenexpressionsinfinally branch from 16c6333 to 5e5ff7c Compare January 25, 2021 20:18
@jerop jerop changed the title TEP-0045: Guard Finally Tasks with WhenExpressions TEP-0045: Reusability and Guarded Execution of Finally Tasks Jan 25, 2021
@jerop jerop force-pushed the whenexpressionsinfinally branch from 5e5ff7c to 56b6b0b Compare January 25, 2021 20:22
@jerop jerop requested a review from pritidesai January 25, 2021 20:24
@jerop jerop force-pushed the whenexpressionsinfinally branch from 56b6b0b to bee880c Compare January 25, 2021 20:32
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Adding this feature makes sense to me and composes well with the existing access Finally tasks get to results and status. 👍

@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg

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 Jan 26, 2021
@pritidesai
Copy link
Member

thanks @jerop

This looks good in proposed state. Please add more design details while making it implementable.

When expressions are so far evaluated after successful or skipped tasks. Will need to address failure scenario in the when expression evaluation i.e. a result reference in when expression could be missing and the finally task ends up in the list of skipped tasks v/s when expression with mismatching inputs and values evaluating to false. This will be no different than how we have implemented task results in finally.

This also makes me think guard and finally together sounds very odd. Should we change the title again to drop guarding? when expression is bringing reusability rather than guarding it compared to non-finally tasks.

@jerop jerop force-pushed the whenexpressionsinfinally branch from bee880c to 262ce4c Compare January 27, 2021 18:02
@jerop jerop changed the title TEP-0045: Reusability and Guarded Execution of Finally Tasks TEP-0045: Reusability and Guarding of Tasks in Finally Jan 27, 2021
@jerop jerop force-pushed the whenexpressionsinfinally branch 2 times, most recently from e4abd7d to bf680b7 Compare January 27, 2021 18:08
@jerop
Copy link
Member Author

jerop commented Jan 27, 2021

thanks for the reviews @sbwsg & @pritidesai!! 🙏🏾

This looks good in proposed state. Please add more design details while making it implementable.

I've added more details about using Parameters, Results and Execution Status, and also addressed the validation and failure scenarios -- please let me know if there's something more you'd like to see

When expressions are so far evaluated after successful or skipped tasks. Will need to address failure scenario in the when expression evaluation i.e. a result reference in when expression could be missing and the finally task ends up in the list of skipped tasks v/s when expression with mismatching inputs and values evaluating to false. This will be no different than how we have implemented task results in finally.

Yes, in both scenarios, they'd be skipped and added to the Skipped Tasks in status -- another reason to look into adding Reason to Skipped Tasks and revisit the general Status design

If the `WhenExpressions` in a `Finally Task` use `Results` from a skipped or failed non-finally `Tasks`, then the
`Finally Task` would also be skipped and be included in the list of `Skipped Tasks` in the `Status`, [similarly to when
`Results` in other parts of the `Finally Task`](https://github.com/tektoncd/pipeline/blob/master/docs/pipelines.md#consuming-task-execution-results-in-finally).

This also makes me think guard and finally together sounds very odd. Should we change the title again to drop guarding? when expression is bringing reusability rather than guarding it compared to non-finally tasks.

@pritidesai, what do you think of Reusability and Guarding of Tasks in Finally for the title?

I'd say the same argument about WhenExpressions improving reusability of Tasks in Finally also applies to the DAG (non-finally), plus I think excluding guarding in the title would imply this TEP is focused more broadly on the reusability of Finally Tasks beyond supporting WhenExpressions

Users can guard execution of `Tasks` using `WhenExpressions`,
but that is not supported in `Finally Tasks`.

This TEP describes the need for supporting `WhenExpressions` in
`Finally Tasks` not only to provide efficient guarded execution
but also to improve the reusability of `Tasks`.

Given we've recently added support for `Results` and `Status` in
`Finally Tasks`, this is an opportune time to enable `WhenExpressions`
in `Finally Tasks`.
@jerop jerop force-pushed the whenexpressionsinfinally branch from bf680b7 to 9cefbba Compare January 27, 2021 19:29
@jerop jerop changed the title TEP-0045: Reusability and Guarding of Tasks in Finally TEP-0045: When Expressions in Finally Tasks Jan 27, 2021
@pritidesai
Copy link
Member

thanks @jerop, we could add more relevant examples for using task results and params with when expressions but its not blocking this proposal.

Thanks for all the updates 🙏

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 27, 2021
@tekton-robot tekton-robot merged commit 7a3f189 into tektoncd:master Jan 27, 2021
jerop added a commit to jerop/community that referenced this pull request Jan 28, 2021
…ntable

We proposed supporting WhenExpressions in Finally Tasks in
tektoncd#317, and the TEP has a
proposed status now

TEP: https://github.com/tektoncd/community/blob/master/teps/0045-whenexpressions-in-finally-tasks.md

In this PR, we want to change TEP status to implementable and provide
modify the examples provided for using `Results` and `Parameters`
jerop added a commit to jerop/community that referenced this pull request Jan 28, 2021
…ntable

We proposed supporting WhenExpressions in Finally Tasks in
tektoncd#317, and the TEP has a
proposed status now

TEP: https://github.com/tektoncd/community/blob/master/teps/0045-whenexpressions-in-finally-tasks.md

In this PR, we want to change TEP status to implementable and update
the examples provided for using `Results` and `Parameters`
jerop added a commit to jerop/community that referenced this pull request Jan 28, 2021
…ntable

We proposed supporting WhenExpressions in Finally Tasks in
tektoncd#317, and the TEP has a
proposed status now

TEP: https://github.com/tektoncd/community/blob/master/teps/0045-whenexpressions-in-finally-tasks.md

In this PR, we want to change TEP status to implementable and update
the examples provided for using `Execution Status`, `Results` and `Parameters`
jerop added a commit to jerop/community that referenced this pull request Jan 28, 2021
…ntable

We proposed supporting WhenExpressions in Finally Tasks in
tektoncd#317, and the TEP has a
proposed status now

TEP: https://github.com/tektoncd/community/blob/master/teps/0045-whenexpressions-in-finally-tasks.md

In this PR, we want to change TEP status to implementable and update
the examples provided for using `Execution Status`, `Results` and `Parameters`
tekton-robot pushed a commit that referenced this pull request Jan 28, 2021
…ntable

We proposed supporting WhenExpressions in Finally Tasks in
#317, and the TEP has a
proposed status now

TEP: https://github.com/tektoncd/community/blob/master/teps/0045-whenexpressions-in-finally-tasks.md

In this PR, we want to change TEP status to implementable and update
the examples provided for using `Execution Status`, `Results` and `Parameters`
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
None yet
Development

Successfully merging this pull request may close these issues.

4 participants