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

Invalidate passing Results between Finally Tasks #4132

Merged
merged 1 commit into from
Aug 2, 2021

Conversation

jerop
Copy link
Member

@jerop jerop commented Jul 30, 2021

Today, passing Results between Finally Tasks through when
expressions is not invalidated

This change invalidates passing Results between Finally Tasks through
when expressions

Finally Tasks should be executed in parallel after all Tasks are
done, therefore we cannot have dependencies between Finally Tasks

A Pipeline that passes a Result from one Finally Task to another
Finally Task should fail due to validation error

Further details in:

This change also refactors the invalidation of Finally Tasks to ensure
that we capture all the reasons to fail due to invalidation instead of
only one reason -- to ensure all invalid syntax in Finally Tasks are
reported to the user at the first execution

Fixes #4130

Changes

Submitter Checklist

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

  • Docs included if any changes are user facing
  • Tests included if any functionality added or changed
  • Follows the commit message standard
  • Meets the Tekton contributor standards (including
    functionality, content, code)
  • Release notes block below has been filled in or deleted (only if no user facing changes)

Release Notes

- passing `Results` between `Finally Tasks` is invalidated
- when a `Pipeline` has validation errors in `Finally Tasks`, all of them will be returned in the first execution of the `Pipeline` instead of one by one

/kind bug

@tekton-robot tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. labels Jul 30, 2021
@tekton-robot tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 30, 2021
@jerop jerop requested a review from pritidesai July 30, 2021 17:49
Today, passing `Results` between `Finally Tasks` through `when`
expressions is not invalidated

This change invalidates passing `Results` between `Finally Tasks` through
`when` expressions

`Finally Tasks` should be executed in parallel after all `Tasks` are
done, therefore we cannot have dependencies between `Finally Tasks`

A `Pipeline` that passes a `Result` from one `Finally Task` to another
`Finally Task` should fail due to validation error

Further details in:
- [TEP-0004: Task Results in Final Tasks](https://github.com/tektoncd/community/blob/main/teps/0004-task-results-in-final-tasks.md)
- [Documentation: Consuming Results in Finally Tasks](https://github.com/tektoncd/pipeline/blob/main/docs/pipelines.md#consuming-task-execution-results-in-finally)

This change also refactors the invalidation of `Finally Tasks` to ensure
that we capture all the reasons to fail due to invalidation instead of
only one reason -- to ensure all invalid syntax in `Finally Tasks` are
reported to the user at the first execution

Fixes tektoncd#4130
@jerop
Copy link
Member Author

jerop commented Jul 30, 2021

/retest

@jerop jerop added this to the Pipelines v0.27 milestone Jul 30, 2021
@jerop
Copy link
Member Author

jerop commented Jul 30, 2021

/test pull-tekton-pipeline-integration-tests
/test pull-tekton-pipeline-alpha-integration-tests

@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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 Aug 2, 2021
@ghost
Copy link

ghost commented Aug 2, 2021

/lgtm

@tekton-robot tekton-robot assigned ghost Aug 2, 2021
@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 2, 2021
@jerop
Copy link
Member Author

jerop commented Aug 2, 2021

/easycla

@vdemeester
Copy link
Member

@jerop I think the only way to force EasyCLA is to close/re-open (or force push)

@vdemeester vdemeester closed this Aug 2, 2021
@vdemeester vdemeester reopened this Aug 2, 2021
@jerop
Copy link
Member Author

jerop commented Aug 2, 2021

@vdemeester aah i see, thank you!

@vdemeester
Copy link
Member

Well, it failed miserably… seems like we have a problem with EasyCLA 🤔

@vdemeester vdemeester closed this Aug 2, 2021
@vdemeester vdemeester reopened this Aug 2, 2021
@tekton-robot tekton-robot merged commit 5f1337c into tektoncd:main Aug 2, 2021
@jerop jerop deleted the no-deps-in-finally branch June 11, 2022 01:43
@jerop jerop restored the no-deps-in-finally branch June 11, 2022 01:43
@jerop jerop deleted the no-deps-in-finally branch June 11, 2022 01:43
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/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. 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.

Invalidate passing Results from a Finally Task to another Finally Task
3 participants