Cloud Event pipeline resource #1091

afrittoli · 2019-07-16T18:15:06Z

Changes

Define a new type of PipelineResource for cloud events.
This resource can be used as an output only. The resource is only
defined here, it's not functional yet.

Submitter Checklist

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

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

There's not much to be unit tested in this PR. This is tested by a PR on top that actually implements the logic behind the cloud event pipeline resource.

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task and TaskRun to build and release this image

pkg/apis/pipeline/v1alpha1/cloud_event_resource.go

Implements in the TaskRun controller the logic to provide the cloud event pipeline resource. This commits puts together the API, cloud event helper and resource definition from three pull requests: - tektoncd#1090 - tektoncd#1091 - tektoncd#1092 It adds unit tests for the new code and one E2E YAML test. The YAML test runs a simple http server that can receive the cloudevent for test purposes. The list of cloud events to be sent is added to the TaskRun status and processed by the TaskRun controller once the pod associated to the TaskRun completes its execution. The `isDone` definition of the TaskRun is not altered, the reconciler checks for events to be sent once the TaskRun.isDone is true. Retries are not implemented yet in the sense that every scheduled event will be attempted exactly once, but it may be that those attempts happen across different invocations of Reconcile. Signed-off-by: Andrea Frittoli <andrea.frittoli@uk.ibm.com>

tekton-robot · 2019-07-16T18:55: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/apis/pipeline/v1alpha1/cloud_event_resource.go	Do not exist	0.0%

tekton-robot · 2019-07-16T18:56:09Z

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/apis/pipeline/v1alpha1/cloud_event_resource.go	Do not exist	0.0%

tekton-robot · 2019-07-16T21:52: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/apis/pipeline/v1alpha1/cloud_event_resource.go	Do not exist	0.0%

bobcatfish

Thanks for breaking this up, it is much easier to review! (Apologies for the parts where the context is missing and so I'm asking about it - however I think being able to understand it in the context of just this code via comments will be useful anyway!) ❤️

pkg/apis/pipeline/v1alpha1/cloud_event_resource.go

tekton-robot · 2019-07-17T23:02: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/apis/pipeline/v1alpha1/cloud_event_resource.go	Do not exist	88.2%

pkg/apis/pipeline/v1alpha1/cloud_event_resource.go

bobcatfish

Left a couple thoughts but generally I think it's looking good! A couple typos maybe to fix but otherwise I would /lgtm!

Will approve anyway:
/approve

pkg/apis/pipeline/v1alpha1/cloud_event_resource.go

pkg/apis/pipeline/v1alpha1/cloud_event_resource_test.go

bobcatfish · 2019-07-22T12:18:54Z

pkg/apis/pipeline/v1alpha1/cloud_event_resource.go

+// GetUploadContainerSpec returns nothing as the cloud event is sent by the controller once the POD execution is completed
+func (s *CloudEventResource) GetUploadContainerSpec() ([]corev1.Container, error) {
+	return nil, nil
+}


The fact that we have no upload or download container spec here I think is interesting and I think hints that maybe a PipelineResource isn't the way we want to go with this in the long run - but I think this is fine for now!

Maybe in the future we can either:

Make it so that emitting events during / before / after Pipeline execution is something the controller always does when provided with a sink

Make it so we have a "finally" clause that allows a PipelineResource like this to be invoked and actually have the context of whether the PipelineRun succeeded or not

Yeah, I wanted to do this using a container scheduled to be executed as the last one in the Pod, but it didn't work out, so we might switch this in future to be something like you proposed.

I like both the ideas you proposed, I think what I need to do is to put together a design for a more general concept of notifications

One thing that I wanted to highlight is that the way this will be used in #837 is for TaskRun notifications, as opposed to PipelineRun.
Both TaskRun and PipelineRun end notifications can be useful, but I focused on TaskRun first because it fits well with the use cases I included in the design.

In the demo that I presented at the OSS I have a pipeline that builds a docker image, runs unit testing and runs a code lint, the three in parallel. All three tasks send a notifications once they're done, and the notification triggers different TaskRuns depending on the source (identified via a label on the TaskRun). This makes it possible to publish test results on GitHub using the PullRequestPipelineResource as soon as the corresponding task is complete, with no need to wait for the other Tasks - i.e. I don't necessarily want to wait for the image build to be complete before I can see the unit test results.

tekton-robot · 2019-07-23T23:24:28Z

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/apis/pipeline/v1alpha1/cloud_event_resource.go	Do not exist	88.2%

pkg/apis/pipeline/v1alpha1/cloud_event_resource.go

ghost · 2019-08-06T12:18:03Z

One minor comment otherwise looks good to me

ghost · 2019-08-06T16:21:01Z

We can fix that comment after merge.

/lgtm

ghost · 2019-08-06T16:26:07Z

/retest

afrittoli · 2019-08-07T12:30:20Z

/retest

ghost · 2019-08-07T12:37:46Z

/retest

afrittoli · 2019-08-07T13:29:20Z

Oh, the resource interface changed, I need to update this :P

tekton-robot · 2019-08-07T13:48:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli, bobcatfish

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,bobcatfish]

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

