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-0063: Workspace Dependencies #413

Merged
merged 1 commit into from
May 10, 2021

Conversation

jerop
Copy link
Member

@jerop jerop commented Apr 23, 2021

In this PR, we discuss the motivation, goals, non-goals and requirements
for Workspace dependencies so that we can align on the problem before
exploring the proposal and alternatives later

Tasks can have either resource dependencies or ordering dependencies
between them. Resource dependencies are based on Results and
Workspaces, while ordering dependencies are defined using runAfter
to sequence Tasks.

Today, users cannot specify resource dependencies based on Workspaces,
that is, a Task should execute and use a given Workspace before
another Task executes and uses the same Workspace. We need to
provide a way for users to specify resource dependencies based on
Workspaces to ensure that failure and skipping strategies for
common CI/CD use cases work and users don't get unexpected Pipeline
failures when we roll out those features.

Failure and skipping strategies:

Issue: tektoncd/pipeline#3109

/cc @souleb @sbwsg @pritidesai @bobcatfish

@tekton-robot tekton-robot requested review from a user, pritidesai and bobcatfish April 23, 2021 15:19
@tekton-robot
Copy link
Contributor

@jerop: GitHub didn't allow me to request PR reviews from the following users: souleb.

Note that only tektoncd members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

In this PR, we discuss the motivation, goals, non-goals and requirements
for Workspace dependencies so that we can align on the problem before
discussing the proposal and alternatives later

Today, users cannot specify resource dependencies based on Workspaces.
We need to provide a way for users to specify resource dependencies
based on Workspaces to ensure that failure and skipping strategies
for common CI/CD use cases work and users don't get unexpected Pipeline
failures when we roll out those features

Failure and skipping strategies:

Issue: tektoncd/pipeline#3109

/cc @souleb @sbwsg @pritidesai @bobcatfish

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 23, 2021
@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 26, 2021
@ghost
Copy link

ghost commented Apr 26, 2021

/assign

@tekton-robot tekton-robot assigned ghost Apr 26, 2021
Copy link
Contributor

@bobcatfish bobcatfish left a comment

Choose a reason for hiding this comment

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

several minor comments from me but overall looks great!!

teps/0063-workspace-dependencies.md Outdated Show resolved Hide resolved
teps/0063-workspace-dependencies.md Outdated Show resolved Hide resolved
teps/0063-workspace-dependencies.md Outdated Show resolved Hide resolved
teps/0063-workspace-dependencies.md Show resolved Hide resolved
@jerop
Copy link
Member Author

jerop commented Apr 26, 2021

@sbwsg @bobcatfish @souleb thank you for the reviews! 😁

it's ready for another review from a non-googler (cc @tektoncd/core-maintainers)

@bobcatfish bobcatfish added the kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). label May 3, 2021
@jerop
Copy link
Member Author

jerop commented May 3, 2021

/kind tep

@bobcatfish
Copy link
Contributor

/assign @pritidesai

@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 7, 2021
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.

Thanks for this @jerop.
I agree that we need a way to express these kind of dependencies - as describe in your example runAfter is not sufficient.
The only concern I have is the overlap with TEP-0030 Workspace Paths, as I that TEP will introduce the same kind of dependency, only path specific.
It may be that this TEP is a first step towards TEP-0030, in which case I'd like to make sure that design decisions in this TEP are mindful of TEP-0030 too.
/approve

@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli, 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

In this PR, we discuss the motivation, goals, non-goals and requirements
for `Workspace` dependencies so that we can align on the problem before
exploring the proposal and alternatives later

`Tasks` can have either resource dependencies or ordering dependencies
between them. Resource dependencies are based on `Results` and
`Workspaces`, while ordering dependencies are defined using `runAfter`
to sequence `Tasks`.

Today, users cannot specify resource dependencies based on `Workspaces`,
that is, a `Task` should execute and use a given `Workspace` before
another `Task` executes and uses the same `Workspace`. We need to
provide a way for users to specify resource dependencies based on
`Workspaces` to ensure that failure and skipping strategies for
common CI/CD use cases work and users don't get unexpected `Pipeline`
failures when we roll out those features.

