Migrate the TaskRun controller to genreconciler. #2771

mattmoor · 2020-06-06T15:36:38Z

There are effectively two pieces to this change:

Generate and consume the base reconciler (dropping boilerplate)
Update tests to plumb certain things differently (the existing tests followed our bad example and used unsafe type cases to root around in the controller.Reconciler.(*Reconciler))

The changes in the pipeline run files are mostly for symmetry, or because a common utility changed.

This is WIP until #2760 lands.

tekton-robot · 2020-06-06T15:37:03Z

This PR cannot be merged: expecting exactly one kind/ label

Available kind/ labels are:

kind/bug: Categorizes issue or PR as related to a bug.
kind/flake: Categorizes issue or PR as related to a flakey test
kind/cleanup: Categorizes issue or PR as related to cleaning up code, process, or technical debt.
kind/design: Categorizes issue or PR as related to design.
kind/documentation: Categorizes issue or PR as related to documentation.
kind/feature: Categorizes issue or PR as related to a new feature.
kind/misc: Categorizes issue or PR as a miscellaneuous one.

tekton-robot · 2020-06-06T15:37:11Z

This PR cannot be merged: expecting exactly one kind/ label

Available kind/ labels are:

kind/bug: Categorizes issue or PR as related to a bug.
kind/flake: Categorizes issue or PR as related to a flakey test
kind/cleanup: Categorizes issue or PR as related to cleaning up code, process, or technical debt.
kind/design: Categorizes issue or PR as related to design.
kind/documentation: Categorizes issue or PR as related to documentation.
kind/feature: Categorizes issue or PR as related to a new feature.
kind/misc: Categorizes issue or PR as a miscellaneuous one.

tekton-robot · 2020-06-06T15:37:51Z

This PR cannot be merged: expecting exactly one kind/ label

Available kind/ labels are:

kind/bug: Categorizes issue or PR as related to a bug.
kind/flake: Categorizes issue or PR as related to a flakey test
kind/cleanup: Categorizes issue or PR as related to cleaning up code, process, or technical debt.
kind/design: Categorizes issue or PR as related to design.
kind/documentation: Categorizes issue or PR as related to documentation.
kind/feature: Categorizes issue or PR as related to a new feature.
kind/misc: Categorizes issue or PR as a miscellaneuous one.

mattmoor · 2020-06-06T15:38:21Z

/kind cleanup

tekton-robot · 2020-06-06T15:39:11Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/metrics.go	86.8%	84.7%	-2.0
pkg/reconciler/pipelinerun/pipelinerun.go	80.2%	80.3%	0.1
pkg/reconciler/taskrun/controller.go	96.2%	96.4%	0.3
pkg/reconciler/taskrun/metrics.go	86.7%	85.4%	-1.2
pkg/reconciler/taskrun/taskrun.go	76.0%	74.7%	-1.3
test/controller.go	0.0%	28.9%	28.9

tekton-robot · 2020-06-06T16:30:41Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/metrics.go	86.8%	84.7%	-2.0
pkg/reconciler/pipelinerun/pipelinerun.go	80.2%	80.3%	0.1
pkg/reconciler/taskrun/controller.go	96.2%	96.4%	0.3
pkg/reconciler/taskrun/metrics.go	86.7%	85.4%	-1.2
pkg/reconciler/taskrun/taskrun.go	76.0%	74.7%	-1.3
test/controller.go	0.0%	28.9%	28.9

mattmoor · 2020-06-06T16:34:38Z

oh boy, my favorite. dependency cycles!

tekton-robot · 2020-06-06T16:41:08Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/metrics.go	86.8%	84.7%	-2.0
pkg/reconciler/pipelinerun/pipelinerun.go	80.2%	80.3%	0.1
pkg/reconciler/taskrun/controller.go	96.2%	96.4%	0.3
pkg/reconciler/taskrun/metrics.go	86.7%	85.4%	-1.2
pkg/reconciler/taskrun/taskrun.go	76.0%	74.7%	-1.3
test/controller.go	0.0%	28.9%	28.9

mattmoor · 2020-06-08T16:13:27Z

/assign @afrittoli @vdemeester @imjasonh

Alright, this should be RFAL. There's some cruft to cleanup after this, but this has already started to creep a bit in the cleanup department. 😬

tekton-robot · 2020-06-08T16:13:44Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/taskrun/controller.go	96.6%	96.7%	0.1
pkg/reconciler/taskrun/taskrun.go	75.5%	74.7%	-0.8

vdemeester

/lgtm

vdemeester · 2020-06-08T09:01:05Z

pkg/apis/config/default_test.go

@@ -14,38 +14,39 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */

-package config
+package config_test


pkg/reconciler/pipelinerun/pipelinerun_test.go

tekton-robot · 2020-06-08T17:02:41Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/taskrun/controller.go	96.6%	96.7%	0.1
pkg/reconciler/taskrun/taskrun.go	75.5%	77.0%	1.5

There are effectively two pieces to this change: 1. Generate and consume the base reconciler (dropping boilerplate) 2. Update tests to plumb certain things differently (the existing tests followed our bad example and used unsafe type cases to root around in the `controller.Reconciler.(*Reconciler)`) The changes in the pipeline run files are mostly for symmetry, or because a common utility changed.

tekton-robot · 2020-06-08T19:40:14Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/taskrun/controller.go	96.6%	96.7%	0.1
pkg/reconciler/taskrun/taskrun.go	75.5%	77.0%	1.5

danielhelfand · 2020-06-08T20:24:01Z

pkg/reconciler/taskrun/taskrun.go

