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.

Sign up for GitHub

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

Jump to bottom

TEP-0007: Conditions Beta Skipping - When Scope #258

Closed
wants to merge 1 commit into from
Closed

TEP-0007: Conditions Beta Skipping - When Scope #258

wants to merge 1 commit into from

Conversation

jerop
Copy link
Member

@jerop jerop commented Nov 12, 2020 •
edited
Loading

Adding scope and expressions field to when allow bundling of the
specifications of when expressions and their skipping options

scope defaults to Branch and be set to Node in guarded tasks with
ordering-dependent tasks only and to allow the execution of the branch

other alternatives considered in doc

/cc @bobcatfish @pritidesai @sbwsg

@tekton-robot tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 12, 2020
afrittoli
afrittoli reviewed Nov 13, 2020
View reviewed changes
Copy link
Member

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

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

Thank you for the updates!
This is a really an important feature of Tekton, thanks for driving this forward.

I wonder if this should be a new TEP instead? @vdemeester @bobcatfish
I don't have a strong feeling about this, if amending the TEP is the preferred approach I'm ok with that.

I'm getting a bit confused about all the guarding options and how they relate to one each other. I think it would be really useful to draw a few diagrams for the different types of conditions / behaviours in case of condition not met, and associate the proposed syntax for each of them.

teps/0007-conditions-beta.md Outdated Show resolved Hide resolved
teps/0007-conditions-beta.md Outdated Show resolved Hide resolved
@jerop jerop mentioned this pull request Nov 20, 2020
Closed
@afrittoli
Copy link
Member

/test pull-community-teps-lint

@vdemeester vdemeester added the kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). label Nov 23, 2020
@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign houshengbo
You can assign the PR to them by writing /assign @houshengbo in a comment when ready.

The full list of commands accepted by this bot can be found 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 size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 8, 2020
@jerop
Copy link
Member Author

jerop commented Dec 8, 2020 •
edited
Loading

thank you for the review @afrittoli 🙏

i described the components and their behavior in further detail, and added a diagram to demonstrate different scenarios

i prefer having it in one TEP so that all the proposals, alternatives and other related info are in one place...there would also be a lot of repetition if we split it up

@jerop jerop force-pushed the skip-alt branch 2 times, most recently from 5aaa065 to 446a548 Compare December 8, 2020 03:22
@jerop
Conditions Beta: When Scope
d2a299c 
adding `scope` and `expressions` field to allow bundling of the
specifications of when expressions and their skipping options.

scope can take either `Node` and `Branch`, where `scope` can be set to
`Node` in guarded tasks with ordering-dependent tasks only and to allow
the execution of the branch
@jerop
Copy link
Member Author

jerop commented Dec 8, 2020 •
edited
Loading

@tektoncd/core-maintainers this tep is ready for review, specifically looking for feedback and suggestions on the naming and syntax

We propose the naming scope:Node/Branch:

  • scope to represent the scope of the WhenExpressions
  • Node to represent the Nodes of the Pipeline DAG, which can be Tasks or Runs (Custom Tasks)
  • Branch to represent the a Node and its dependent Nodes

We considered Task/Branch because users are more familiar with Tasks, but chose Node/Branch instead because:

  • Nodes are not only Tasks but also Runs
  • Pipelines are Directed Acyclic Graphs
  • If users have to get familiar with Branch, it makes sense to get familiar with Nodes as well

Pipeline as a DAG with Nodes and Edges has already been briefly introduced to users in the execution order section of the pipeline docs but we could describe it further in the overview section so that the concepts don't suddenly come up in the WhenExpressions section

ghost
ghost reviewed Dec 8, 2020
View reviewed changes
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.

I have one open question about the best way to implement the different syntaxes in a single field but otherwise this looks good to me!

teps/0007-conditions-beta.md Show resolved Hide resolved
@ghost
Copy link

ghost commented Dec 8, 2020

/lgtm

@tekton-robot tekton-robot assigned ghost Dec 8, 2020
@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 8, 2020
pritidesai
pritidesai reviewed Dec 9, 2020
View reviewed changes
teps/0007-conditions-beta.md
- can take the values `Branch` or `Node` only; there will be a validation error if anything else is passed to `scope`
- can take the value `Node` in guarded `Nodes` with ordering dependencies only; there will be a validation error if `scope` is set to `Node` in `Nodes` with resource dependencies

The latter syntax allows us to make it explicit that the skipping policy, as specified by `WhenScope`, is related to the `WhenExpressions`. We can support both the former and latter syntaxes initially then give users 9 months to migrate to the new syntax, per [the beta api policy](https:/tektoncd/pipeline/blob/master/api_compatibility_policy.md).
Copy link
Member

Choose a reason for hiding this comment

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

Are we saying users who migrated to when expressions with 0.16 (old syntax) will not be benefited with this feature? To make use of this feature, users will have to migrate to this new syntax 🤔 I feel we had promised continueAfterSkip with the old syntax and now proposing continueAfterSkip only possible with new syntax? wdyt?

pritidesai
pritidesai reviewed Dec 10, 2020
View reviewed changes
teps/0007-conditions-beta.md
values: ['bar']
```

The `scope` field is optional, defaults to `Branch` and can be set to `Branch` to specify that the `Node`'s `WhenExpressions` guard the execution of the `Branch` (the `Node` and its dependent `Nodes`). However, when `Node` has ordering dependencies only, users can set the `scope` field to `Node` to specify that the `Node`'s `WhenExpressions` guard the execution of the `Node` only, thus the all dependent nodes (which are ordering dependent tasks only) can be executed.
Copy link
Member

Choose a reason for hiding this comment

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

I understand Node as self, when expressions is limited to that particular pipelineTask/node where it's defined. And Branch includes all dependencies i.e. all dependencies are impacted (skipped) by the when expressions of a parent task.

@bobcatfish
Copy link
Contributor

Update from API WG: @jerop is currently exploring other alternatives, will update

Base automatically changed from master to main February 3, 2021 16:34
@bobcatfish
Copy link
Contributor

hey @jerop when you get back to this PR, is it possible to update TEP-0007 with some links to relevant issues in the pipelines repo? i was trying to track them down and it'd be very handy to be able to find them here

@jerop jerop mentioned this pull request Mar 3, 2021
Merged
@vdemeester vdemeester mentioned this pull request Mar 3, 2021
Merged
@jerop jerop closed this Mar 29, 2021
@pritidesai pritidesai mentioned this pull request Apr 26, 2021
Merged
@jerop jerop deleted the skip-alt branch January 6, 2022 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pritidesai pritidesai pritidesai left review comments

@afrittoli afrittoli afrittoli left review comments

@bobcatfish bobcatfish Awaiting requested review from bobcatfish

Assignees
No one assigned
Labels
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
TEP Workflow
Status: Implemented
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jerop @afrittoli @tekton-robot @bobcatfish @pritidesai @vdemeester