-
Notifications
You must be signed in to change notification settings - Fork 121
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
TektonListener and EventBinding CRD refactor proposal #19
Comments
Ive read through this and there is a lot of superb information in here, thank you for it. I think I need to digest it a bit more before being able to describe my perspective on the solutions above. My only initial takeaway is that, whatever we land on, I would like the There is def so much room for improvement in the types definitions, thanks again for this. Once Im over my cold I will drop a response of my thoughts here. Thanks! |
Thanks for reading it over Ian. I hope you feel better, and I look forward to hearing the rest of your thoughts on the proposal. Thanks! |
FWIW, I like this proposal 👍 |
@iancoffey |
@vincent-pli Check out the latest PR here #25, this removes most barriers regarding trying/demoing EventBindings. There will be a really in-depth Getting Started Guide PR'ed here soon. I think its important to see this work through before changing course too drastically, so my effort for now is 100% toward getting it all ready for usage, until next week or so. Or course, all options are on the table once things are in a good spot and Id expect some changes to the Types then. |
Pretty sure we're good to close this now aren't we 😆? Excellent work has been done by those above especially ^^ around Triggers (I'm doing an issue cleanup) 😄. For posterity and anyone new to this area, see https://github.com/tektoncd/triggers Edit: waited 25 days and heard nothing and we're good now, closing |
I'm creating this issue following the
TektonListener
andEventBinding
code being merged into the experimental repo in thetekton-listener
directory. These two CRDs add great value for eventing with Tekton, but I think they can be refactored to improve their usability.Sorry in advance for my long-winded explanations 😄
Why I think the CRDs should be refactored
My concerns with the current
TektonListener
&EventBinding
CRDs (documented here) are as follows:1. The
TektonListener
andEventBinding
store overlapping information aboutPipelineRun
s.First, both CRDs store the same
pipelineRef
field. Also, theTektonListener
stores an entirePipelineRunSpec
, but theEventBinding
also stores theparams
for thePipelineRunSpec
. There is a weird division where theTektonListener
owns thePipelineRun
information, and theEventBinding
owns thePipelineResource
information; however, theEventBinding
creates theTektonListener
, so theEventBinding
must also own some of thePipelineRun
information which it uses to create theTektonListener
. This ownership division is confusing to me, and I think that one of the CRDs should own all of thePipelineRun
information.2. The
TektonListener
andEventBinding
store overlapping information about eventing.This is similar to my previous concern about ownership of information. The
EventBinding
has anEventRef
field with anEventName
andEventType
. This same information is also stored in theTektonListener
as itsEvent
andEventType
fields. Again, this ownership division is confusing to me, and I think that one of the CRDs should own all of the eventing information.3. The
TektonListener
andEventBinding
s store reusable information that cannot be reusedTogether, the
TektonListener
andEventBinding
store the entire configuration of a Pipeline Execution: aPipelineRun
with its necessaryPipelineResources
. I think that this "Pipeline Execution" information should be reusable for different types of events. However, as I wrote in my previous two points, the current configuration stores overlappingPipelineRun
and eventing information in both theTektonListener
andEventBinding
CRDs. So, neither of these CRDs can be reused.For example, if a user has a working
TektonListener
andEventBinding
setup for GitHubpull_request
events, and now wants to use the same Pipeline with a different repository and GitLabpush
events, the user will need to create newTektonListener
andEventBinding
CRDs. Even though the "Pipeline Execution" configuration ofPipelineRun
andPipelineResource
s will be the same for these two new CRDs, the user will not be able to reuse either of the CRDs.4.
PipelineRun
s cannot use information from the event as parametersPipelineRun
s are not included in theEventBinding
template section, so their parameters cannot store information from the event.Proposal for refactoring the CRDs
My primary motivation with refactoring the CRDs is to separate the eventing information from the Pipeline Execution information, and to keep the core functionality that @iancoffey introduced intact. The following is an example of a simple Pipeline Execution triggered by a GitHub push event, using my refactored CRDs named
TemplateBinding
andTemplate
(the names as well as the designs are open to discussion):Similar to the
TektonListener
CRD, eachTemplateBinding
CRD would create its own event listener.Improvements
Here is how the proposed refactor addresses my previous concerns:
1. The
TektonListener
andEventBinding
store overlapping information aboutPipelineRun
s.The
Template
owns all the information aboutPipelineRun
s, aside from some parameters that it exposes to theTemplateBinding
.2. The
TektonListener
andEventBinding
store overlapping information about eventing.The
TemplateBinding
owns all the information about eventing.3. The
TektonListener
andEventBinding
s store reusable information that cannot be reusedThe
Template
stores the Pipeline Execution information that can be reused by any number ofTemplateBinding
s. This will be especially useful when you want to create a Pipeline definition, because your Pipeline can support any type of eventing by providing aTemplate
file along with yourPipeline
andTask
YAML files.For example, if a user has a working
Template
andTemplateBinding
setup for GitHubpull_request
events, and now wants to use the same Pipeline with a different repository and GitLabpush
events, the user will only need to create one newTemplateBinding
CRD.4.
PipelineRun
s cannot use information from the event as parametersThe
Template
can expose a parameter used in thePipelineRun
template, and theTemplateBinding
can populate this parameter with information from the event.Feedback
Please let me know what you think about this refactor proposal 🙂
@iancoffey @dlorenc
The text was updated successfully, but these errors were encountered: