-
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
Include More Details on Pod Deletion for Cancelled/Timed Out TaskRuns #3053
Include More Details on Pod Deletion for Cancelled/Timed Out TaskRuns #3053
Conversation
/kind documentation |
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.
thanks for updating this @danielhelfand ! a couple of nitpicks from me
/approve
docs/taskruns.md
Outdated
If you set the timeout to 0, the `TaskRun` fails immediately upon encountering an error. | ||
You can use the `timeout` field to set the `TaskRun's` desired timeout value. If you do not specify this | ||
value for the `TaskRun`, the global default timeout value applies. If you set the timeout to 0, the `TaskRun` will | ||
have no timeout and will fail immediately upon encountering an error. |
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.
i'm nitpicking wording that isn't yours but "the TaskRun
will have no timeout and will fail immediately upon encountering an error" confused me a bit b/c "fail immediately upon encountering an error" is true regardless of whether there is a timeout or not
i think what this might want to say is something like "the TaskRun
will have no timeout and will run until it completes either successfully or with an error"?
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.
Thanks! I changed to the following:
If you set the global timeout to 0, all `TaskRuns` that do not have a timeout set
will have no timeout and will run until it completes successfully or fails from an error.
docs/taskruns.md
Outdated
[`config/config-defaults.yaml`](./../config/config-defaults.yaml). | ||
[`config/config-defaults.yaml`](./../config/config-defaults.yaml). If you set the global timeout to 0, | ||
all `TaskRuns` that do not have a timeout set will have no timeout and will fail immediately upon | ||
encountering an error. |
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.
same comment about the error
docs/taskruns.md
Outdated
|
||
The `timeout` value is a `duration` conforming to Go's | ||
[`ParseDuration`](https://golang.org/pkg/time/#ParseDuration) format. For example, valid | ||
values are `1h30m`, `1h`, `1m`, and `60s`. If you set the global timeout to 0, all `TaskRuns` | ||
that do not have an individual timeout set will fail immediately upon encountering an error. | ||
values are `1h30m`, `1h`, `1m`, and `60s`. |
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.
is 0 not a valid value here? maybe worth mentioning that explicitly
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.
Yes, it is valid. I have added it in.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bobcatfish 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 |
1d6ab02
to
e802a54
Compare
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.
Looks great, thanks again @danielhelfand !!
/lgtm
Part of #3051
This pull request updates documentation on pod deletion for both timed out/cancelled TaskRuns to mention that TaskRun pod deletion is part of the process. It also provides a brief amount of information on why this takes place.
In addition to the updates above, there are some minor changes also made to reorganize the content.
Submitter Checklist
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