Skip to content
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

Add Support for Embedding TriggerBindingSpec in EL #548

Merged
merged 1 commit into from
May 8, 2020

Conversation

khrm
Copy link
Contributor

@khrm khrm commented Apr 21, 2020

Changes

Fixes #371

Submitter Checklist

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

See the contribution guide for more details.

Release Notes


-  Support Embedding in EventListenerSpec
   TriggerBindingSpec can be added to EventListener Trigger.

-  Add Ref for TriggerBinding  in EventListenerSpec Trigger
   In addition to name, Ref also need to be specified. 
   At present, if ref isn't given and spec is also not there, name is considered as Ref. This behaviour will be deprecated in future releases. 

@tekton-robot tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 21, 2020
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 21, 2020

CLA Check
The committers are authorized under a signed CLA.

@tekton-robot tekton-robot requested review from dibyom and vtereso April 21, 2020 15:22
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 21, 2020
@khrm khrm force-pushed the embedBinding branch 2 times, most recently from 9d21426 to a405a57 Compare April 28, 2020 13:49
@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
test/builder/eventlistener.go 92.2% 88.2% -4.0

@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
test/builder/eventlistener.go 92.2% 88.2% -4.0

@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
test/builder/eventlistener.go 92.2% 92.6% 0.5

@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/triggers/v1alpha1/event_listener_defaults.go 100.0% 91.7% -8.3
pkg/apis/triggers/v1alpha1/event_listener_validation.go 92.6% 79.3% -13.3
test/builder/eventlistener.go 92.2% 92.6% 0.5

@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/triggers/v1alpha1/event_listener_defaults.go 100.0% 91.7% -8.3
pkg/apis/triggers/v1alpha1/event_listener_validation.go 92.6% 79.3% -13.3
pkg/template/resource.go 100.0% 98.0% -2.0
test/builder/eventlistener.go 92.2% 92.6% 0.5

@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/triggers/v1alpha1/event_listener_defaults.go 100.0% 91.7% -8.3
pkg/apis/triggers/v1alpha1/event_listener_validation.go 92.6% 79.3% -13.3
pkg/template/resource.go 100.0% 98.0% -2.0
test/builder/eventlistener.go 92.2% 92.6% 0.5

@khrm khrm changed the title WIP: Add Support for Embedding TriggerBindingSpec in EL Add Support for Embedding TriggerBindingSpec in EL Apr 28, 2020
@tekton-robot tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 28, 2020
@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/triggers/v1alpha1/event_listener_defaults.go 100.0% 91.7% -8.3
pkg/apis/triggers/v1alpha1/event_listener_validation.go 92.6% 79.3% -13.3
pkg/template/resource.go 100.0% 98.0% -2.0
test/builder/eventlistener.go 92.2% 92.6% 0.5

@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/triggers/v1alpha1/event_listener_defaults.go 100.0% 91.7% -8.3
pkg/apis/triggers/v1alpha1/event_listener_validation.go 92.6% 79.3% -13.3
pkg/template/resource.go 100.0% 98.0% -2.0
test/builder/eventlistener.go 92.2% 92.6% 0.5

@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/triggers/v1alpha1/event_listener_defaults.go 100.0% 91.7% -8.3
pkg/apis/triggers/v1alpha1/event_listener_validation.go 92.6% 79.3% -13.3
pkg/template/resource.go 100.0% 98.0% -2.0
test/builder/eventlistener.go 92.2% 92.6% 0.5

@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/triggers/v1alpha1/event_listener_defaults.go 100.0% 91.7% -8.3
pkg/apis/triggers/v1alpha1/event_listener_validation.go 92.6% 79.3% -13.3
pkg/template/resource.go 100.0% 98.0% -2.0
test/builder/eventlistener.go 92.2% 92.6% 0.5

@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/triggers/v1alpha1/event_listener_defaults.go 100.0% 91.7% -8.3
pkg/apis/triggers/v1alpha1/event_listener_validation.go 92.6% 93.1% 0.5
test/builder/eventlistener.go 92.2% 92.6% 0.5

@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/triggers/v1alpha1/event_listener_defaults.go 100.0% 91.7% -8.3
pkg/apis/triggers/v1alpha1/event_listener_validation.go 92.6% 93.1% 0.5
test/builder/eventlistener.go 92.2% 92.6% 0.5

@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/triggers/v1alpha1/event_listener_defaults.go 100.0% 91.7% -8.3
pkg/apis/triggers/v1alpha1/event_listener_validation.go 92.6% 93.1% 0.5
test/builder/eventlistener.go 92.2% 92.6% 0.5

@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/triggers/v1alpha1/event_listener_defaults.go 100.0% 91.7% -8.3
pkg/apis/triggers/v1alpha1/event_listener_validation.go 92.6% 93.1% 0.5
test/builder/eventlistener.go 92.2% 92.6% 0.5

@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/triggers/v1alpha1/event_listener_defaults.go 100.0% 91.7% -8.3
pkg/apis/triggers/v1alpha1/event_listener_validation.go 92.6% 93.1% 0.5
test/builder/eventlistener.go 92.2% 92.6% 0.5

@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/triggers/v1alpha1/event_listener_defaults.go 100.0% 91.7% -8.3
pkg/apis/triggers/v1alpha1/event_listener_validation.go 92.6% 93.1% 0.5
test/builder/eventlistener.go 92.2% 92.6% 0.5

@khrm
Copy link
Contributor Author

khrm commented Apr 30, 2020

@dibyom
I have kept name as an identifier to be used within EL irrespective of whether we have embedded or ref bindings. Atm, there doesn't seems to be a need for it but I can that we need to differentiate on how a param is coming from like here: #508
So we might need to do: tt.bindingname.param

@dibyom
Copy link
Member

dibyom commented May 4, 2020

I have kept name as an identifier to be used within EL irrespective of whether we have embedded or ref bindings. Atm, there doesn't seems to be a need for it but I can that we need to differentiate on how a param is coming from like here: #508
So we might need to do: tt.bindingname.param

That's definitely a valid scenario though I'm not sure if we'd want to couple bindings within the template. At the moment though, keeping both name and ref seems confusing. Can we remove them from the examples at least? (we can keep/support the field but let's document and use ref as the default?). Should be simple enough to add back name if we need to later.

@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/triggers/v1alpha1/event_listener_defaults.go 100.0% 91.7% -8.3
pkg/apis/triggers/v1alpha1/event_listener_validation.go 92.6% 92.9% 0.3
test/builder/eventlistener.go 92.2% 92.8% 0.6

TriggerBindingSpec can be added to EventListener Trigger
@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/triggers/v1alpha1/event_listener_defaults.go 100.0% 91.7% -8.3
pkg/apis/triggers/v1alpha1/event_listener_validation.go 92.6% 92.9% 0.3
test/builder/eventlistener.go 92.2% 92.8% 0.6

@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/triggers/v1alpha1/event_listener_defaults.go 100.0% 91.7% -8.3
pkg/apis/triggers/v1alpha1/event_listener_validation.go 92.6% 92.9% 0.3
test/builder/eventlistener.go 92.2% 92.8% 0.6

@dibyom
Copy link
Member

dibyom commented May 7, 2020

/approve

@tekton-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom

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:

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

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 7, 2020
@dibyom
Copy link
Member

dibyom commented May 8, 2020

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 8, 2020
@tekton-robot tekton-robot merged commit 454c8e0 into tektoncd:master May 8, 2020
@khrm khrm deleted the embedBinding branch July 28, 2021 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Embed TriggerBinding in EventListener triggers
4 participants