@@ -727,8 +662,7 @@ func updateStoppedSidecarStatus(pod *corev1.Pod, tr *v1beta1.TaskRun, c *Reconci
 			})
 		}
 	}
-	_, err := c.updateStatus(tr)


Just making a note here in case this comes into question, but checked this out and seems to be fine without the call to updateStatus. Thanks for noticing and correcting.

afrittoli

Very nice, thank you!
/approve

afrittoli · 2020-06-09T16:55:12Z

pkg/reconciler/taskrun/taskrun_test.go

@@ -1328,7 +1307,7 @@ func TestReconcile(t *testing.T) {
 				t.Fatalf("Expected actions to be logged in the kubeclient, got none")
 			}

-			err = checkEvents(fr, tc.name, tc.wantEvents)
+			err = checkEvents(testAssets.Recorder, tc.name, tc.wantEvents)


Nice!! 🤩

afrittoli · 2020-06-09T17:20:53Z

test/controller.go

+	Pipeline    *fakepipelineclientset.Clientset
+	Resource    *fakeresourceclientset.Clientset
+	Kube        *fakekubeclientset.Clientset
+	CloudEvents cloudeventclient.CEClient


I knew there was a better place to store this client :D only at the time I had no idea :P

vdemeester

/lgtm
/meow

tekton-robot · 2020-06-09T17:26:42Z

@vdemeester:

In response to this:

/lgtm
/meow

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.

tekton-robot · 2020-06-09T17:26:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli, 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~~ [afrittoli,vdemeester]

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

mattmoor · 2020-06-09T18:18:24Z

/retest

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 6, 2020

tekton-robot requested review from afrittoli and imjasonh June 6, 2020 15:36

tekton-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 6, 2020

tekton-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Jun 6, 2020

mattmoor force-pushed the taskrun branch from 3d7c26f to 2c598dc Compare June 6, 2020 16:28

mattmoor force-pushed the taskrun branch from 2c598dc to 8d308fd Compare June 6, 2020 16:38

mattmoor force-pushed the taskrun branch from 8d308fd to db4413f Compare June 8, 2020 16:10

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 8, 2020

mattmoor changed the title ~~[WIP] Migrate the TaskRun controller to genreconciler.~~ Migrate the TaskRun controller to genreconciler. Jun 8, 2020

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 8, 2020

tekton-robot assigned afrittoli, imjasonh and vdemeester Jun 8, 2020

vdemeester reviewed Jun 8, 2020

View reviewed changes

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 8, 2020

mattmoor force-pushed the taskrun branch from db4413f to 9ea3f99 Compare June 8, 2020 17:00

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 8, 2020

mattmoor force-pushed the taskrun branch from 9ea3f99 to a0449b8 Compare June 8, 2020 19:37

mattmoor mentioned this pull request Jun 8, 2020

Various reconciler cleanups #2784

Merged

danielhelfand reviewed Jun 8, 2020

View reviewed changes

afrittoli reviewed Jun 9, 2020

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 9, 2020

vdemeester approved these changes Jun 9, 2020

View reviewed changes

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 9, 2020

tekton-robot merged commit 9e8c654 into tektoncd:master Jun 9, 2020

mattmoor deleted the taskrun branch June 9, 2020 19:42

mattmoor mentioned this pull request Jun 25, 2020

Pick up genreconciler tektoncd/triggers#635

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate the TaskRun controller to genreconciler. #2771

Migrate the TaskRun controller to genreconciler. #2771

mattmoor commented Jun 6, 2020

tekton-robot commented Jun 6, 2020

tekton-robot commented Jun 6, 2020

tekton-robot commented Jun 6, 2020

mattmoor commented Jun 6, 2020

tekton-robot commented Jun 6, 2020

tekton-robot commented Jun 6, 2020

mattmoor commented Jun 6, 2020

tekton-robot commented Jun 6, 2020

mattmoor commented Jun 8, 2020

tekton-robot commented Jun 8, 2020

vdemeester left a comment

vdemeester Jun 8, 2020

tekton-robot commented Jun 8, 2020

tekton-robot commented Jun 8, 2020

danielhelfand Jun 8, 2020

afrittoli left a comment

afrittoli Jun 9, 2020

afrittoli Jun 9, 2020

vdemeester left a comment

tekton-robot commented Jun 9, 2020

tekton-robot commented Jun 9, 2020

mattmoor commented Jun 9, 2020

Migrate the TaskRun controller to genreconciler. #2771

Migrate the TaskRun controller to genreconciler. #2771

Conversation

mattmoor commented Jun 6, 2020

tekton-robot commented Jun 6, 2020

tekton-robot commented Jun 6, 2020

tekton-robot commented Jun 6, 2020

mattmoor commented Jun 6, 2020

tekton-robot commented Jun 6, 2020

tekton-robot commented Jun 6, 2020

mattmoor commented Jun 6, 2020

tekton-robot commented Jun 6, 2020

mattmoor commented Jun 8, 2020

tekton-robot commented Jun 8, 2020

vdemeester left a comment

Choose a reason for hiding this comment

vdemeester Jun 8, 2020

Choose a reason for hiding this comment

tekton-robot commented Jun 8, 2020

tekton-robot commented Jun 8, 2020

danielhelfand Jun 8, 2020

Choose a reason for hiding this comment

afrittoli left a comment

Choose a reason for hiding this comment

afrittoli Jun 9, 2020

Choose a reason for hiding this comment

afrittoli Jun 9, 2020

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Jun 9, 2020

tekton-robot commented Jun 9, 2020

mattmoor commented Jun 9, 2020