Add validation for PR infinite timeouts #4539
Conversation
|
Related: #4460 |
|
The following is the coverage report on the affected files.
|
|
nice!! good call, thanks @lbernick /approve |
There was a problem hiding this comment.
thanks for adding this validation @lbernick!
This would imply that
timeouts.tasksortimeouts.finallyis longer thantimeouts.pipeline, and it's not clear how Tekton should handle this.
it seems that the current behavior is that timeouts.pipeline will apply, right? it may be useful for future reference to document this existing behavior in the commit/pr
agree that users setting no/infinite timeouts for tasks while the pipeline has a timeout should be invalid - and if they want the tasks to run for as long as needed up to the pipeline timeout, they can make it explicit in the spec
and timeouts is in alpha (#4460) so this is a good time to fix this
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bobcatfish, jerop 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 |
I wasn't sure what the existing behavior is so I tried it out with |
Prior to this commit, it was possible to create a PipelineRun with `timeouts.tasks` or `timeouts.finally` set to 0 (no timeout) without setting the `timeouts.pipeline` to 0. If `timeouts.pipeline` is not set, it defaults to the global timeout value, which is not 0. This would imply that `timeouts.tasks` or `timeouts.finally` is longer than `timeouts.pipeline`, and it's not clear how Tekton should handle this. Prior to this commit, Tekton in practice handles this situation by timing out the PipelineRun when `timeouts.pipeline` is reached, but never timing out the TaskRun, which is clearly inconsistent behavior. This commit results in an error returned to the user if they define a PipelineRun spec as described above, and documents this behavior.
33eaaad to
215fa0f
Compare
|
The following is the coverage report on the affected files.
|
|
/lgtm |
|
/test pull-tekton-pipeline-integration-tests |
Changes
Prior to this commit, it was possible to create a PipelineRun with
timeouts.tasksortimeouts.finallyset to 0 (no timeout) without setting the
timeouts.pipelineto 0. Iftimeouts.pipelineis not set,it defaults to the global timeout value, which is not 0. This would imply that
timeouts.tasksortimeouts.finallyis longer than
timeouts.pipeline, and it's not clear how Tekton should handle this.This commit results in an error returned to the user if they define a PipelineRun spec as described above,
and documents this behavior.
/kind bug
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
Release Notes