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

fix(tekton): Allow setting pipeline timeout and set default on PipelineRun to 240 hours #4251

Merged
merged 2 commits into from
Jun 13, 2019
Merged

fix(tekton): Allow setting pipeline timeout and set default on PipelineRun to 240 hours #4251

merged 2 commits into from
Jun 13, 2019

Conversation

abayer
Copy link
Contributor

@abayer abayer commented Jun 12, 2019 •
edited
Loading

Submitter checklist

  • Change is code complete and matches issue description.
  • Change is covered by existing or new tests.

Description

Starting in Tekton Pipelines v0.4.0, the default timeout for
TaskRuns of 10 minutes when no timeout is specified is actually
enforced. The only way to specify a timeout for a TaskRun is by
setting a timeout on the PipelineRun, so until
tektoncd/pipeline#978 and/or
tektoncd/pipeline#979 are addressed, we
should just set a ridiculously high timeout on all PipelineRuns.

But also, let's allow configuring top-level timeouts in the first place. The syntax has existed for ages already.

Special notes for the reviewer(s)

/assign @hferentschik
/assign @dwnusbaum
/assign @wbrefvem
/assign @carlossg

Which issue this PR fixes

fixes #4250

@carlossg
Copy link

/lgtm

@abayer
Copy link
Contributor Author

abayer commented Jun 12, 2019

/test bdd

@abayer
Copy link
Contributor Author

abayer commented Jun 12, 2019

/test lint

@abayer
Copy link
Contributor Author

abayer commented Jun 12, 2019

/test lint
/test bdd

@wbrefvem
Copy link

/lgtm

houndci-bot
houndci-bot reviewed Jun 12, 2019
View reviewed changes
pkg/tekton/syntax/pipeline.go
@@ -95,7 +95,7 @@ type Timeout struct {
Unit TimeoutUnit `json:"unit,omitempty"`
}

func (t Timeout) toDuration() (*metav1.Duration, error) {
func (t *Timeout) ToDuration() (*metav1.Duration, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

exported method Timeout.ToDuration should have comment or be unexported

@codecov
Copy link

codecov bot commented Jun 12, 2019

Codecov Report

Merging #4251 into master will decrease coverage by 1.42%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4251      +/-   ##
==========================================
- Coverage   43.19%   41.77%   -1.43%     
==========================================
  Files         798      798              
  Lines      100096   100097       +1     
==========================================
- Hits        43241    41820    -1421     
- Misses      53255    54981    +1726     
+ Partials     3600     3296     -304
Flag Coverage Δ
#e2e 6.62% <0%> (-24.61%) ⬇️
#integration 41.52% <100%> (-0.01%) ⬇️
Impacted Files Coverage Δ
pkg/tekton/pipelines.go 32.88% <100%> (+0.29%) ⬆️
pkg/jenkins/health.go 0% <0%> (-66.67%) ⬇️
pkg/jx/cmd/step_bdd.go 8.84% <0%> (-46.69%) ⬇️
pkg/kube/secrets.go 27.27% <0%> (-43.19%) ⬇️
pkg/kube/ingress.go 0% <0%> (-39.59%) ⬇️
pkg/jx/cmd/create_jenkins_token.go 7.26% <0%> (-38.41%) ⬇️
pkg/jx/cmd/delete_team.go 12.14% <0%> (-37.39%) ⬇️
pkg/kube/teams.go 23.52% <0%> (-37.26%) ⬇️
...ntset/versioned/typed/jenkins.io/v1/environment.go 32.89% <0%> (-34.22%) ⬇️
pkg/kube/owners.go 0% <0%> (-33.34%) ⬇️
... and 49 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4eaaee...5ce3c83. Read the comment docs.

@codecov
Copy link

codecov bot commented Jun 12, 2019 •
edited
Loading

Codecov Report

Merging #4251 into master will increase coverage by 24.64%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #4251       +/-   ##
===========================================
+ Coverage    6.61%   31.25%   +24.64%     
===========================================
  Files         680      680               
  Lines       90021    90029        +8     
===========================================
+ Hits         5951    28141    +22190     
+ Misses      83655    60510    -23145     
- Partials      415     1378      +963
Flag Coverage Δ
#e2e 31.25% <0%> (+24.64%) ⬆️
#integration 100% <ø> (?)
Impacted Files Coverage Δ
pkg/cmd/step/create/step_create_task.go 5% <0%> (-0.03%) ⬇️
pkg/tekton/pipelines.go 0% <0%> (ø) ⬆️
pkg/tekton/syntax/pipeline.go 0% <0%> (ø) ⬆️
pkg/cmd/opts/buildpacks.go 1.73% <0%> (+1.73%) ⬆️
pkg/packages/packages.go 2.7% <0%> (+2.7%) ⬆️
pkg/gits/provider.go 3.46% <0%> (+3.46%) ⬆️
pkg/cmd/opts/install.go 4.06% <0%> (+4.06%) ⬆️
pkg/log/log.go 44.89% <0%> (+5.1%) ⬆️
pkg/util/suggestions.go 5.55% <0%> (+5.55%) ⬆️
pkg/kube/vault/vault.go 6.16% <0%> (+6.16%) ⬆️
... and 90 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65962ff...5759395. Read the comment docs.

@abayer abayer force-pushed the set-default-pipelinerun-timeout branch from 5ce3c83 to bf1b325 Compare June 12, 2019 18:34
@abayer
Copy link
Contributor Author

abayer commented Jun 12, 2019

/test bdd
/test integration
/test lint

@abayer
Copy link
Contributor Author

abayer commented Jun 12, 2019

/test bdd

@abayer
Copy link
Contributor Author

abayer commented Jun 12, 2019

/test integration

@abayer abayer changed the title fix(tekton): Set a default PipelineRun timeout of 10 days fix(tekton): Allow setting pipeline timeout and set default on PipelineRun to 240 hours Jun 12, 2019
@abayer
Copy link
Contributor Author

abayer commented Jun 12, 2019

/test lint

@abayer
Copy link
Contributor Author

abayer commented Jun 12, 2019

/test bdd

@abayer
Copy link
Contributor Author

abayer commented Jun 12, 2019

/test integration

@hferentschik
Copy link

@abayer due for a rebase now ;-)

hferentschik
hferentschik reviewed Jun 12, 2019
View reviewed changes
pkg/jx/cmd/step/create/test_data/step_create_task/override_block_step/pipelinerun.yml Outdated
status: {}
Copy link

Choose a reason for hiding this comment

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

👍 thanks for the newline :-)

