Proposal to move TriggerBinding-TriggerTemplate connection out of TriggerBinding definition about triggers HOT 17 CLOSED

yeah I like that proposal above from @ncskier better than the current project design. TriggerBindings are actually reusable to some degree because they aren't tied to specific TriggerTemplates.

from triggers.

I agree with proceeding to implement @ncskier 's suggestion

from triggers.

Very late to the party but in general I think these are some pretty cool improvements! My comments kinda go hand in hand with #62 - when I look at this proposed design:

apiVersion: tekton.dev/v1alpha1
kind: TriggerBinding
metadata:
  name: my-triggerbinding
spec:
  inputParams:
  - name: secret
    description: The git webhook secret
  - name: branch
    description: The git branch
    default: master
  headerMatches:
  - X-GitHub-Event: push
  - X-GitHub-Secret: $(inputParams.secret)
  conditionals:
  - $(event.ref): refs/heads/$(inputParams.branch)
  outputParams:
  - name: gitrevision
    value: $(event.head_commit.id)
  - name: gitrepositoryurl
    value: $(event.repository.url)

I'm wondering a couple of things:

1. Does it make sense to have these responsibilities all in the same CRD?
2. What is the bare minimum set of functionality that we want to get working in our first iteration?

@ncskier was saying:

1. The EventListener only knows about the addressable endpoint information (deployment + service configurations).
2. The TriggerBinding only knows about the event information, and how it maps to parameters.
3. The TriggerTemplate only knows about what resources to create, and how to inject its parameters into these resources.

I like this division of responsibility but for (2), TriggerBinding's responsibilities, I wonder if we are ready to introduce conditions into the design and if this is the right place to do it - I think we can get a first working iteration of triggers without including conditionals? (Correct me if I'm wrong tho - I'm basically assuming we want to target a use case like GitHub PR triggering for our first iteration, and that we don't need conditionals or filtering of any kind to make that happen)

(And plus if we want to add Conditionals I'd like to explore making use of Pipeline Conditions if we can! But I'm hoping we can punt on this until we need to :D)

from triggers.

The way I understand it, the TriggerBinding is simply pulling all of the specified parameters from the payload and storing them. This is then passed to the TriggerTemplate to be utilized. In this way, TriggerBindings are very much tied to TriggerTemplates, but it is the variance in configuration (Conditions, headerMatch, etc.) that warrants these two to be distinct (encouraged reuse). Some TriggerBindings may have a superset of parameters that are functionally compatible with certain/other TriggerTemplates, but it would likely be better for a user to create an entirely different TriggerBinding that only pulls what is necessary. Although I think it would be better, this is not technically necessary (unused params would be ignored). In this way, I suppose the thinking is that TriggerBindings are not reusable.

However much to your point/previous discussions, considering the use case of the same (using the current design) TriggerBinding across multiple repos that have (the to-be implemented) headerMatch field againsts authentication headers, this copy-paste situation is actualized. The one flaw I see with this design is that if header matching and condititionals are pulled into the EventListenerSpec, this creates a leaky abstraction of the TriggerBinding because now it is the responsibility of the EventListener to know about particular event bindings. If only headers are pulled into the EventListener and Conditions were to stay inside the TriggerBindings, I think this does potentially encourage reuse.

To further this thought, I see a potential problem. Likely, there would have also been a Condition on the TriggerBinding where the "same binding" would have been made distinct as a result, which breaks the reuse.

I see merit in this design, but I think this needs to be adjusted slightly, but I'm not sure how. I wonder if someone else has another use case?

Also just a nit, a TriggerBinding can reference however many TriggerTemplates in the current design (whether good or bad) whereas this sample yaml appears to be one-to-one.

from triggers.

a TriggerBinding can reference however many TriggerTemplates in the current design (whether good or bad) whereas this sample yaml appears to be one-to-one.

Oh right, I was staring at the YAML in the docs and forgot that the spec is a slice of TemplateBindings. Thanks for pointing that out

The merit of this proposal depends on the complexity of user payloads. If we are intending to target simple projects where all you might need to extract is something like the commit ID and repository URL, then my suggestion might not make sense. But with large complex payloads, it might be worth decoupling the field-capturing logic from a particular set of TriggerTemplate connections.

TriggerBinding does 3 things right now:

1. Enables you to capture fields within an event payload and store them as parameters

2. Connects that particular event and accompanying parameters to specific TriggerTemplates

3. Allow for Conditions and headerMatch to be specified for the event payload

Point 1 can be reused, but 2 and possibly 3 are project-specific.

I just wonder if users will want to be able to separate the field-separating logic... It's easy to overlook this right now because we're envisioning such simple payloads.

from triggers.

I completely agree that the first point is an area that could be reused.

