Refactor some taskrun fields into helper functions #1110

dibyom · 2019-07-22T20:09:51Z

Changes

This commit refactors the labels, annotations, default timeouts, and
service account fields from taskruns into helper functions. This will
allow us to easily reuse some common defaults for regular taskruns as
well as those created for conditionals in #1093

This replaces some of the TestReconcile_ funcs that were testing
only the functionality in these new helper functions with their own
dedicated unit tests

Adds unit tests for getTaskRunTimeout, which sets the timeout
values for taskrunscreated by pipelineruns. While adding the tests, I
noticedthat if pr.Spec.Timeout is nil, we were setting the default
timeout to 60 seconds and not minutes. This is really an edge
case since we only hit this block if the webhook does not set
the defaults properly.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Commit messages follow commit message best practices

tekton-robot · 2019-07-22T20:12:06Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go	Do not exist	80.0%

pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go

dlorenc · 2019-07-22T20:58:54Z

pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go

@@ -497,6 +450,66 @@ func clearStatus(tr *v1alpha1.TaskRun) {
 	tr.Status.PodName = ""
 }

+func getTaskrunAnnotations(pr *v1alpha1.PipelineRun) map[string]string {


It looks like these are still covered by the mega-tests in taskrun_test/pipelinerun_test, but do you think it might be worthwhile to simplify those tests and add coverage here instead?

Yeah, will add some!

tekton-robot · 2019-07-22T21:36:12Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go	Do not exist	80.0%

vdemeester

😻

bobcatfish · 2019-07-23T19:47:02Z

THANK YOU!!!! 😻

tekton-robot · 2019-07-23T21:07:50Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go	Do not exist	80.5%

tekton-robot · 2019-07-23T22:07:32Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go	Do not exist	80.5%

vdemeester

I am not sure we should remove the TestReconcile* tests though, as without them, we do not cover that it actually does that (no matter the implementation/split into functions).

tekton-robot · 2019-07-24T14:47:26Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go	Do not exist	80.5%

This commit refactors the labels, annotations, default timeouts, and service account fields from taskruns into helper functions. This will allow us to easily reuse some common defaults for regular taskruns as well as those created for conditionals in tektoncd#1093 Signed-off-by: Dibyo Mukherjee <dibyo@google.com>

dibyom · 2019-07-24T14:50:03Z

Ok, added those back in. Kept the additional tests for timeouts, since that logic is a bit more complex than the other helpers

tekton-robot · 2019-07-24T14:51:36Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go	Do not exist	80.5%

getTaskRunTimeout sets the timeout values for taskruns created by pipelineruns. While adding the tests, I noticed that if `pr.Spec.Timeout` is nil, we were setting the default timeout to 60 seconds and not minutes. This is really an edge case since we only hit this block if the webhook does not set the defaults properly. Signed-off-by: Dibyo Mukherjee <dibyo@google.com>

tekton-robot · 2019-07-24T15:07:23Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go	Do not exist	80.5%

vdemeester

/lgtm

tekton-robot · 2019-07-24T15:15:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom, 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:

~~OWNERS~~ [vdemeester]

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

dibyom · 2019-07-24T16:00:17Z

Looks like we have some flaky unit tests again 🤔
I0724 15:24:43.087] ERROR: test gopath-test=SucceededFalse but should be succeededtrue

/test pull-tekton-pipeline-integration-tests

dibyom · 2019-07-24T17:27:42Z

/test pull-tekton-pipeline-integration-tests

tekton-robot requested review from bobcatfish and imjasonh July 22, 2019 20:09

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Jul 22, 2019

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 22, 2019

dlorenc reviewed Jul 22, 2019

View reviewed changes

dibyom force-pushed the taskrun_refactor branch from 17fddd9 to 1e403a7 Compare July 22, 2019 21:34

vdemeester approved these changes Jul 23, 2019

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 23, 2019

dibyom force-pushed the taskrun_refactor branch from 1e403a7 to 3407133 Compare July 23, 2019 21:03

dibyom force-pushed the taskrun_refactor branch from 3407133 to 060fa93 Compare July 23, 2019 22:05

vdemeester reviewed Jul 24, 2019

View reviewed changes

dibyom force-pushed the taskrun_refactor branch from 060fa93 to e9fa865 Compare July 24, 2019 14:45

dibyom force-pushed the taskrun_refactor branch from e9fa865 to 994e036 Compare July 24, 2019 14:48

dibyom force-pushed the taskrun_refactor branch from 994e036 to 4bc47d3 Compare July 24, 2019 15:05

tekton-robot assigned vdemeester Jul 24, 2019

vdemeester approved these changes Jul 24, 2019

View reviewed changes

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 24, 2019

tekton-robot merged commit 3409f09 into tektoncd:master Jul 24, 2019

dibyom deleted the taskrun_refactor branch July 24, 2019 18:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor some taskrun fields into helper functions #1110

Refactor some taskrun fields into helper functions #1110

dibyom commented Jul 22, 2019 •

edited

Loading

tekton-robot commented Jul 22, 2019

dlorenc Jul 22, 2019

dibyom Jul 22, 2019

dibyom Jul 23, 2019

tekton-robot commented Jul 22, 2019

vdemeester left a comment

bobcatfish commented Jul 23, 2019

tekton-robot commented Jul 23, 2019

tekton-robot commented Jul 23, 2019

vdemeester left a comment

tekton-robot commented Jul 24, 2019

dibyom commented Jul 24, 2019

tekton-robot commented Jul 24, 2019

tekton-robot commented Jul 24, 2019

vdemeester left a comment

tekton-robot commented Jul 24, 2019

dibyom commented Jul 24, 2019

dibyom commented Jul 24, 2019

Refactor some taskrun fields into helper functions #1110

Refactor some taskrun fields into helper functions #1110

Conversation

dibyom commented Jul 22, 2019 • edited Loading

Changes

Submitter Checklist

tekton-robot commented Jul 22, 2019

dlorenc Jul 22, 2019

Choose a reason for hiding this comment

dibyom Jul 22, 2019

Choose a reason for hiding this comment

dibyom Jul 23, 2019

Choose a reason for hiding this comment

tekton-robot commented Jul 22, 2019

vdemeester left a comment

Choose a reason for hiding this comment

bobcatfish commented Jul 23, 2019

tekton-robot commented Jul 23, 2019

tekton-robot commented Jul 23, 2019

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Jul 24, 2019

dibyom commented Jul 24, 2019

tekton-robot commented Jul 24, 2019

tekton-robot commented Jul 24, 2019

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Jul 24, 2019

dibyom commented Jul 24, 2019

dibyom commented Jul 24, 2019

dibyom commented Jul 22, 2019 •

edited

Loading