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

[WIP] Add continuous integration models (Projects and Builds) #1612

Closed
wants to merge 1 commit into from

Conversation

eddycharly
Copy link
Member

@eddycharly eddycharly commented Jul 10, 2020

Changes

This PR adds continuous integration models to the dashboard.
It does so by adding two new CRDs Projects and Builds.
Those two new resources help bootstrapping the triggers and pipelines resources to setup continuous integration with pipeline definitions stored in the target repository.

A Project will setup:

  • an EventListener to receive github webhooks
  • a TriggerBinding to bind properties from the incoming webhook
  • a TriggerTemplate to create a TaskRun that will clone the repository read the pipeline definition file and instantiate a Build
  • an Ingress pointing the event listener service

A Build will setup:

  • a PipelineRun implementing the pipeline definition that came from the repo

All those resources are correctly meshed together through owner references, they are reconciled and have statuses that aggregate the statuses from the resources they are responsible for.

I will make a demo on next WG meeting, i opened the PR if someone wants to give a look or try it out before Monday.

/cc @AlanGreene @a-roberts

A few screenshots:

Projects list
image

Builds list
image

Create project form
image

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.

@tekton-robot tekton-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 10, 2020
@eddycharly eddycharly force-pushed the ci branch 6 times, most recently from c43d6c2 to b7a5a2f Compare July 12, 2020 20:20
@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 13, 2020
@tekton-robot
Copy link
Contributor

@eddycharly: PR needs rebase.

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.

@afrittoli
Copy link
Member

This feels like something that would be a good candidate for a TEP @vdemeester @bobcatfish @wlynch

@wlynch
Copy link
Member

wlynch commented Jul 15, 2020

This feels like something that would be a good candidate for a TEP @vdemeester @bobcatfish @wlynch

Agreed! This sounds similar to our ideas around generators. See https://github.com/tektoncd/experimental/tree/master/generators for some work we're doing in this space.

@a-roberts
Copy link
Member

Big +1 on this being a TEP and thanks for sharing @wlynch! As I said on the working group call today, it seems it's a sought-after feature in some way, shape, or form, and having the abstraction makes a late of sense.

@eddycharly what do you think, perhaps we'd extend the dashboard to show the abstractions that do became part of the TEP, or your projects/builds become yours?

Full disclosure, I used to work on a Kubernetes offering called Microclimate and we used the exact same terminology (we actually had CRDs), and that worked really well... unfortunately we didn't open-source that, but if you would like to know more please let me know and I'm sure we can make something awesome for the community to benefit from.

@afrittoli
Copy link
Member

One vague concern I have about this is that Tekton has been this far rather non-opinionated about how you setup you CI/CD pipelines. I'm a bit wary of taking a direction away from that.
On the plus side, giving our users a way kick-start a CI is probably a good idea :)

@a-roberts
Copy link
Member

a-roberts commented Jul 15, 2020

One vague concern I have about this is that Tekton has been this far rather non-opinionated about how you setup you CI/CD pipelines. I'm a bit wary of taking a direction away from that.
On the plus side, giving our users a way kick-start a CI is probably a good idea :)

Good point, that's why the webhooks extension was an experimental thing really and was quite opinionated and limited in the type of pipelines you run - I think that may be the biggest thinking point actually...

@eddycharly
Copy link
Member Author

Thanks for your feedback.

I’ve eared very little about the generators project but if I understand correctly it’s supposed to simplify writing pipelines. If so it would be complementary and allow to write CI spec in a simplified syntax (something like travis ?).

Regarding the TEP suggestion, why not, it doesn’t have to be bound to the dashboard. In the end it’s just two new resource types and controllers.

It was easy to start in the dashboard repo but it can be extracted very easily.

What would be nice would be to be able to create Builds directly using a trigger template instead of going through a task run just for that. Any plan to support arbitrary resources in trigger templates ?

@AlanGreene
Copy link
Member

I like the idea of keeping the core dashboard non-opinionated. It was originally envisioned as a Tekton analog of the Kubernetes dashboard, presenting a view of the live state of Tekton resources in the cluster. Developing a UI for a full CI/CD solution has a different set of design considerations as it needs to solve some very different problems.

The current prototype should work well as a dashboard extension.

A topic we've discussed a few times is making extensions more powerful, for example allowing them not only to add new pages to the dashboard, but also to allow them to integrate into / customise core pages. This wouldn't be needed for the current prototype but could open up some interesting possibilities.

@AlanGreene
Copy link
Member

