Proposal: Make triggers less generic and more coupled to Pipelines

[bobcatfish]

TriggerTemplates are very generic and can (theoretically) be used to create anything. We initially made them very generic on purpose b/c this felt like a better design: less coupling = better!

However, by making them less generic, the interface is pretty verbose and we lose out on validation (i.e. we have no idea what you want to create until you try to create it).

We've also started to talk about co-ordinating releases https://github.com/tektoncd/plumbing/issues/413 and it seems like the releases that it makes the most sense to combine are pipelines + triggers. We've also debated how to display what versions of triggers + pipelines items in the catalog are compatible with, and what the CLI and dashboard will be compatible with.

I'd like to propose: what if we release Triggers + Pipelines together? If we're doing that, what if we couple them together as well? And if want to take this to its extreme, we could even move them into the same repo (this would simplify things such as consuming updates from knative/pkg for our controllers).

We don't have to go that far just yet, but in the near term I'd like to propose an update to TriggerTemplates that is more coupled to Pipelines. If we go this route, we might even want to rename the type, and deprecate TriggerTemplates.

So my proposal is: replace TriggerTemplates with something else. I'm not sure what to call it, it's not exactly a template, but it would be used to create instances of PipelineRuns, TaskRuns and Runs - maybe it's a "factory"? ( @ImJasonH you might have some naming ideas here) "recipe" even? PipelineRecipe <-- that's my strawman proposal but totally open to anything better.

Here's what it could look like (based loosely on https://github.com/tektoncd/plumbing/blob/0ccc386dc6247a4b6e0669d8ab2b11a3f5a81d51/tekton/ci/plumbing-template.yaml):

# There's definitely a question of what api group this is part of: it's so coupled
# to Pipelines that maybe it makes sense to have it in the pipelines api group?
# If we DO want to go as far as combining the repos, we might want to use the same
# api group for both triggers and pipelines
apiVersion: triggers.tekton.dev/v1beta1
kind: PipelineRecipe
metadata:
  name: tekton-plumbing-ci-pipeline
spec:
  params:
  - name: buildUUID
  - name: package
  - name: pullRequestNumber
  - name: gitRepository
  - name: gitRevision
  - name: gitCloneDepth
  - name: gitHubCommand
  pipelineruns:
  -  metadata:
        name: tekton-noop-check-$(uid)
        annotations:
          tekton.dev/gitURL: "$(r.params.gitRepository)"
     serviceAccountName: tekton-ci-jobs
     pipelineRef: # I made all the "spec" fields top level - could keep spec, but what im trying to show is that PipelineRecipe would be more aware of Pipelines
       name: tekton-noop-check
     params:
       - name: passorfail
         value: pass
       - name: gitCloneDepth
         value: $(params.gitCloneDepth) # "r.params" for recipe? i dunno :D - or we might not need it since in the spec embedding case we'd know more about the pipeline and it's params
       - name: fileFilterRegex
         value: "(tekton-demo)"
  # We'd have explicit sections for Pipeline types we know about
  taskruns:
    ...
  runs:
    ...

In this version, we've removed a lot of the templating ability you have in TriggerTemplates, e.g. you can no longer use templating to control the names of fields, only the values. I think that might be a good thing, but we also don't have to go all the way to this extreme.

What do you think???

[imjasonh]

I really like the general direction of this change. I don't have any good naming suggestions but I'll keep thinking about it.

While we're making this change, do we think that we have to keep the 1:N mapping so that TriggerWhatevers can spawn multiple PipelineRuns?

[bobcatfish]

do we think that we have to keep the 1:N mapping so that TriggerWhatevers can spawn multiple PipelineRuns?

@ImJasonH do you think it would be better to have 1:1? I don't think I've thought about it too much - in https://github.com/tektoncd/plumbing/blob/0ccc386dc6247a4b6e0669d8ab2b11a3f5a81d51/tekton/ci/plumbing-template.yaml we have multiple PipelineRuns, so maybe @afrittoli has some thoughts

[imjasonh]

Oh I don't have a strong opinion or any signal either way. I just thought while we're changing the API we can take the opportunity to reassess other decisions if we thought it could be helpful. This could also be a distraction! 😅

[siamaksade]

I like the idea of a pipeline recipe as a blueprint for creating pipeline run, task run and runs for a particular pipeline, a resource that contains the config data. However this doesn't need to be part of triggers. Trigger would be one way to invoke the API to create instances of pipeline runs based on the recipe, but the same need exists for CLI or Tekton Dashboard.

For example with tkn, I would be able to run tkn pipeline start and point to a recipe, instead of numerous arguments to configure workspaces, service accounts, pipeline resource, etc.

[bobcatfish]

oooo i like that @siamaksade !

(also maybe: PipelineBlueprint?)

