Issue55 reusable bindings #63

vtereso · 2019-08-06T21:45:44Z

Changes

Modify EventListener/TriggerBinding to promote TriggerBinding reuse. Without this change, the additions of conditions and headers within the TriggerBinding would have prevented them from being reusable. Now, input parameters can be specified from the EventListener such that TriggerBindings can be used between multiple EventListeners.

Also, this PR connects TriggerBindings and TriggerTemplates in a one-to-one relationship. My reasoning is that when validation is added, it would be most clear to validate all outputParams from the TriggerBinding against all params in the TriggerTemplate. In this way, the template should likely do everything necessary rather than segment everything between different templates. Also, between templates there is no way to cross reference generated resources, which could be another point of confusion if this was supported.

UPDATE: As per the WG discussion, it seems we are aligned in the one-to-one TriggerBinding-TriggerTemplate approach.

Fixes #55

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

tekton-robot · 2019-08-06T21:45:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vtereso

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [vtereso]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

vtereso · 2019-08-07T14:01:51Z

@EliZucker how does this look?

vtereso · 2019-08-07T14:35:53Z

Waiting on #67
/hold

vtereso · 2019-08-14T20:57:44Z

/hold cancel

vtereso · 2019-08-15T13:42:41Z

This PR places connects a serviceAccount per trigger, but another approach could be to have a single serviceAccount for the entire EventListener.

bobcatfish

Nice, I think this is an improvement for sure!!! I left a bunch of random comments, I think my feedback is:

I think it could make sense to remove validation from TriggerBinding (I'm happy to open an issue if there isn't one already)
We should get docstrings into our _types.go asap (esp. as we make changes, to make it clear to everyone what the fields were doing and what they will do)

Also @vtereso could you add some more details to the commit message? Right now it just says "Restructure TriggerBinding to make them reusable" but it would make sense to summarize (as briefly as possible) the conclusions reached in #55 to explain the context of the change - more tips in the commit message guidelines)

README.md

docs/eventlisteners.md

docs/triggerbindings.md

bobcatfish · 2019-08-15T14:44:54Z

docs/triggerbindings.md

+    - name: gitrevision
+      value: $(event.head_commit.id)
+    - name: gitrepositoryurl
+      value: $(event.repository.url)


can we show inputParams and the other fields in this example as well, and document how they work?

Since we don't have conditionals/header matching yet, I omitted them for the time being.

hm id rather either:

not include them in the types until we're ready to document them