Any plan to support arbitrary resources in trigger templates ?

This has been raised a few times, see for example tektoncd/triggers#482 and some of the other issues linked to it.

@bobcatfish
Copy link
Contributor

One vague concern I have about this is that Tekton has been this far rather non-opinionated about how you setup you CI/CD pipelines. I'm a bit wary of taking a direction away from that.
On the plus side, giving our users a way kick-start a CI is probably a good idea :)

My dream would be to have a solution that captures both: or at least if the abstractions are more opinionated, at least have them be optional.

I think getting a proposal that we're all happy with will probably take a while and there will be a lot to consider but I'd love to see the discussion get started!! If dashboard wants to go ahead with something in the meantime I could see that working too, as long as it's something that could be changed later if we create tekton-wide abstractions.

@a-roberts @eddycharly do you think someone working on this feature would be able to give us an overview in the API working group one day soon?

@wlynch
Copy link
Member

wlynch commented Jul 15, 2020

I’ve eared very little about the generators project but if I understand correctly it’s supposed to simplify writing pipelines. If so it would be complementary and allow to write CI spec in a simplified syntax (something like travis ?).

Not only pipelines, but all related resources (e.g. Triggers). The reason why we wanted to do this is for the same motivation as your reason for the Project CRD - to help users bootstrap integration specific resources for CI.

What would be nice would be to be able to create Builds directly using a trigger template instead of going through a task run just for that. Any plan to support arbitrary resources in trigger templates ?

There's no technical limitation for creating other resources as a part of trigger templates. My understanding is that we restricted it because we were worried about the potential for abuse for creating arbitrary k8s resources. You can allow new clientsets using: https://github.com/tektoncd/triggers/tree/master/pkg/client/dynamic/clientset (see https://github.com/tektoncd/triggers/blob/d51f0f43612b394e2327bdabeeee020457b62f2d/cmd/eventlistenersink/main.go#L61 for how this is used).

Another option is reconsidering the GVK restriction altogether and just trust EventListener service account RBAC roles to be a sufficient perimeter. /cc @gabemontero

a TriggerTemplate to create a TaskRun that will clone the repository read the pipeline definition file and instantiate a Build

This sounds like config as code! This is a problem we want to solve for Tasks/Pipelines in general.
See tektoncd/pipeline#2298, tektoncd/triggers#189

A Build will setup: a PipelineRun implementing the pipeline definition that came from the repo

Instead of defining a new wrapper type, my general preference would be to stick with PipelineRuns and solve the underlying root problem of config as code pipelines (since we know this is something we want to support for Pipeline/TaskRuns in general).

This may benefit from Pipeline in Pipelines via Custom Tasks - https://github.com/tektoncd/community/blob/master/teps/0002-custom-tasks.md

One vague concern I have about this is that Tekton has been this far rather non-opinionated about how you setup you CI/CD pipelines. I'm a bit wary of taking a direction away from that.

+1. One thing we wanted to be careful about in Generators was to not make things GitHub specific (or even within GitHub, how can I configure OAuth vs GitHub Apps?)

@eddycharly
Copy link
Member Author

@bobcatfish i will try to join on next monday. no guarantee though as i'll be on vacation.

@eddycharly
Copy link
Member Author

This sounds like config as code! This is a problem we want to solve for Tasks/Pipelines in general.

Exactly, in-repo config, that was one of the most important point to address for me. Build strategy can be different from one branch to another and it should be the devs responsibility.

+1. One thing we wanted to be careful about in Generators was to not make things GitHub specific (or even within GitHub, how can I configure OAuth vs GitHub Apps?)

Same here, a Project only represents the tunnel to stimulate a build from an event coming from the outside world. A Build connects a resource generated from the event with a pipeline that know how to process this event (the event can come from github, gitlab, curl, anything...). Right now the dashboard only supports GitHub in the UI but you can create whatever you want in yaml.

Instead of defining a new wrapper type, my general preference would be to stick with PipelineRuns and solve the underlying root problem of config as code pipelines (since we know this is something we want to support for Pipeline/TaskRuns in general).

I tend to disagree on this one, having a wrapping type allows to add features like decorating pull requests, notifications, etc... easily. Those things do not really need to work with raw pipelineruns. It also allows easy aggregations/filtering (like filtering builds for a particular repo/branch).

One vague concern I have about this is that Tekton has been this far rather non-opinionated about how you setup you CI/CD pipelines. I'm a bit wary of taking a direction away from that.
On the plus side, giving our users a way kick-start a CI is probably a good idea :)

