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 TektonListener to accept events and trigger pipeline/task runs #783

Closed
wants to merge 12 commits into from

Conversation

iancoffey
Copy link
Member

@iancoffey iancoffey commented Apr 23, 2019

Changes

Proposal: https://docs.google.com/document/d/1tfG6dkARP2xko-5trDV3D62DJZiXQBZgLtLH7GyUopM/edit#

This is a mostly-functioning WIP of the proposal linked above. It adds a new type, PipelineListener and CRD which allow for a workflow to accept events and trigger pipeline runs end-to-end from any source. This implementation is meant to allow the project to accept any event type, with initial support for CloudEvents, and the ability to add event handling as needed for other types/sources.

Related

This concept started off in the build project, there is some good background there as well.

knative/build#577
knative/build#591

TODO

  • Add tests
  • investigate ftest situation
  • end-to-end checksuite event success

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

Adds a new CRD and type `PipelineListener` to allow for PipelineRuns to be triggered automatically from events.

@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: iancoffey
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: imjasonh

If they are not already assigned, you can assign the PR to them by writing /assign @imjasonh in a comment when ready.

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

@googlebot googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Apr 23, 2019
@tekton-robot tekton-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 23, 2019
Copy link
Contributor

@dlorenc dlorenc left a comment

Choose a reason for hiding this comment

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

A few high level comments, thanks for sending this out!

spec:
serviceAccountName: tekton-pipelines-controller
listener-image: github.com/tektoncd/pipeline/cmd/pipelinelistener
event-type: com.github.checksuite
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to take data out of the event payload and thread that through into the pipeline via creation of a new resource, or similar?

Copy link
Contributor

Choose a reason for hiding this comment

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

Here's a strawperson I put together on how binding eventdata into a resource might work: https://gist.github.com/dlorenc/bdd3dc952cd9b9cb10b837224b031214

Any thoughts?

Copy link
Member Author

@iancoffey iancoffey Apr 24, 2019

Choose a reason for hiding this comment

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

I find this very interesting! Admittedly though, I am still processing how the example EventBinding type fits into the puzzle, inre: this PR.

Would that kind be used to spawn a listener behind the scenes based on the binding type? Or am I way off :)

I would love to chat about this more, feel free to ping on tekton slack as well.

role: test-build-pipeline-listener
spec:
serviceAccountName: tekton-pipelines-controller
listener-image: github.com/tektoncd/pipeline/cmd/pipelinelistener
Copy link
Contributor

Choose a reason for hiding this comment

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

What about storing info like the image used in a configmap or a flag somewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

That sounds like a better plan, Ill make that change.

@bobcatfish
Copy link
Collaborator

/ok-to-test

@tekton-robot tekton-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 24, 2019
@iancoffey iancoffey changed the title Add pipelines listener to accept events and trigger pipeline runs Add TektonListener to accept events and trigger pipeline/task runs Apr 25, 2019
@khrm
Copy link
Contributor

khrm commented Apr 26, 2019

@iancoffey

  1. Doesn't this implementation create a stateful set for every repo?

  2. Shouldn't this pr be raised wrt https://github.com/tektoncd/experimental/tree/master/webhooks-extension ?

  3. Great idea of using runSpec to define pipelineRun . I have done an implementation which uses location of pipelinerun.yaml to define it.

@iancoffey
Copy link
Member Author

iancoffey commented Apr 26, 2019

@khrm hello, some answers inline:

Doesn't this implementation create a stateful set for every repo?

It creates a statefulset for every listener. Since it creates a listener for any run spec provided - there could also be multiple Listeners for one repo. Thinking of use cases - there may be different events that need to be handled, or you may build different parts of your monorepo based on specific events. I think the implementation allows for almost any combo of repo/branch/pipeline but to be fair I have only tested 1:1:1.

Shouldn't this pr be raised wrt https://github.com/tektoncd/experimental/tree/master/webhooks-extension ?

Yes, its possible. My goal for next Tuesdays Tekton WG meet is to understand where this should be added to the experimental repo or if this can land right here once feedback is addressed.

Great idea of using runSpec to define pipelineRun . I have done an implementation which uses location of pipelinerun.yaml to define it.

Cool! Thanks. There are many updates coming via the proposal document that will likely change much of this but that part of the spec will likely stay and grow.

@bobcatfish
Copy link
Collaborator

@iancoffey can we close this PR now that we've got https://github.com/tektoncd/experimental/tree/master/tekton-listener ? :D

@iancoffey
Copy link
Member Author

yes and ty!

@iancoffey iancoffey closed this May 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants