Add sidecars to Tasks #1236

ghost · 2019-08-22T17:56:25Z

Changes

Closes #980

This commit adds user-defined sidecars to the TaskSpec. A sidecar is a
container intended to provide some auxiliary support to a pod's primary
containers. An example usecase is a mock API server for your app to hit
during testing.

This commit doesn't introduce any extra lifecycle management beyond that
added in #936 - sidecars will be stopped without any warning or cleanup
time. At the moment I'm thinking of leaving this as-is until the sidecars
KEP reaches general availability in Kubernetes.

Design doc: https://docs.google.com/document/d/1OiV75sIqwo77QbASiItBVFVsM5gWgxvtpuc4fLcbjBs/edit

Submitter Checklist

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

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

Reviewer Notes

If API changes
are included, additive changes
must be approved by at least two OWNERS
and backwards incompatible changes
must be approved by more than 50% of the OWNERS,
and they must first be added
in a backwards compatible way.

Release Notes

Tasks can now define a list of sidecar containers to run alongside their steps

tekton-robot · 2019-08-22T17:58: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/taskrun/resources/pod.go	88.9%	87.8%	-1.1

tekton-robot · 2019-08-22T19:34:42Z

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/taskrun/resources/pod.go	88.9%	88.4%	-0.5

tekton-robot · 2019-08-22T19:46:40Z

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/taskrun/resources/pod.go	88.9%	88.4%	-0.5

ghost · 2019-08-22T19:48:30Z

pkg/reconciler/taskrun/resources/cloudevent/cloud_event_controller_test.go

@@ -114,7 +114,7 @@ func TestSendCloudEvents(t *testing.T) {
 				SendSuccessfully: true,
 			}
 			err := SendCloudEvents(tc.taskRun, NewFakeClient(&successfulBehaviour), logger)