I completely agree and understand this position. IMO it's not a vague concern but an essential point though.
Bottom line, this is a product decision. I see good reasons why Tekton has to stay non-opinionated but hopefully someone will build a product from it.

@wlynch
Copy link
Member

wlynch commented Jul 15, 2020

It also allows easy aggregations/filtering (like filtering builds for a particular repo/branch).

You might be interested in tektoncd/community#147! I recently proposed this to define how we can better annotate this type of data in Task/PipelineRuns.

@eddycharly
Copy link
Member Author

eddycharly commented Jul 15, 2020

Interesting ! It looks like Task/PipelineRuns are getting responsible of a lot of things though.

I like the idea of a more high level resource used to federate smaller resources that address a single responsibility.

@bobcatfish
Copy link
Contributor

@eddycharly no hurry at all! vacations are important, plz don't feel any pressure to join, happy to wait till you're back :)

@eddycharly
Copy link
Member Author

@bobcatfish no pressure at all, I’ll join if I can don’t worry ;)

@gabemontero
Copy link

I’ve eared very little about the generators project but if I understand correctly it’s supposed to simplify writing pipelines. If so it would be complementary and allow to write CI spec in a simplified syntax (something like travis ?).

Not only pipelines, but all related resources (e.g. Triggers). The reason why we wanted to do this is for the same motivation as your reason for the Project CRD - to help users bootstrap integration specific resources for CI.

What would be nice would be to be able to create Builds directly using a trigger template instead of going through a task run just for that. Any plan to support arbitrary resources in trigger templates ?

There's no technical limitation for creating other resources as a part of trigger templates. My understanding is that we restricted it because we were worried about the potential for abuse for creating arbitrary k8s resources. You can allow new clientsets using: https://github.com/tektoncd/triggers/tree/master/pkg/client/dynamic/clientset (see https://github.com/tektoncd/triggers/blob/d51f0f43612b394e2327bdabeeee020457b62f2d/cmd/eventlistenersink/main.go#L61 for how this is used).

Another option is reconsidering the GVK restriction altogether and just trust EventListener service account RBAC roles to be a sufficient perimeter. /cc @gabemontero

Apologies for the delay (did not get to this before stepping away from work a little over a week ago).

If we were to fall back / depend on service account RBAC, in addition to the EventListener's, I would also suggest the current Trigger SA association, as well as whatever falls out from the Multi Tenant EventListener and new Trigger CRD should also be considered as the authenticated party in object creation. Generally speaking, providing options wrt granularity (i.e. different controllers or different api server clients) on who can create whatever objects would be preferable.

a TriggerTemplate to create a TaskRun that will clone the repository read the pipeline definition file and instantiate a Build

This sounds like config as code! This is a problem we want to solve for Tasks/Pipelines in general.
See tektoncd/pipeline#2298, tektoncd/triggers#189

A Build will setup: a PipelineRun implementing the pipeline definition that came from the repo

Instead of defining a new wrapper type, my general preference would be to stick with PipelineRuns and solve the underlying root problem of config as code pipelines (since we know this is something we want to support for Pipeline/TaskRuns in general).

This may benefit from Pipeline in Pipelines via Custom Tasks - https://github.com/tektoncd/community/blob/master/teps/0002-custom-tasks.md

One vague concern I have about this is that Tekton has been this far rather non-opinionated about how you setup you CI/CD pipelines. I'm a bit wary of taking a direction away from that.

+1. One thing we wanted to be careful about in Generators was to not make things GitHub specific (or even within GitHub, how can I configure OAuth vs GitHub Apps?)

@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign eddycharly
You can assign the PR to them by writing /assign @eddycharly in a comment when ready.

The full list of commands accepted by this bot can be found 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

@tekton-robot
Copy link
Contributor

@eddycharly: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-tekton-dashboard-unit-tests b7a5a2f link /test tekton-dashboard-unit-tests
pull-tekton-dashboard-build-tests ac6cc61 link /test pull-tekton-dashboard-build-tests
pull-tekton-dashboard-integration-0-13-2-tests ac6cc61 link /test pull-tekton-dashboard-integration-0-13-2-tests

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@AlanGreene
Copy link
Member

Closing as discussed on recent Dashboard WG calls since this feature doesn't make sense as part of the core Dashboard application.

This is still very useful and could be implemented as an extension or combined with the generators experimental project or other related efforts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. 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.

8 participants