@abayer
fix(tekton): Set a default PipelineRun timeout of 10 days
5347338 
Starting in Tekton Pipelines v0.4.0, the default timeout for
`TaskRun`s of 10 minutes when no timeout is specified is actually
enforced. The only way to specify a timeout for a `TaskRun` is by
setting a timeout on the `PipelineRun`, so until
tektoncd/pipeline#978 and/or
tektoncd/pipeline#979 are addressed, we
should just set a ridiculously high timeout on all `PipelineRun`s.

Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
@abayer abayer force-pushed the set-default-pipelinerun-timeout branch from bf1b325 to 5c6d090 Compare June 12, 2019 20:40
@abayer
Copy link
Contributor Author

abayer commented Jun 12, 2019

aaaaand rebased. =)

@abayer
fix(tekton): Allow configuring Pipeline timeout
5759395 
Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
@abayer abayer force-pushed the set-default-pipelinerun-timeout branch from 5c6d090 to 5759395 Compare June 12, 2019 21:12
@jenkins-x-bot
Copy link
Contributor

jenkins-x-bot commented Jun 12, 2019 •
edited
Loading

@abayer: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
lint bf1b325 link /test lint

View all Builds for this Pull Request

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. I understand the commands that are listed here.

@jenkins-x-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: carlossg, hferentschik, wbrefvem

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:
  • OWNERS [hferentschik,wbrefvem]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jenkins-x-bot jenkins-x-bot merged commit 791131f into jenkins-x:master Jun 13, 2019
daveconde pushed a commit to daveconde/jx that referenced this pull request Apr 7, 2020
@abayer
fix(tekton): Allow setting pipeline timeout and set default on Pipeli…
7a80085 
…neRun to 240 hours (jenkins-x#4251)

* fix(tekton): Set a default PipelineRun timeout of 10 days

Starting in Tekton Pipelines v0.4.0, the default timeout for
`TaskRun`s of 10 minutes when no timeout is specified is actually
enforced. The only way to specify a timeout for a `TaskRun` is by
setting a timeout on the `PipelineRun`, so until
tektoncd/pipeline#978 and/or
tektoncd/pipeline#979 are addressed, we
should just set a ridiculously high timeout on all `PipelineRun`s.

Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

* fix(tekton): Allow configuring Pipeline timeout

Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@houndci-bot houndci-bot houndci-bot left review comments

@hferentschik hferentschik hferentschik approved these changes

Assignees

@carlossg carlossg

@hferentschik hferentschik

@dwnusbaum dwnusbaum

@wbrefvem wbrefvem

Labels
approved lgtm size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Tekton pipelines timeout in 10 minutes
7 participants
@abayer @carlossg @wbrefvem @hferentschik @jenkins-x-bot @houndci-bot @dwnusbaum