-
Notifications
You must be signed in to change notification settings - Fork 420
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 ability to send cloudevent during trigger processing #1259
Conversation
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
bd194a4
to
ccf01b1
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
/test pull-tekton-triggers-integration-tests |
🎉 🎉 @khrm It would be great if you add doc on how to use cloudevents for EventListener |
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
/approve Some housekeeping things - Could you update the PR description + add a release note? |
The following is the coverage report on the affected files.
|
@@ -376,6 +400,8 @@ func (r Sink) processTrigger(t triggersv1.Trigger, request *http.Request, event | |||
return | |||
} | |||
go r.recordResourceCreation(resources) | |||
r.sendCloudEvents(request.Header, *el, eventID, events.TriggerProcessingSuccessfulV1) |
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.
Are we planning to send events of processTrigger as well ?
/cc @dibyom
Asking this because with that line only success event will be sent do we plan to send failure events as well?
like
finalPayload, header, iresp, err := r.ExecuteTriggerInterceptors(t, request, event, log, eventID, extensions)
if err != nil {
log.Error(err)
return
}
so above when there is error we are not capturing event
Same applicable for K8s as well
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.
TriggerProcessing errors aren't error most of the time. It's because of filters, etc. So we are sending only Successful Event if a trigger succeed.
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.
Ah okok could you add this as well
r.emitEvents(r.EventRecorder, el, events.TriggerProcessingSuccessfulV1, nil)
which handles for K8s
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.
Sure.
The following is the coverage report on the affected files.
|
Define `targetURI` field in EL spec to send cloud event during trigger processing. FeatureFlag should also be `alpha`.
The following is the coverage report on the affected files.
|
/test pull-tekton-triggers-integration-tests |
The following is the coverage report on the affected files.
|
/test pull-tekton-triggers-integration-tests |
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
Thank you 👍 🎉
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dibyom, savitaashture 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 |
Changes
Define
targetURI
field in EL spec to send cloud events during trigger processing. FeatureFlag should also bealpha
.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