tekton-robot · 2019-08-07T13:50:48Z

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/apis/pipeline/v1alpha1/cloud_event_resource.go	Do not exist	78.9%

ghost · 2019-08-07T13:53:04Z

Looks like just the generated code changed, is that right @afrittoli ?

tekton-robot · 2019-08-07T14:37:24Z

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/apis/pipeline/v1alpha1/cloud_event_resource.go	Do not exist	78.9%

tekton-robot · 2019-08-07T14:44:45Z

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/apis/pipeline/v1alpha1/cloud_event_resource.go	Do not exist	78.9%

afrittoli · 2019-08-07T14:56:47Z

/test pull-tekton-pipeline-build-tests

Define a new type of PipelineResource for cloud events. This resource can be used as an output only. The resource is only defined here, it's not functional yet. Signed-off-by: Andrea Frittoli <andrea.frittoli@uk.ibm.com>

tekton-robot · 2019-08-07T15:10:44Z

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/apis/pipeline/v1alpha1/cloud_event_resource.go	Do not exist	78.9%

ghost · 2019-08-07T15:19:50Z

/lgtm

Implements in the TaskRun controller the logic to provide the cloud event pipeline resource. This commits puts together the API, cloud event helper and resource definition from four pull requests: - tektoncd#1090 - tektoncd#1091 - tektoncd#1092 - TBD TBD: address review comments TBD: test for interaction with the image digest export It adds unit tests for the new code and one E2E YAML test. The YAML test runs a simple http server that can receive the cloudevent for test purposes. The list of cloud events to be sent is added to the TaskRun status and processed by the TaskRun controller once the pod associated to the TaskRun completes its execution. The `isDone` definition of the TaskRun is not altered, the reconciler checks for events to be sent once the TaskRun.isDone is true. Retries are not implemented yet in the sense that every scheduled event will be attempted exactly once, but it may be that those attempts happen across different invocations of Reconcile. Signed-off-by: Andrea Frittoli <andrea.frittoli@uk.ibm.com>

Implements in the TaskRun controller the logic to provide the cloud event pipeline resource. This commits puts together the API, cloud event helper and resource definition from four pull requests: - tektoncd#1090 - tektoncd#1091 - tektoncd#1092 TBD: Test for interaction with the image digest export It adds unit tests for the new code and one E2E YAML test. The YAML test runs a simple http server that can receive the cloudevent for test purposes. The list of cloud events to be sent is added to the TaskRun status and processed by the TaskRun controller once the pod associated to the TaskRun completes its execution. The `isDone` definition of the TaskRun is not altered, the reconciler checks for events to be sent once the TaskRun.isDone is true. Retries are not implemented yet in the sense that every scheduled event will be attempted exactly once, but it may be that those attempts happen across different invocations of Reconcile. Signed-off-by: Andrea Frittoli <andrea.frittoli@uk.ibm.com>

Implements in the TaskRun controller the logic to provide the cloud event pipeline resource. This commits puts together the API, cloud event helper and resource definition from four pull requests: - tektoncd#1090 - tektoncd#1091 - tektoncd#1092 It adds unit tests for the new code and one E2E YAML test. The YAML test runs a simple http server that can receive the cloudevent for test purposes. The list of cloud events to be sent is added to the TaskRun status and processed by the TaskRun controller once the pod associated to the TaskRun completes its execution. The `isDone` definition of the TaskRun is not altered, the reconciler checks for events to be sent once the TaskRun.isDone is true. Retries are not implemented yet in the sense that every scheduled event will be attempted exactly once, but it may be that those attempts happen across different invocations of Reconcile. Signed-off-by: Andrea Frittoli <andrea.frittoli@uk.ibm.com>

Implements in the TaskRun controller the logic to provide the cloud event pipeline resource. This commits puts together the API, cloud event helper and resource definition from four pull requests: - #1090 - #1091 - #1092 It adds unit tests for the new code and one E2E YAML test. The YAML test runs a simple http server that can receive the cloudevent for test purposes. The list of cloud events to be sent is added to the TaskRun status and processed by the TaskRun controller once the pod associated to the TaskRun completes its execution. The `isDone` definition of the TaskRun is not altered, the reconciler checks for events to be sent once the TaskRun.isDone is true. Retries are not implemented yet in the sense that every scheduled event will be attempted exactly once, but it may be that those attempts happen across different invocations of Reconcile. Signed-off-by: Andrea Frittoli <andrea.frittoli@uk.ibm.com>

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

