-
Notifications
You must be signed in to change notification settings - Fork 414
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 TriggerCRD object Ref to ELSpec Trigger #726
Conversation
The following is the coverage report on the affected files.
|
7e997ae
to
03b9f10
Compare
The following is the coverage report on the affected files.
|
d037aa4
to
75e7569
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good -- just some minor comments about the number of yaml files for the example and one question on string vs *string.
examples/triggers/binding.yaml
Outdated
@@ -0,0 +1,11 @@ | |||
apiVersion: rbac.authorization.k8s.io/v1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: I know we have not been very consistent with the RBAC resources in our examples. Could we either symlink to the resources in the role-resources/ folder or at least move binding.yaml, role.yaml, serviceaccount.yaml into a single file (say, rbac.yaml?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the idea being -- the fewer yaml files needed to get started with an example, the easier it is for a first time easier to understand -- ideally, one day we can do something like the pipeline examples where each example contains a single file containing a taskRun with an embedded taskSpec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added everything related to rbac in a single file.
Template EventListenerTemplate `json:"template"` | ||
Bindings []*EventListenerBinding `json:"bindings,omitempty"` | ||
Template *EventListenerTemplate `json:"template,omitempty"` | ||
TriggerRef string `json:"triggerRef,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be use *string here since TriggerRef is optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's not necessary. Empty string or not giving anything is invalid, so we are good with this. The empty string will be considered as omitempty while using go client. An empty string or not giving anything will be considered omitempty while using json.
[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 |
Support for adding Trigger CR Object as Ref to EventListenerSpec.
The following is the coverage report on the affected files.
|
Example eventlistener-triggerref and trigger created. Also, added documentation of Trigger and information in Eventlistener for refering to Trigger.
The following is the coverage report on the affected files.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Changes
Support for adding Trigger CR Object as Ref to EventListenerSpec
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