[bitsofinfo]

hopefully this wouldn't involve a major breaking change, i.e. refactoring of existing trigger config setups running in folks prod systems to comply w/ this change. Works fine for me as is, and today it makes intuitive sense on how it fits together. Why change it? If "recipies" needs to be added maybe do it separately, whenever I hear of a "recipe" i think of "wizardy" types of things which I avoid. The less coupling the better. The name choice of "trigger" to me seems perfect. The generic and totally open way triggers is currently implemented is exactly why I like it, I can do all sorts of very custom things with it.

stick w/ more generic and less coupled.

you can no longer use templating to control the names of fields, only the values

nooo!

[dibyom]

@bitsofinfo curious -- whats your use case for using variable interpolation for the field names. I've always looked at that as an unintentional "feature" since I can't really think of a use case for it 👼

[bobcatfish]

hopefully this wouldn't involve a major breaking change

if we did move to this, it would be a breaking change. im sure TriggerTemplates would continue to be supported for a while - we could choose to support them indefinitely, but once we do a beta release we could decide to drop support for them

Apologies for the potential inconvenience, but being able to make a change like this is one of the reasons triggers is currently alpha and not yet beta

We could make the migration easier by providing tools

[bitsofinfo]

so this isn't a proposal but more of a feature that is definitely happening?

[bobcatfish]

so this isn't a proposal but more of a feature that is definitely happening?

it's a proposal

[dibyom]

Thanks for the detailed write up @bobcatfish