include them in the docs but not have the functionality (which is what we've done for most other types anyway)

my personal feeling is that before 0.1 it's okay to have our docs be aspirational - if we intend to have the functionality in 0.1, i think we can put it in the docs before it actually works (as long as it does before 0.1). but if we dont intend to include it in 0.1, i dont think we should include the types in the code at all

Since TriggerTemplates may not even take params if #71 goes through, and also to keep this PR lightweight to meet the milestone, I have removed params from the types until we're ready to implement and document them.

bobcatfish · 2019-08-15T14:45:56Z

pkg/apis/triggers/v1alpha1/event_listener_types.go

+type Trigger struct {
+	TriggerBinding     TriggerBindingRef  `json:"triggerBinding"`
+	TriggerTemplate    TriggerTemplateRef `json:"triggerTemplate"`
+	ServiceAccountName string             `json:"serviceAccountName"`


can we have docstrings for all the structs + attributes we're adding?

(looks like we need to turn on more linting maybe, there should be linting that catches when exported values don't have docstrings 🤔 )

Adding -E golint to our lint command (golangci-lint run) should allow us to get this behavior, but when I tried it only complained about naming stutter errors. Maybe open an issue for it? Pipelines doesn't seem to have this enabled either.

kk! Ive opened tektoncd/plumbing#64 in the plumbing repo so hopefully we can get this for both projects :D (and cli too?)

For now, I have added docstrings for the existing public types.

pkg/apis/triggers/v1alpha1/event_listener_types.go

pkg/apis/triggers/v1alpha1/event_listener_validation.go

EliZucker

I have addressed the existing comments on this PR. Ready for more feedback or lgtm!

EliZucker · 2019-08-19T20:59:02Z

docs/triggerbindings.md

+    - name: gitrevision
+      value: $(event.head_commit.id)
+    - name: gitrepositoryurl
+      value: $(event.repository.url)


Since TriggerTemplates may not even take params if #71 goes through, and also to keep this PR lightweight to meet the milestone, I have removed params from the types until we're ready to implement and document them.

EliZucker · 2019-08-19T22:02:23Z

pkg/apis/triggers/v1alpha1/event_listener_types.go

+type Trigger struct {
+	TriggerBinding     TriggerBindingRef  `json:"triggerBinding"`
+	TriggerTemplate    TriggerTemplateRef `json:"triggerTemplate"`
+	ServiceAccountName string             `json:"serviceAccountName"`


For now, I have added docstrings for the existing public types.

EliZucker · 2019-08-20T02:46:09Z

/test pull-tekton-triggers-build-tests

EliZucker · 2019-08-20T13:53:41Z

/retest

vtereso · 2019-08-20T15:39:37Z

@EliZucker Nice updates.

I thought it would be nice to have one service account per trigger to separate the permissions, but actually only one service account can be used (at the moment) since each EventListener creates a single deployment (where the podTemplate takes one serviceAccount). ServiceAccountName as a result should live directly under spec. Also, since we are likely not using input params in the TriggerBinding, maybe params (as prior) is more appropriate than outputParams?

vtereso · 2019-08-20T20:01:29Z

Another improvement could also clean up the json tags:
json:"triggerBinding" -> json:"binding"
json:"triggerTemplate" -> json:"template"

EliZucker · 2019-08-20T20:20:44Z

@vtereso All really good points, thanks. Should be updated now.

bobcatfish

Looking great! I am completely ready to lgtm, but the one request I have is can we add more context to the commit message? (e.g. see https://github.com/tektoncd/community/blob/master/standards.md#commit-messages) 🙏

bobcatfish · 2019-08-21T15:16:07Z

pkg/apis/triggers/v1alpha1/trigger_binding_validation.go

 func (t *TriggerBinding) Validate(ctx context.Context) *apis.FieldError {
 	return t.Spec.Validate(ctx)
 }

+// Validate TriggerBindingSpec.


if you feel like it, this is a place where it would make sense to say something like "there is nothing to validate in a TriggerBindingSpec b/c xyz"

This comment reminded me there actually is a bit of validation we can do... Added now.

bobcatfish · 2019-08-21T15:16:36Z

cmd/eventlistener/main.go

 		}
+		log.Print(tt)


we're going to want to start adding unit tests for the eventlistener asap but np if you dont want to do it in this PR

Modify EventListener and TriggerBinding such that TriggerBindings can be reused. This was proposed and discussed in tektoncd#55 . Instead of specifying a particular TriggerTemplate within a TriggerBinding (as was previously proposed), users now specify one-to-one relationships between TriggerBindings and TriggerTemplates within an EventListener. Catalogs of TriggerTemplates can exist and be reused with this change. Although adding Params may be something we want to do for reusability in the future, particularly for the case where filtering/conditions are embeded within the TriggerBinding, it is out of this PR's scope. We may separate the conditional/filtering logic in which case parameters would make more sense there anyway. Co-authored-by: Vincent-DeSousa-Tereso <[email protected]>

bobcatfish · 2019-08-22T20:14:16Z

Thanks for the commit message update!

Let's do this 😎

/lgtm

tekton-robot requested review from dlorenc and iancoffey August 6, 2019 21:45

tekton-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 6, 2019

vtereso force-pushed the Issue55-ReusableBindings branch 4 times, most recently from d8d2ca4 to 41336d4 Compare August 7, 2019 13:48

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 7, 2019

vtereso force-pushed the Issue55-ReusableBindings branch 6 times, most recently from 15ce3d2 to 10af96d Compare August 7, 2019 16:41

ncskier mentioned this pull request Aug 7, 2019

Update TriggerBinding type to only have one event type #57

Closed

3 tasks

vtereso force-pushed the Issue55-ReusableBindings branch 6 times, most recently from 3834c0a to 7fef8e7 Compare August 8, 2019 18:17

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 14, 2019

vtereso force-pushed the Issue55-ReusableBindings branch 3 times, most recently from bfbe173 to afe3698 Compare August 14, 2019 21:27

bobcatfish reviewed Aug 15, 2019

View reviewed changes

bobcatfish mentioned this pull request Aug 16, 2019

Linting should catch exported values without docstrings tektoncd/plumbing#64

Closed

EliZucker force-pushed the Issue55-ReusableBindings branch from 66a02cf to 9f984cd Compare August 19, 2019 20:53

EliZucker reviewed Aug 19, 2019

View reviewed changes

EliZucker force-pushed the Issue55-ReusableBindings branch from c91ccc0 to 2d07ce4 Compare August 20, 2019 14:48

EliZucker force-pushed the Issue55-ReusableBindings branch from 2d07ce4 to 574e227 Compare August 20, 2019 20:12

EliZucker force-pushed the Issue55-ReusableBindings branch 2 times, most recently from f553b7b to 504db96 Compare August 20, 2019 20:57

bobcatfish reviewed Aug 21, 2019

View reviewed changes

EliZucker force-pushed the Issue55-ReusableBindings branch from 504db96 to de67026 Compare August 22, 2019 20:07

tekton-robot assigned bobcatfish Aug 22, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 22, 2019

tekton-robot merged commit f61c392 into tektoncd:master Aug 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue55 reusable bindings #63

Issue55 reusable bindings #63

vtereso commented Aug 6, 2019 •

edited by bobcatfish

Loading

tekton-robot commented Aug 6, 2019

vtereso commented Aug 7, 2019

vtereso commented Aug 7, 2019 •

edited

Loading

vtereso commented Aug 14, 2019

vtereso commented Aug 15, 2019

bobcatfish left a comment

bobcatfish Aug 15, 2019

vtereso Aug 15, 2019

bobcatfish Aug 16, 2019

EliZucker Aug 19, 2019

bobcatfish Aug 15, 2019

vtereso Aug 15, 2019 •

edited

Loading

bobcatfish Aug 16, 2019

EliZucker Aug 19, 2019

EliZucker left a comment

EliZucker Aug 19, 2019

EliZucker Aug 19, 2019

EliZucker commented Aug 20, 2019

EliZucker commented Aug 20, 2019

vtereso commented Aug 20, 2019

vtereso commented Aug 20, 2019 •

edited

Loading

EliZucker commented Aug 20, 2019

bobcatfish left a comment

bobcatfish Aug 21, 2019

EliZucker Aug 22, 2019

bobcatfish Aug 21, 2019

bobcatfish commented Aug 22, 2019

Issue55 reusable bindings #63

Issue55 reusable bindings #63

Conversation

vtereso commented Aug 6, 2019 • edited by bobcatfish Loading

Changes

Submitter Checklist

tekton-robot commented Aug 6, 2019

vtereso commented Aug 7, 2019

vtereso commented Aug 7, 2019 • edited Loading

vtereso commented Aug 14, 2019

vtereso commented Aug 15, 2019

bobcatfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtereso Aug 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EliZucker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EliZucker commented Aug 20, 2019

EliZucker commented Aug 20, 2019

vtereso commented Aug 20, 2019

vtereso commented Aug 20, 2019 • edited Loading

EliZucker commented Aug 20, 2019

bobcatfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobcatfish commented Aug 22, 2019

vtereso commented Aug 6, 2019 •

edited by bobcatfish

Loading

vtereso commented Aug 7, 2019 •

edited

Loading

vtereso Aug 15, 2019 •

edited

Loading

vtereso commented Aug 20, 2019 •

edited

Loading