Failure and skipping strategies:
- [TEP-0059: Skip Guarded Task Only](https://github.com/tektoncd/community/blob/main/teps/0059-skip-guarded-task-only.md)
- [TEP-0050: Ignore Task Failures](https://github.com/tektoncd/community/blob/main/teps/0050-ignore-task-failures.md)

Issue: tektoncd/pipeline#3109
@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 10, 2021
@jerop
Copy link
Member Author

jerop commented May 10, 2021

The only concern I have is the overlap with TEP-0030 Workspace Paths, as I that TEP will introduce the same kind of dependency, only path specific.
It may be that this TEP is a first step towards TEP-0030, in which case I'd like to make sure that design decisions in this TEP are mindful of TEP-0030 too.

thanks for the review @afrittoli 😁

had considered TEP-0030 as a solution to this issue, but there were drawbacks to it -- will discuss in the alternatives

will make sure to also consider / discuss how the TEPs would work together (cc @sbwsg)

@ghost
Copy link

ghost commented May 10, 2021

Updating my approve to an lgtm. Agree that there are considerable drawbacks to Workspace Paths vis-a-vis workspace resource dependency. I'd be happy to spend a bit of time re-drafting parts of Workspace Paths in response to this TEP as well, then bring those modifications up for review here. Thanks a lot @jerop !

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 10, 2021
@tekton-robot tekton-robot merged commit e8f5416 into tektoncd:main May 10, 2021
jerop added a commit to jerop/community that referenced this pull request Jun 3, 2021
In tektoncd#413, we added the problem
statement for [TEP-0063: Workspace Dependencies](https://github.com/tektoncd/community/blob/main/teps/0063-workspace-dependencies.md)
that discusses the need to give users a way to specify resource
dependencies based on `Workspaces`. That will ensure that failure and
skipping strategies for common CI/CD use cases work and users don't get
unexpected failures.

In this change, we add the proposal and discuss the alternatives for
solving that problem. To enable users to specify resource dependencies
based on `Workspaces`, we will provide a field - `useAfter` - that can
be used to specify that a given `Task` should use a specific `Workspace`
after another `Task` has already used it. It will be used to construct
the `Directed Acyclic Graph` that represents the `Pipeline` to enforce
the execution order.
jerop added a commit to jerop/community that referenced this pull request Jun 3, 2021
In tektoncd#413, we added the problem
statement for [TEP-0063: Workspace Dependencies](https://github.com/tektoncd/community/blob/main/teps/0063-workspace-dependencies.md)
that discusses the need to give users a way to specify resource
dependencies based on `Workspaces`. That will ensure that failure and
skipping strategies for common CI/CD use cases work and users don't get
unexpected failures.

In this change, we add the proposal and discuss the alternatives for
solving that problem. To enable users to specify resource dependencies
based on `Workspaces`, we will provide a field - `useAfter` - that can
be used to specify that a given `Task` should use a specific `Workspace`
after another `Task` has already used it. It will be used to construct
the `Directed Acyclic Graph` that represents the `Pipeline` to enforce
the execution order.
jerop added a commit to jerop/community that referenced this pull request Jun 22, 2021
In tektoncd#413, we added the problem
statement for [TEP-0063: Workspace Dependencies](https://github.com/tektoncd/community/blob/main/teps/0063-workspace-dependencies.md)
that discusses the need to give users a way to specify resource
dependencies based on `Workspaces`. That will ensure that failure and
skipping strategies for common CI/CD use cases work and users don't get
unexpected failures.

In this change, we add the proposal and discuss the alternatives for
solving that problem. To enable users to specify resource dependencies
based on `Workspaces`, we will provide a field - `useAfter` - that can
be used to specify that a given `Task` should use a specific `Workspace`
after another `Task` has already used it. It will be used to construct
the `Directed Acyclic Graph` that represents the `Pipeline` to enforce
the execution order.
@jerop jerop deleted the workspace-dependencies branch January 6, 2022 15:08
@jerop jerop restored the workspace-dependencies branch January 6, 2022 15:08
@jerop jerop deleted the workspace-dependencies branch January 29, 2022 19:24
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: Proposed
Development

Successfully merging this pull request may close these issues.

6 participants