Assuming there were multiple different TriggerBindings for different GitHub repositories on a single EventListener, they would all be triggered (assuming the same kind of event) since the headers match. As a result, I envision Conditions to be necessary components to TriggerBindings to prevent all bindings from firing (match on org/repo). This gets back to the leaky abstraction point (EventListeners are now concerned with event payloads) if we were to push Conditions from TriggerBindings to the EventListener.

from triggers.

I wonder if we could list reasons why TriggerTemplates are intended to be more reusable vs TriggerBindings?

Not saying I think one way or another, but it seems kind of arbitrary to me that TriggerTemplates are the ones that are reusable, and we should probably justify it to ourselves while we're at such an early stage in the project. Theoretically Conditions and headerMatch could move to TriggerTemplate and then TriggerBinding would be the one that is reusable, right?

If only one can be reusable (and I'm not saying this should be the case), then is it more valuable to users that they can reuse payload parameter extraction logic or that they can reuse the logic that turns those parameters into other resources? Our current design seems to imply that the second one is more important.

from triggers.

A compromise solution:

apiVersion: tekton.dev/v1alpha1
kind: EventListener
metadata:
  name: simple-listener
spec:
 handler:
    - event:
        type: com.github.push
        source: https://github.com/tektoncd/triggers
      triggerBinding: simple-pipeline-binding
      triggerTemplate: simple-pipeline-template
    - event:
        type: com.github.issues_events
        source: https://github.com/tektoncd/triggers
      triggerBinding: another-pipeline-binding
      triggerTemplate: another-pipeline-template

Then TriggerBinding only take care of event parameter extraction logic.

from triggers.

A compromise solution:

apiVersion: tekton.dev/v1alpha1
kind: EventListener
metadata:
  name: simple-listener
spec:
 handler:
    - event:
        type: com.github.push
        source: https://github.com/tektoncd/triggers
      triggerBinding: simple-pipeline-binding
      triggerTemplate: simple-pipeline-template
    - event:
        type: com.github.issues_events
        source: https://github.com/tektoncd/triggers
      triggerBinding: another-pipeline-binding
      triggerTemplate: another-pipeline-template

Then TriggerBinding only take care of event parameter extraction logic.

This seems analogous to moving only the headers into the EventListener (event will go away). I think this is fine if we wanted to do this, but since TriggerBindings are connected to particular kinds of events, it seems to less explicit as to what kind of message type is being parsed inside of the TriggerBinding. As I mentioned before, this will only probably only help reuse where we have some distinct non event-type related headers (like authentication) where this presumed reusable TriggerBinding would also not have a Condition.

from triggers.

Sorry I'm jumping into this conversation a little late; thanks for writing up this proposal Eli! 👍
I think it's a really good idea to be thinking about how to reuse TriggerBindings.

Conditional/filtering information would also go within the connections field. This way, the payload-variable logic is truly encapsulated and reusable within triggerBinding. I see EventListener analogous to a PipelineRun or TaskRun, in that it provides project-specific configuration details like connections, filtering, or perhaps default parameter/payload values, while triggerBinding and triggerTemplate can be reused as needed.

I think it makes more sense to put the filtering/conditional information in the TriggerBinding (possibly conditional info can go in the TriggerTemplate though?). This is because I like the clear division of information in our current design. The EventListener only knows about the addressable endpoint information (deployment + service configurations). The TriggerBinding only knows about the event information, and how it maps to parameters. The TriggerTemplate only knows about what resources to create, and how to inject its parameters into these resources. So, I personally don't like the idea of moving event information into the EventListener.

I wonder if we could list reasons why TriggerTemplates are intended to be more reusable vs TriggerBindings?

I think that the TriggerTemplates are definitely more reusable than the TriggerBindings, because the TriggerTemplates are more general-purpose and less specific than TriggerBindings. TriggerTemplates define a set of resources that we want to create & a set of parameters needed to create these resources. This set of resources can be reused in many different contexts given different sets of parameters. For example sending parameters for building & testing one of my GitHub projects, versus building & testing a different GitHub project, versus a GitLab project, etc. the TriggerTemplate is reused to create the same set of resources in each different context. On the other hand, TriggerBindings are much more specific. With the current proposal, TriggerBindings are tied to an event type (for example, a GitHub push event), and they are tied to a specific TriggerTemplate (this connection is more loosely defined, because any TriggerTemplate with the same parameters can be used). So the TriggerBinding cannot be reused as easily as the TriggerTemplate.

Furthermore, the TriggerBindings are currently more lightweight than the TriggerTemplates, because they are only mapping parameters from an event payload into the TriggerTemplate parameters. So, it is not very costly to create slightly repetitive TriggerBindings. This isn't to say that the TriggerBindings shouldn't be reusable. However, I don't think the case is as compelling to make them reusable.

I do think that how we integrate header filtering & conditional logic could play into the reusability of TriggerBindings. If both header filtering & conditional logic lives in the TriggerBinding, they will become less lightweight, and it might become more compelling for a user to want to reuse TriggerBindings.

from triggers.

I think this proposal in getting at the heart of something important and I would like to see more reuse as outlined. I have reservation because of the leaky abstraction nature of this, where as currently everything is neatly encapsulated. However, it's a good idea.

from triggers.

Yeah, thinking about it more, I like the idea of reuse when thinking about a catalog. I don't think that we can decouple the TriggerBinding from the TriggerTemplate. However, I think that it is compelling to have an entry in a catalog with a suite of reusable TriggerBindings & TriggerTemplate(s).

For example, consider having a Pipeline that builds and deploys a Go project. In the example catalog entry, I can have a

• Pipeline
• TriggerTemplate (creates the PipelineRun, git PipelineResource, and image PipelineResource)
• TriggerBinding for GitHub push events
• TriggerBinding for GitLab push events
• TriggerBinding for BitBucket push events
• TriggerBinding for "manual" trigger (we haven't talked about this much, but maybe the user can manually trigger the TriggerTemplate)

This catalog scenario makes it very easy for a user with a GitHub project, GitLab project, or BitBucket project to create a webhook to run this Pipeline on their project.

To me, this is the most compelling case for reusing TriggerBindings.

from triggers.

the TriggerBindings are currently more lightweight than the TriggerTemplates, because they are only mapping parameters from an event payload into the TriggerTemplate parameters.

To be clear they're also specifying a target TriggerTemplate, doing header filtering, and allowing for conditionals.

I also don't fully understand why @vincent-pli 's solution or any potential solution that moves parts of TriggerBinding that aren't parameter extraction logic into EventListener creates a leaky abstraction to the point that it isn't worth doing. From the end user's perspective, who doesn't know/care about the implementation details, it's arguably better to have that information listed in EventListener so that TriggerBinding logic can be reused (potentially from a catalog). It's not like EventListener had much in the YAML anyway besides a list of TriggerBindings. Considering the overall complexity of TriggerBinding and TriggerTemplate user files this doesn't seem too unreasonable to add to EventListener.

Additionally, I think that we can have an internal state representation that maintains that abstraction you are talking about. Analogous to pipelines, imagine that fields like TriggerTemplate in TriggerBindings are essentially ParamSpecs, waiting for a value, and that value is passed in through the values listed in EventListener (akin to a pipelinerun supplying params). This way the eventlistener pod doesn't have to actually be responsible for those things, but the TriggerBinding YAML itself is still reusable.

from triggers.

If we're going to make TriggerBindings reusable, then I think it would be beneficial to put conditionals (and maybe also header matching) in the TriggerBindings, and introduce input parameters for the TriggerBindings. To do this, TriggerBindings would look something like the following:

apiVersion: tekton.dev/v1alpha1
kind: TriggerBinding
metadata:
  name: my-triggerbinding
spec:
  inputParams:
  - name: secret
    description: The git webhook secret
  - name: branch
    description: The git branch
    default: master
  headerMatches:
  - X-GitHub-Event: push
  - X-GitHub-Secret: $(inputParams.secret)
  conditionals:
  - $(event.ref): refs/heads/$(inputParams.branch)
  outputParams:
  - name: gitrevision
    value: $(event.head_commit.id)
  - name: gitrepositoryurl
    value: $(event.repository.url)

So the EventListener would look something like this:

apiVersion: tekton.dev/v1alpha1
kind: EventListener
metadata:
  name: my-eventlistener
spec:
  bindings:
    triggerBindingRef: my-triggerbinding
      params:
      - name: secret
        value: asdfasdfasdf
      - name: branch
        value: master
    triggerTemplateRef: my-triggertemplate

In this example, we would only create the resources in the TriggerTemplate when we receive a GitHub push event from the master branch with the secret asdfasdfasdf. I think that this gives us maximum reusability of TriggerBindings, and it keeps the event specific information only in the TriggerBinding and out of the EventListener.

from triggers.

I question how much benefit there is in not including references to the TriggerTemplate in the TriggerBinding. I suppose two templates might have slight variances in naming/labels/etc. where it could potentially see more reuse so seems good I suppose.

This is a semi-blocker for a few things, so if we are in agreement, can proceed with implementing @ncskier's suggestion? @EliZucker @iancoffey @bobcatfish

If so, we can close #56 and it will be under this issue as well?

from triggers.

/assign

from triggers.

Could leverage Knative eventing for the event filter, the diagram is a recent demo:

But I prefer to tekton's own Condition, I like @ncskier 's proposal, event it's a little multi-responsibility : D.
For the Condition implementation, I still think Pipeline Condition is heavy, we already have discussion here: #49

from triggers.