-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
WIP - implementing finally at the pipeline level 🤞🏼💥😱 #2437
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
The following is the coverage report on pkg/.
|
Hey @pritidesai ! Very excited to see this moving along!! :D :D I'm going to update the PR description to include the release notes, design doc and related issue. Is it possible to include the docs on how to use this in the PR? This makes reviewing the PR waaaay easier, otherwise one has to read the code to understand how this is intended to work |
9279df2
to
0795040
Compare
|
0795040
to
326bb89
Compare
06d4d8c
to
d6602c6
Compare
The following is the coverage report on pkg/.
|
d6602c6
to
1f7213d
Compare
The following is the coverage report on pkg/.
|
1f7213d
to
5e2de42
Compare
The following is the coverage report on pkg/.
|
5e2de42
to
7aa21eb
Compare
The following is the coverage report on pkg/.
|
7aa21eb
to
7e9a69e
Compare
The following is the coverage report on the affected files.
|
/test pull-tekton-pipeline-build-tests |
this is ready for review now, thanks 🙏 |
/hold There's still some ongoing discussion in the design doc that I'd like to see resolved before we merge this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few comments 👼
The following is the coverage report on the affected files.
|
We can now specify a list of tasks needs to be executed just before pipeline exits (either after finishing all non-final tasks successfully or after a single failure) Most useful for tasks such as report test results, cleanup cluster resources, etc ``` apiVersion: tekton.dev/v1beta1 kind: Pipeline metadata: name: pipeline-with-final-tasks spec: tasks: - name: pre-work taskRef: Name: some-pre-work - name: unit-test taskRef: Name: run-unit-test runAfter: - pre-work - name: integration-test taskRef: Name: run-integration-test runAfter: - unit-test finally: - name: cleanup-test taskRef: Name: cleanup-cluster - name: report-results taskRef: Name: report-test-results ```
The following is the coverage report on the affected files.
|
thanks @vdemeester I have addressed all of your comments, let me know if any more changes needed 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great! I think we're all almost totally agreed about the feature itself so most of my comments are about the implementation.
Main themes:
- I'm not sure about allowing "from" in finally tasks, feels a bit confusing to me when we don't allow results (e.g. imagine a git resource that is "from" a task that builds - if the build task executes, the data on disc for the resource may be different than if it doesnt)
- I think its worth trying to avoid duplicating the validation logic as much as possible, since it leads to some nasty bugs, for example we ran into that with results (Fix 3 bugs with results #2471) - we're still duplicating some logic there so it might not be completely avoidable, but if we can avoid it its worth the effort
- If we get the structure of PipelineRunState right, i bet we can avoid some of the duplicated code in the reconciler
- Better to add functions in separate packages than add more functions to the reconciler
- Is it possible for the logic that determines what runs next to become aware of finally? If the logic that returns the next tasks to run can handle Finally tasks, i bet we need a lot less special casing for finally
It looks like the coverage generally went down, and we want it to generally go up or stay the same so it might be worth digging into that a bit once we're more sure about the details of the implementation:
| one or more `PipelineTask` skipped and rest successful | all final tasks successful | `true` | `Completed` | | ||
| one or more `PipelineTask` skipped and rest successful | one or more failure of final tasks | `false` | `Failed` | | ||
| single failure of `PipelineTask` | all final tasks successful | `false` | `failed` | | ||
| single failure of `PipelineTask` | one or more failure of final tasks | `false` | `failed` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! 🎉 thanks for making this explicit :D
I was just also thinking about ways to make incremental progress on this - I wonder if it would work to have a PR to add the types and their validation before adding the functionality. |
/kind feature |
I have addressed all the comments from this PR. Please review the two (new) separate PRs:
Closing this PR 👋 |
Changes
We can now specify a list of tasks that needs to be executed just before
pipeline exits (either after finishing all non-final tasks successfully or after
a single failure)
Most common final tasks could be (1) report test results, (2) cleanup cluster resources, etc
Design doc: https://docs.google.com/document/d/1lxpYQHppiWOxsn4arqbwAFDo4T0-LCqpNa6p-TJdHrw/edit#
Part of #1684
Fixes #2446
Depends on: #2504
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Double check this list of stuff that's easy to miss:
cmd
dir, please updatethe release Task to build and release this image.
Reviewer Notes
If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.
Release Notes