• We should definitely figure out a way to do better validation for the templates. This has come up in other issues as well (see #678)

• No strong opinions on renaming the type itself (maybe TektonTemplates to indicate these are for Tekton resources only, or RunTemplates in a future where we only have Run objects that are usually created)

• TriggerTemplates (or the Shiny New Thing) should definitely be usable by things beyond just Triggers e.g. #200 or manual Trigger invocations from CLI/Dashboard, plus things like the git polling operator etc.

• @khrm and @gabemontero mentioned coupling Triggers and Pipeline releases together might make it easier for the Operator as well.

• Personally, I'd be ok with the extreme option of merging the repos together -- but even if we don't go that route maybe it makes sense to have Templates (or the New Thing) be part of the Pipelines API, given that it can be used by more than just Triggers and so that it can benefit from better validation/versioning.

[bobcatfish]

whenever I hear of a "recipe" i think of "wizardy" types of things which I avoid. The less coupling the better

Totally: the less magic the better! And that's pretty much exactly why we made the design so generic in the first place.

The generic and totally open way triggers is currently implemented is exactly why I like it, I can do all sorts of very custom things with it.

And @bitsofinfo, if we decide to pursue this (if!), we could decide that we want to have BOTH the more coupled type and continue to support the ultra generic TriggersTemplates as well even into beta.

Are you able to provide more detail about how are you are using the more generic features TriggersTemplates?

[abayer]

This looks really solid to me - being able to point to a recipe in Lighthouse for a presubmit or postsubmit, rather than needing a full PipelineRunSpec, would be great.

[eddycharly]

I definitely liked the genericity of trigger templates.

I see the advantages of being able to validate the template content but I also hoped that it would let me create any kind of resource.

Would it make sense to support both ?

[bitsofinfo]

@dibyom looking through my current use of triggers I don't have any $() var usage within the name: or key: or ref attrs for params or CEL overlays etc across interceptors, TriggerBindings, TriggerTemplates etc. I do make extensive use of this across values in all 3 of these areas however.

I do use it in the metadata.name of items listed under resourcetemplates and despite not doing so now would expect that all fields in objects in the resourcetemplates list could reference variables; names, vals, pipelieRef.name's etc, serviceaccountname etc; as after all they are "templates".

[rakhbari]

I put my vote down for keeping the existing generic template/binding/eventlistener functionality as is. I really like the fact that I can "cause" the creation of any Kube object with any sort of internal or external event. The separation of the EL, from definition of what's to be created (TriggerTemplate), and then having the binding glue things into TrigerTemplate was brilliant and I certainly wouldn't want that changed.

I don't see it as too verbose. A bit of a steep learning curve when you're first trying to get your head around it, but when you understand what it's trying to achieve and how powerful it is, you get it immediately. Some better documentation can help with the learning curve. Also, automation tooling can make the job of creating them easier. But I wouldn't want to devolve the model in any way because of these reasons.

we lose out on validation (i.e. we have no idea what you want to create until you try to create it).

@bobcatfish I'm not sure what you mean by this. TriggerTemplate already contains resourcetemplates.kind and with that knowledge the rest of the fields can be precisely validated. Or am I missing something?

I realize this is way after the fact, but my input on the name TriggerTemplate is that it's not really a "template" for the trigger, rather a template for the TARGET object (resource) of the trigger. So I thought it would be better named as TriggerTargetTemplate. But like I said, that ship has already sailed, so I'm happy to live with TriggerTemplate.

release Triggers + Pipelines together? If we're doing that, what if we couple them together as well?

My 2-cent's worth of advice on this... I'm generally against mono-repos. Keep the code bases as small as possible, singular function in mind, low coupling through interfaces, etc. Compatibility between projects can be easily managed with an addition of a compatibility map that lists the project and a minimalVersion string that can be examined at runtime and proper error messages produced if there's an attempt to run it with a lower than minimalVersion.

PipelineRecipe

I really don't see a need for this or what it also could be called as PipelineTemplate. From everything I can tell, modeling a Pipeline as has been done and then injecting runtime args into it and modeling that as a PipelineRun run is plenty enough.

[imjasonh]

@rakhbari What types of non-Tekton resources are you creating using Triggers?

[siamaksade]

@rakhbari after you create a TriggerTemplate and specify all the resources you need to be created in it when a trigger comes, let's say you want to test your pipeline with manually starting it. What do you do that doesn't involve duplication and copy-pasting snippets from TriggerTemplate into a PipelineRun? Let's say afterward you update your pipeline and add a new parameter that doesn't have a default value. How do you avoid updating this single change in multiple places (TriggerTemplate and PipelineRuns)?

[rakhbari]

@ImJasonH We (eBay) have greatly extended our internally installed/maintained Kubernetes clusters with several CRDs. A few, having to do with recording of "quality gates" need to be created when certain events occur in our Github Enterprise servers. The EventListener mechanism is the absolute perfect vehicle for these use cases.

So the versatility of the triggers/binding/EL mechanism can't be overstated. Regardless of what sort of optimization is made to create a more pipeline-centric mechanism, I for one would very much advocate for that to be an addition and keeping the existing triggers/binding/EL functionality as is.

[rakhbari]

@siamaksade Frankly the PipelineRun I created manually are no longer of any use to me once I've embedded them in a TriggerTemplate. I may maintain one in my environment config repo, just as a way to manually test changes to the pipeline it targets, but once I've vetted those changes, the TriggerTemplate will be my source of truth for those changes, since that's what actually gets invoked when Github events occur.

I actually thought long and hard about the idea of having a TriggerTemplate somehow "refer" to a PipelineRun, so I don't have to repeat params, resources, in both places. But a PipelineRun is already a fairly minimal runtime-centric object. It's a "created" resource and as such its identifier is a new value on every invocation. So you can say that a Pipeline is effectively a "template" object for a PipelineRun and no matter what you do you have to have something to pass in the runtime params, resources, etc, into a Pipeline, and the PipelineRun object is it.

And if you want to have a way of something else automate the creation of this "run" object, there's really no choice but to repeat those runtime params, resources, etc in it.

As I said, I'm not against creating an additional mechanism to couple (short-circuit) the TriggerTemplate/Binding/EL mechanism to be very pipeline-centric. Heck, I may even use it. But I just wouldn't want to lose the existing TriggerTemplate/Binding/EL functionality. I think it was brilliantly (and elegantly) designed and works quite well for a majority of use cases.

[gabemontero]

Cool idea, regardless if it remains as a "change to triggers" as originally cited, or if it evolves into a "something additional instead of changing something existing" as the existing usage etc. gets sorted out.

At least for now, I've only got one thing to add to the prior comments: one of the discussion points in the trigger CRD TEP, both in the TEP itself and in WG discussions around the subject, has been around the trigger author possibly needing to access the current EventListener pod logs to assist in diagnostics.

Would we envision a similar concern with this path? Or is mitigated somehow?

Any thoughts / details from anybody participating here either way would help me. I think I can guess at a few answers, but feel like I need to think about it more before I could post something with a semblance of sanity. Or maybe given the newness of this proposal, it is simply/reasonably a consideration to add to the list?

Thanks

[dibyom]

Summarizing discussion from the WG last week:

1. Generic templates for non tekton resources seem popular even though today we restrict TriggerTemplates only to Tekton resources. There are also some concerns around renaming the resource and backwards compatibility. We can keep exploring if we should support both a Generic type like today as well as a more coupled type.

2. Better validation should be possible even for the generic TriggerTemplate that we have today (we can track that in #678)

3. TriggerTemplates should be usable by CLI, Dashboard, Triggers - this sounds related to the TriggerTemplateRun idea proposed in #200

4. Field name variable interpolation -- let's keep discussing this to find out the use cases.

[tekton-robot]

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.

[bobcatfish]

I think this is an important discussion to have before the beta release. talking with @dibyom last week i think he's planning to follow up on this.

/remove-lifecycle stale