-			if err == nil {
+			if err != nil {
 				t.Fatalf("Unexpected error sending cloud events: %v", err)


@afrittoli I'm a little confused by this fatal message and the if above it. Is it supposed to detect errors or nils? Reads to me like it should be if err != nil but want to check with you before I merge it like this.

hm this test must start failing if you change it right?

That's right - I fixed a bug in the way errors were being returned related to CloudEvents and this test started failing. Changing this test got it passing after my bug fix was introduced but I wanted to double check what the actual expected behaviour was.

That was a silly double-error - mistake in the code, enforced by the test :S
Thanks for catching / fixing that!

tekton-robot · 2019-08-22T20:43:17Z

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/taskrun/resources/pod.go	88.9%	89.0%	0.2

tekton-robot · 2019-08-22T20:54:03Z

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/taskrun/resources/pod.go	88.9%	89.0%	0.2

ghost · 2019-08-23T15:04:03Z

/test pull-tekton-pipeline-integration-tests

tekton-robot · 2019-08-26T20:11:01Z

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/taskrun/resources/pod.go	88.9%	89.0%	0.2
test/builder/task.go	87.8%	85.6%	-2.3

tekton-robot · 2019-08-26T20:28:37Z

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/taskrun/resources/pod.go	88.9%	89.0%	0.2
test/builder/task.go	87.8%	85.6%	-2.3

tekton-robot · 2019-08-26T21:27:04Z

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/taskrun/resources/pod.go	88.9%	89.0%	0.2
pkg/status/taskrunpod.go	100.0%	97.1%	-2.9
test/builder/task.go	87.8%	85.6%	-2.3

ghost · 2019-08-27T13:01:17Z

/test pull-tekton-pipeline-integration-tests

ghost · 2019-08-27T14:52:41Z

`ಠ_ಠ`

dibyom · 2019-08-27T14:55:48Z

2019-08-27_10-51-44
ಠ_ಠ

Usually due to a YAML test failure!

ghost · 2019-08-27T18:34:35Z

/test pull-tekton-pipeline-integration-tests

ghost · 2019-08-27T18:48:15Z

I've run the failing yaml tests locally and didn't hit any issues.

bobcatfish

Looking great! Just a few minor comments from me

docs/taskruns.md

docs/tasks.md

pkg/reconciler/taskrun/resources/cloudevent/cloud_event_controller.go

bobcatfish · 2019-08-27T19:17:48Z

pkg/reconciler/taskrun/resources/cloudevent/cloud_event_controller_test.go

@@ -114,7 +114,7 @@ func TestSendCloudEvents(t *testing.T) {
 				SendSuccessfully: true,
 			}
 			err := SendCloudEvents(tc.taskRun, NewFakeClient(&successfulBehaviour), logger)
-			if err == nil {
+			if err != nil {
 				t.Fatalf("Unexpected error sending cloud events: %v", err)


hm this test must start failing if you change it right?

pkg/status/taskrunpod.go

test/sidecar_test.go

ghost · 2019-08-28T11:51:51Z

So the YAML tests were failing and are now fixed without intervention. I assume they were timing out because of limited resources or some similar problem 🤞

tekton-robot · 2019-08-28T12:12:00Z

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/taskrun/resources/pod.go	88.9%	89.0%	0.2
test/builder/task.go	87.8%	85.6%	-2.3

tekton-robot · 2019-08-28T13:57: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/taskrun/resources/pod.go	88.9%	89.0%	0.2
test/builder/task.go	87.8%	85.6%	-2.3

ghost · 2019-08-28T17:19:48Z

/test pull-tekton-pipeline-build-tests

docs/tasks.md

pkg/status/taskrunpod_test.go

tekton-robot · 2019-08-28T18:42:01Z

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/taskrun/resources/pod.go	88.9%	89.0%	0.2
test/builder/task.go	87.8%	85.6%	-2.3

pkg/status/taskrunpod_test.go

examples/taskruns/task-sidecar-dind.yaml

This commit adds user-defined sidecars to the TaskSpec. A sidecar is a container intended to provide some auxiliary support to a pod's primary containers. An example usecase is a mock API server for your app to hit during testing. This commit doesn't introduce any extra lifecycle management beyond that added in #936 - sidecars will be stopped without any warning or cleanup time. At the moment I'm thinking of leaving this as-is until the sidecars KEP reaches general availability in Kubernetes.

tekton-robot · 2019-08-28T19:16:08Z

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/taskrun/resources/pod.go	88.9%	89.0%	0.2
test/builder/task.go	87.8%	85.6%	-2.3

dibyom

/lgtm

tekton-robot · 2019-08-29T17:19:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom, sbwsg

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~~ [dibyom,sbwsg]

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

afrittoli

Cool, nice work. It's really handy being able to specify a sidecar like this!
And thank you for catching the bug on the cloud_event code.

afrittoli · 2019-08-29T17:18:55Z

pkg/reconciler/taskrun/resources/pod_test.go

@@ -385,6 +433,7 @@ func TestMakePod(t *testing.T) {
 			}
 		})
 	}
+


afrittoli · 2019-08-29T17:21:31Z

pkg/status/taskrunpod_test.go

@@ -454,3 +454,84 @@ func TestUpdateStatusFromPod(t *testing.T) {
 		})
 	}
 }