tekton-robot requested review from dlorenc and vdemeester July 16, 2019 18:15

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

dibyom reviewed Jul 16, 2019

View reviewed changes

pkg/apis/pipeline/v1alpha1/cloud_event_resource.go Outdated Show resolved Hide resolved

pkg/apis/pipeline/v1alpha1/cloud_event_resource.go Outdated Show resolved Hide resolved

pkg/apis/pipeline/v1alpha1/cloud_event_resource.go Outdated Show resolved Hide resolved

afrittoli mentioned this pull request Jul 16, 2019

Cloud Event output resource #837

Merged

3 tasks

afrittoli force-pushed the cloud_event_resource branch 2 times, most recently from 32af6d1 to 45329e0 Compare July 16, 2019 18:52

afrittoli force-pushed the cloud_event_resource branch from 45329e0 to f92f210 Compare July 16, 2019 21:49

bobcatfish reviewed Jul 17, 2019

View reviewed changes

afrittoli force-pushed the cloud_event_resource branch from f92f210 to 43648e4 Compare July 17, 2019 23:00

dlorenc reviewed Jul 22, 2019

View reviewed changes

pkg/apis/pipeline/v1alpha1/cloud_event_resource.go Show resolved Hide resolved

bobcatfish reviewed Jul 22, 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 22, 2019

afrittoli force-pushed the cloud_event_resource branch from 43648e4 to bb020ac Compare July 23, 2019 23:21

ghost reviewed Aug 6, 2019

View reviewed changes

pkg/apis/pipeline/v1alpha1/cloud_event_resource.go Outdated Show resolved Hide resolved

tekton-robot assigned ghost Aug 6, 2019

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

afrittoli force-pushed the cloud_event_resource branch from bb020ac to de64a44 Compare August 7, 2019 13:48

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

afrittoli force-pushed the cloud_event_resource branch from de64a44 to 403fbc8 Compare August 7, 2019 14:34

afrittoli force-pushed the cloud_event_resource branch from 403fbc8 to f4d8028 Compare August 7, 2019 14:42

Cloud Event output resource

281b799

Define a new type of PipelineResource for cloud events. This resource can be used as an output only. The resource is only defined here, it's not functional yet. Signed-off-by: Andrea Frittoli <andrea.frittoli@uk.ibm.com>

afrittoli force-pushed the cloud_event_resource branch from f4d8028 to 281b799 Compare August 7, 2019 15:08

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

tekton-robot merged commit 8272358 into tektoncd:master Aug 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cloud Event pipeline resource #1091

Cloud Event pipeline resource #1091

afrittoli commented Jul 16, 2019

tekton-robot commented Jul 16, 2019

tekton-robot commented Jul 16, 2019

tekton-robot commented Jul 16, 2019

bobcatfish left a comment

tekton-robot commented Jul 17, 2019

bobcatfish left a comment

bobcatfish Jul 22, 2019

afrittoli Jul 23, 2019

tekton-robot commented Jul 23, 2019

ghost commented Aug 6, 2019

ghost commented Aug 6, 2019

ghost commented Aug 6, 2019

afrittoli commented Aug 7, 2019

ghost commented Aug 7, 2019

afrittoli commented Aug 7, 2019

tekton-robot commented Aug 7, 2019

tekton-robot commented Aug 7, 2019

ghost commented Aug 7, 2019

tekton-robot commented Aug 7, 2019

tekton-robot commented Aug 7, 2019

afrittoli commented Aug 7, 2019

tekton-robot commented Aug 7, 2019

ghost commented Aug 7, 2019

Cloud Event pipeline resource #1091

Cloud Event pipeline resource #1091

Conversation

afrittoli commented Jul 16, 2019

Changes

Submitter Checklist

tekton-robot commented Jul 16, 2019

tekton-robot commented Jul 16, 2019

tekton-robot commented Jul 16, 2019

bobcatfish left a comment

Choose a reason for hiding this comment

tekton-robot commented Jul 17, 2019

bobcatfish left a comment

Choose a reason for hiding this comment

bobcatfish Jul 22, 2019

Choose a reason for hiding this comment

afrittoli Jul 23, 2019

Choose a reason for hiding this comment

tekton-robot commented Jul 23, 2019

ghost commented Aug 6, 2019

ghost commented Aug 6, 2019

ghost commented Aug 6, 2019

afrittoli commented Aug 7, 2019

ghost commented Aug 7, 2019

afrittoli commented Aug 7, 2019

tekton-robot commented Aug 7, 2019

tekton-robot commented Aug 7, 2019

ghost commented Aug 7, 2019

tekton-robot commented Aug 7, 2019

tekton-robot commented Aug 7, 2019

afrittoli commented Aug 7, 2019

tekton-robot commented Aug 7, 2019

ghost commented Aug 7, 2019