Support for dynamic params to TriggerTemplates beyond just event values about triggers HOT 12 CLOSED

I like it!

Might be a noob questions, but would we need to explicitly list all fields in outputs? It seems like it would be great if we could treat the output of the step similar to the event in TriggerTemplates.

e.g. for the examples you gave above, being able to reference the ParamBinding in the TriggerBinding:

spec:
  params: # side note - are these outputs? i think thats the intention...
    - name: version
      value: $(event.head_commit.id)
    - name: version
      value: $(nightly-version-binding.versionTag)

For filtering, this may not solve all problems since we might want to make some bindings conditional (e.g. don't call out to GitHub to check membership if we don't even care about the repo).

from triggers.

@bobcatfish Do you feel that this has been "solved" by adding interceptors as well as static parameters in the EventListener?

from triggers.

I agree. Interceptors (both CEL overlays + Webhooks) can modify the event body and add extra "dynamic" parameters

from triggers.

In the case of creating a release, there is no event to process? A "manual" TaskRun/Pipeline could use whatever parameters necessary to do this. Alternatively, if a tag was added to the repository, a "tag trigger" (or some such event that contains the necessary params) could take this event information and work as current to upload a release?

from triggers.

I think having additional parameters exterior to the event might be unavoidable though and we wouldn't want to ruin re-usability. To this point, the initial thought is to push more into the EventListener since it isn't reusable.

from triggers.

Also, this gets at the discussion of reusability in general and how much there really is. Spoke with @ncskier about this yesterday and a TriggerTemplate can only be as reusable as the resources it is templating (e.g. a Pipeline). I can easily be swayed to the pursuit of reusability, but I don't believe the case is as strong as with core Tekton. If tools like tkn or the skylark approach pick up steam, maybe a catalog is unnecessary and reusability becomes less important? Just a thought.

from triggers.

The way I implemented this for now is an initContainer in the CronJob that triggers the EventListener: https://github.com/tektoncd/plumbing/blob/master/tekton/config/nightly-release-cron-base/trigger-with-uuid.yaml#L35.

I could do the same with an interceptor, but it's not clear for me yet how to best use them.
Since interceptors are services, if I have to create a new service for every time I need to inject an extra parameter, I will end up with a lot of services that do nothing most of the time.
I could use one interceptor to handle different pipelines, but then I would lose the separation of concerns, and my interceptor would become more complex and hard to maintain.

from triggers.

In PR #438, a discussion started about the use-case of chaining Interceptors: #438 (comment)

@ncskier A use case for chained interceptors - validate using a Github interceptors, add a webhook interceptor to call out to GH for adding additional info, and then perhaps a CEL filter. I think these would be fairly common.

The use-case described made me think about this issue, and whether it should be the job of an Interceptor to "call out" for additional information, or if we need a separate mechanism in Triggers for this.

Interceptors were initially built as a way to validate events. However, their role has now grown to include calling out for additional information (the feature called "dynamic params" in this issue).

If we're happy with Interceptors taking on this dual-role, then we can close this issue. However, I think that it would be helpful to revisit this issue, because @afrittoli raised some good points about why we might not want to use Interceptors to get "dynamic params" :

Since interceptors are services, if I have to create a new service for every time I need to inject an extra parameter, I will end up with a lot of services that do nothing most of the time.
I could use one interceptor to handle different pipelines, but then I would lose the separation of concerns, and my interceptor would become more complex and hard to maintain.

I'm interested to know if anything about this issue has changed since it was last discussed in October 😅

from triggers.

I think with the inclusion of WebhookInterceptors we can close this out.

Since interceptors are services, if I have to create a new service for every time I need to inject an extra parameter, I will end up with a lot of services that do nothing most of the time.
I could use one interceptor to handle different pipelines, but then I would lose the separation of concerns, and my interceptor would become more complex and hard to maintain.

I think #270 will help this. You can either provide a Service, or a URL (where you could host multiple interceptor endpoints). I suspect possibly adding a URL path to Services may also help this out this particular case, this way you could deploy a single Service and route accordingly.

I suspect this is an area we can learn from Knative Addressables (e.g. https://github.com/knative/pkg/blob/d9a38f13e8b9aa736f714b793ee28788de1b30e0/apis/duck/v1beta1/destination.go#L26-L47)

from triggers.

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close.

/lifecycle rotten

Send feedback to tektoncd/plumbing.

from triggers.

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.

/lifecycle stale

Send feedback to tektoncd/plumbing.

from triggers.

I feel like interceptors are the answer to this these days 🤔 , do you agree @dibyom @wlynch ?

from triggers.