+
+func TestCountSidecars(t *testing.T) {


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 Aug 22, 2019

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

tekton-robot requested review from dibyom and imjasonh August 22, 2019 17:56

tekton-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 22, 2019

tekton-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 22, 2019

ghost commented Aug 22, 2019

View reviewed changes

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 22, 2019

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 22, 2019

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

ghost changed the title ~~WIP Add sidecars to Tasks~~ Add sidecars to Tasks Aug 27, 2019

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 Aug 27, 2019

bobcatfish reviewed Aug 27, 2019

View reviewed changes

imjasonh reviewed Aug 28, 2019

View reviewed changes

docs/tasks.md Show resolved Hide resolved

pkg/status/taskrunpod_test.go Outdated Show resolved Hide resolved

imjasonh reviewed Aug 28, 2019

View reviewed changes

pkg/status/taskrunpod_test.go Outdated Show resolved Hide resolved

examples/taskruns/task-sidecar-dind.yaml Outdated Show resolved Hide resolved

dibyom approved these changes Aug 29, 2019

View reviewed changes

tekton-robot assigned dibyom Aug 29, 2019

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

tekton-robot merged commit 14425d1 into tektoncd:master Aug 29, 2019

afrittoli reviewed Aug 29, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sidecars to Tasks #1236

Add sidecars to Tasks #1236

ghost commented Aug 22, 2019 •

edited by ghost

Loading

tekton-robot commented Aug 22, 2019

tekton-robot commented Aug 22, 2019

tekton-robot commented Aug 22, 2019

ghost Aug 22, 2019

bobcatfish Aug 27, 2019

ghost Aug 27, 2019

afrittoli Aug 29, 2019

tekton-robot commented Aug 22, 2019

tekton-robot commented Aug 22, 2019

ghost commented Aug 23, 2019

tekton-robot commented Aug 26, 2019

tekton-robot commented Aug 26, 2019

tekton-robot commented Aug 26, 2019

ghost commented Aug 27, 2019

ghost commented Aug 27, 2019 •

edited by ghost

Loading

dibyom commented Aug 27, 2019

ghost commented Aug 27, 2019

ghost commented Aug 27, 2019

bobcatfish left a comment

bobcatfish Aug 27, 2019

ghost commented Aug 28, 2019

tekton-robot commented Aug 28, 2019

tekton-robot commented Aug 28, 2019

ghost commented Aug 28, 2019

tekton-robot commented Aug 28, 2019

tekton-robot commented Aug 28, 2019

dibyom left a comment

tekton-robot commented Aug 29, 2019

afrittoli left a comment

afrittoli Aug 29, 2019

afrittoli Aug 29, 2019

@@ @@ -385,6 +433,7 @@ func TestMakePod(t *testing.T) { @@
               			}
               		})
               	}

Add sidecars to Tasks #1236

Add sidecars to Tasks #1236

Conversation

ghost commented Aug 22, 2019 • edited by ghost Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

tekton-robot commented Aug 22, 2019

tekton-robot commented Aug 22, 2019

tekton-robot commented Aug 22, 2019

ghost Aug 22, 2019

Choose a reason for hiding this comment

bobcatfish Aug 27, 2019

Choose a reason for hiding this comment

ghost Aug 27, 2019

Choose a reason for hiding this comment

afrittoli Aug 29, 2019

Choose a reason for hiding this comment

tekton-robot commented Aug 22, 2019

tekton-robot commented Aug 22, 2019

ghost commented Aug 23, 2019

tekton-robot commented Aug 26, 2019

tekton-robot commented Aug 26, 2019

tekton-robot commented Aug 26, 2019

ghost commented Aug 27, 2019

ghost commented Aug 27, 2019 • edited by ghost Loading

ಠ_ಠ

ಠ_ಠ

ಠ_ಠ

dibyom commented Aug 27, 2019

ghost commented Aug 27, 2019

ghost commented Aug 27, 2019

bobcatfish left a comment

Choose a reason for hiding this comment

bobcatfish Aug 27, 2019

Choose a reason for hiding this comment

ghost commented Aug 28, 2019

tekton-robot commented Aug 28, 2019

tekton-robot commented Aug 28, 2019

ghost commented Aug 28, 2019

tekton-robot commented Aug 28, 2019

tekton-robot commented Aug 28, 2019

dibyom left a comment

Choose a reason for hiding this comment

tekton-robot commented Aug 29, 2019

afrittoli left a comment

Choose a reason for hiding this comment

afrittoli Aug 29, 2019

Choose a reason for hiding this comment

afrittoli Aug 29, 2019

Choose a reason for hiding this comment

ghost commented Aug 22, 2019 •

edited by ghost

Loading

ghost commented Aug 27, 2019 •

edited by ghost

Loading

`ಠ_ಠ`

`ಠ_ಠ`

`ಠ_ಠ`