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

Add the support of configuring the default timeout of TaskRun #996

Merged
merged 1 commit into from
Jul 3, 2019
Merged

Add the support of configuring the default timeout of TaskRun #996

merged 1 commit into from
Jul 3, 2019

Conversation

houshengbo
Copy link

@houshengbo houshengbo commented Jun 20, 2019

Changes

  • Changes the default timeout from 10 mins to 60 mins
  • Adds a config_defaults configMap for configuring pipeline default values per tektoncd/pipeline instance

Closes #979

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.

Release Notes

The default timeout for TaskRuns and PipelineRuns is increased from 10 minutes to 60 minutes. This timeout will now be configurable.

@tekton-robot tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 20, 2019
@googlebot googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Jun 20, 2019
@tekton-robot tekton-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jun 20, 2019
@bobcatfish
Copy link
Collaborator

I think part of your change might be missing from the commit, looking at 2b10d24 it says " Add a new param default_timeout for TaskRun" but the changes look like they only do the first part "Change the default timeout from 10 mins to 60 mins".

Looks like a step in the right direction anyway!

(I hope you don't mind @houshengbo I updated the "release note" section of the PR description)

@bobcatfish
Copy link
Collaborator

bobcatfish commented Jun 20, 2019

I think part of your change might be missing from the commit,

Or maybe it's b/c this was a "WIP" in progress PR, in which case my apologies for reviewing too early! 🙏

@tekton-robot tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 21, 2019
@houshengbo
Copy link
Author

@bobcatfish Thank you for the updates.
It is still under development.

@tekton-robot tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 21, 2019
@abayer
Copy link
Contributor

abayer commented Jun 21, 2019

@houshengbo Looks like codegen needs to be run. =)

@abayer
Copy link
Contributor

abayer commented Jun 21, 2019

/assign @abayer

@houshengbo
Copy link
Author

houshengbo commented Jun 21, 2019

@abayer @bobcatfish @vdemeester
I need some suggestions from you folks. I created a store to load the configuration of the default timeout as defined in config-defaults.yaml. Where do I need to load the data, by calling NewStore()?

Should I do it in reconciler for pipelinerun and taskrun? Or should I call is somewhere, so that global context will have it, and can be accessed anywhere in need?
Thx.

@abayer
Copy link
Contributor

abayer commented Jun 21, 2019

I'm wondering if this needs to be this elaborate? We use config-artifact-bucket and config-artifact-pvc just via c.CoreV1().ConfigMap(ns).Get(name, metav1.GetOptions{}) - is that something that we don't think will scale well as we keep adding more configuration?

@houshengbo
Copy link
Author

@abayer
I would rather use an existing one, but
do u think config-artifact-pvc or config-artifact-bucket are good candidates to add a global default timeout?

@abayer
Copy link
Contributor

abayer commented Jun 24, 2019

I have no problem with creating a new one, and making it more general, I'm just wondering if we need to be using the approach you're using here to get data from it or if we can use something more like we do with the existing ones.

@tekton-robot tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 24, 2019
@houshengbo houshengbo changed the title WIP: Add the support of configuring the default timeout of TaskRun Add the support of configuring the default timeout of TaskRun Jun 24, 2019
@tekton-robot tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 24, 2019
@dlorenc
Copy link
Contributor

dlorenc commented Jun 25, 2019

/test pull-tekton-pipeline-go-coverage

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

Seems good to me (and it follows knative/* pattern for those things so yay 🎉)
One remark, we need to start using The Tekton Authors instead of The Knative Authors at some point 👼

/cc @bobcatfish

pkg/apis/config/default.go Show resolved Hide resolved
pkg/apis/config/default.go Outdated Show resolved Hide resolved
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 28, 2019

CLA Check
All committers are now authorized under a signed CLA.

@houshengbo
Copy link
Author

/test pull-tekton-pipeline-integration-tests

@houshengbo
Copy link
Author

@dlorenc @imjasonh Could you help me review this PR? Thx.

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

This may conflict a tiny bit with #1015 but it's fine 👼
@houshengbo Can you update the commit message to remove * Add a new param default_timeout for TaskRun and add * Adds a config_defaults configMap for configuring pipeline default values per tektoncd/pipeline instance ?

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 1, 2019
@houshengbo
Copy link
Author

@vdemeester Already did it.

* Changes the default timeout from 10 mins to 60 mins
* Adds a config_defaults configMap for configuring pipeline
  default values per tektoncd/pipeline instance
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 3, 2019
@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: houshengbo, vdemeester

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

@tekton-robot tekton-robot merged commit d494344 into tektoncd:master Jul 3, 2019
@bobcatfish
Copy link
Collaborator

Thanks for making this change @houshengbo ! Would it also make sense to add some docs at https://github.com/tektoncd/pipeline/blob/master/docs/install.md#configuring-tekton-pipelines so users know this is something they can configure?

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. cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The default TaskRun timeout is too short.
7 participants