MakeTriggerBinding more reusable about triggers HOT 11 CLOSED

Could we resolve this by make EventListener.Triggers.TriggerBinding as a array, like this:

kind: EventListener
metadata:
  name: my-eventlistener
spec:
  triggers:
  - binding:
      - name: static-parameters
      - name: event-parameters
    template:
      name: my-triggertemplate

This could keep EventListener simple.

from triggers.

I can see why this could be attractive in some way, but I believe it complicates things more than clarifies. Likely, we should create two different explicit types within the EventListener trigger, one for static and one for dynamic values. Otherwise, it would appear you could have as many TriggerBindings as you would like. In this scenario, I would still prefer passing the variables in from the EventListener since it also confuses TriggerBindings where some are reusable and some are not.

from triggers.

Users can provide static values to the TriggerTemplate by putting them to the params in the TriggerBinding instead of the attribute values in the event.

I don't know if this solves #87 since it is just shifting the location of the default value, where values could have been statically defined the TriggerTemplate.

Adding the capability to pass the param values from the EventListener to the TriggerBinding.

Since EventListener's aren't resuable, I think it make sense to delegate as much here as possible.

from triggers.

@vtereso It is a different requirement. The param values come from different places for these issues.

from triggers.

Yes, currently a user is able to supply parameters for a TriggerTemplate in the TriggerBinding. I agree with Aki's assessment that the TriggerBinding is currently not very reusable (which was the goal of #55).

I agree with Aki that a good solution could be to have the TriggerBinding take input parameters and pass these in from the EventListener. I propose the following code refactor:

Current design

kind: EventListener
metadata:
  name: my-eventlistener
spec:
  triggers:
  - binding:
      name: my-triggerbinding
    template:
      name: my-triggertemplate

kind: TriggerBinding
metadata:
  name: my-triggerbinding
spec:
  params:
    - name: dockerRegistry
      value: ncskier

Proposed design

kind: EventListener
metadata:
  name: my-eventlistener
spec:
  triggers:
  - binding:
      name: my-triggerbinding
      params:
        - name: dockerRegistry
          value: ncskier
    template:
      name: my-triggertemplate

kind: TriggerBinding
metadata:
  name: my-triggerbinding
spec:
  inputParams:
    - name: dockerRegistry
      description: "example description"
      default: exampledefault
  outputParams:
    - name: dockerRegistry
      value: $(params.dockerRegistry)

Note: this would mean that the dynamic params proposed in #87 could be specified in the EventListener, instead of the TriggerBinding.

from triggers.

/assign

from triggers.

@vincent-pli If I understand correctly, you're proposing that we merge two TriggerBinding resources where one has static parameters, and the other has the event-parameters. While I like your intent of keeping the EventListener simple, I think this complicates the implementation and also the TriggerBinding type. Your proposal makes the TriggerBinding play a dual-role of (1) mapping event information to the TriggerTemplate, and (2) passing static variables to the TriggerTemplate. Additionally, this adds logic for merging multiple sets of parameters from TriggerBindings into one TriggerTemplate which complicates readability/usability for someone using Triggers.

If we wanted to pass parameters into the EventListener, I would prefer to pass them as a ConfigMap, or as a new type. Something like this:

kind: EventListener
metadata:
  name: my-eventlistener
spec:
  triggers:
  - binding:
      name: static-parameters
      params: 
        name: event-parameters
    template:
      name: my-triggertemplate

However, I'm not sure if we really get much from this over my initial proposal. The EventListener resource isn't designed to be reusable, so I don't really understand why we would want to pass parameters in as a separate resource (let me know if I'm missing something though)

from triggers.

@ncskier Thanks for your comments.
I agree with you the proposal will complicates the implementation.
but can get one benefit , for example:
User could put all static information in one single TriggerBinding, the information such as:

• dockerRegistry
• docker token
• git registry
• git token
• kube config path

and use it in every EventBinding, no need do the copy-paste work.

from triggers.

Thanks @vincent-pli, I agree that it might be helpful to reuse static input values. My issue is with using the TriggerBinding to specify these inputs, because it gives the TriggerBinding a dual-role. I think it would be more clear to create a new type or use a ConfigMap to store these parameters instead of using the TriggerBinding.

However, to keep things simple for now, I would like to try to implement a first pass of this without creating a new type or using ConfigMaps. Then, after using Triggers, if we find that we really want reusable parameters, then we can extend the parameters with a new type or ConfigMaps.

from triggers.

Would it ever be desirable to inline and altogether do without the separate binding?
e.g.

kind: EventListener
metadata:
  name: my-eventlistener
spec:
  triggers:
  - binding:
      params:
        - name: dockerRegistry
          value: ncskier
    template:
      name: my-triggertemplate

A common way I would want to use this is to add additional params that I can use in the template that are not necessarily defined in the binding.

e.g.

kind: EventListener
metadata:
  name: my-eventlistener
spec:
  triggers:
  - binding:
      name: my-triggerbinding
      params:
        - name: another-param
          value: cats
    template:
      name: my-triggertemplate
kind: TriggerBinding
metadata:
  name: my-triggerbinding
spec:
  params:
    - name: dockerRegistry
      value: ncskier

resulting in both 'dockerRegistryandanother-param` having values when a template is processed.

Or maybe that should be treated as an array similar to what @vincent-pli suggested earlier
e.g.

  - binding:
      - name: my-triggerbinding
      - params:
         - name: another-param
            value: cats

from triggers.

@skaegi

Would it ever be desirable to inline and altogether do without the separate binding?

The only case I can think of where you wouldn't want a TriggerBinding is for a Cron job, or something where you don't care about the event payload. Otherwise, you would want to pass event information to your PipelineRun/TaskRun/PipelineResource requiring a TriggerBinding.

A common way I would want to use this is to add additional params that I can use in the template that are not necessarily defined in the binding.

This is interesting; in your use case is there a reason why you wouldn't want to define the parameters in the TriggerBinding, then pass the parameters from the EventListener? For example

kind: TriggerBinding
metadata:
  name: pipeline-binding
spec:
  inputParams:
    - name: message
      description: The message to print
      default: This is the default message
  outputParams:
    - name: gitrevision
      value: $(event.head_commit.id)
    - name: gitrepositoryurl
      value: $(event.repository.url)
    - name: message
      value: $(inputParams.message)
---
kind: EventListener
metadata:
  name: listener
spec:
  triggers:
    - binding:
        name: pipeline-binding
        params:
          - name: message
            value: Hello there
      template:
        name: pipeline-template

Also, I think that the point of this issue is to get away from hardcoding parameters into the TriggerBinding. So, in your second example, we wouldn't want a user to hardcode

- name: dockerRegistry
  value: ncskier

in the TriggerBinding, because we want the TriggerBinding to be reusable. So, this would be hardcoded elsewhere (either in the EventListener, or another resource).

from triggers.