-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Start emitting CloudEvents for TaskRuns #2837
Start emitting CloudEvents for TaskRuns #2837
Conversation
This PR cannot be merged: expecting exactly one kind/ label Available
|
This PR cannot be merged: expecting exactly one kind/ label Available
|
1 similar comment
This PR cannot be merged: expecting exactly one kind/ label Available
|
The following is the coverage report on the affected files.
|
WIP because tests are missing for the config map part of the PR - I need to test that the config option is pulled in correctly in the context. It may be worth adding at least one reconciler test. |
37e9ed5
to
127f563
Compare
127f563
to
7675af8
Compare
The following is the coverage report on the affected files.
|
7675af8
to
16c5ad0
Compare
The following is the coverage report on the affected files.
|
16c5ad0
to
d7c6327
Compare
The following is the coverage report on the affected files.
|
d7c6327
to
817a82e
Compare
The following is the coverage report on the affected files.
|
817a82e
to
b08f7c7
Compare
The following is the coverage report on the affected files.
|
b08f7c7
to
1b5cadd
Compare
The following is the coverage report on the affected files.
|
1b5cadd
to
83beab3
Compare
59da6a3
to
df55797
Compare
The following is the coverage report on the affected files.
|
df55797
to
b4235c0
Compare
The following is the coverage report on the affected files.
|
b4235c0
to
c47c926
Compare
The following is the coverage report on the affected files.
|
c47c926
to
a90bc08
Compare
The following is the coverage report on the affected files.
|
Should add unit tests to |
a90bc08
to
d4bbe49
Compare
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.
/meow
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vdemeester 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 |
Add a new method 'SendCloudEventWithRetries' to the cloud events controller. It that allows emitting cloud events asynchronously (in a go routine), taking benefit of the retries capabilities of the cloudevents go sdk. Rework the fake client to allow for unit testing of 'SendCloudEventWithRetries'. The new implementation is similar to that of the fake recorder from client-go, which allows to unit test k8s events and cloud events in a similar fashion. Add a new config option default-cloud-events-sink in the defaults config map. This options allows setting a default sink for cloud events. If the default sink is setup, cloud events are sent, else they're disabled. Invoke 'SendCloudEventWithRetries' from the events modules so that we start sending cloud events in the same places we were send k8s events (except for errors). Most of the plumbing is in place to start emitting cloud events for pipeline runs too, however there is a small refactor for the config maps required before we can add that, so this commit only enabled cloud events for task runs for now.
d4bbe49
to
84a60bd
Compare
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
Add a new method 'SendCloudEventWithRetries' to the cloud events
controller. It that allows emitting cloud events asynchronously
(in a go routine), taking benefit of the retries capabilities of
the cloudevents go sdk.
Rework the fake client to allow for unit testing of
'SendCloudEventWithRetries'. The new implementation is similar
to that of the fake recorder from client-go, which allows to
unit test k8s events and cloud events in a similar fashion.
Add a new config option default-cloud-events-sink in the
defaults config map. This options allows setting a default sink
for cloud events. If the default sink is setup, cloud events are
sent, else they're disabled.
Invoke 'SendCloudEventWithRetries' from the events modules so
that we start sending cloud events in the same places we were
send k8s events (except for errors).
Most of the plumbing is in place to start emitting cloud events
for pipeline runs too, however there is a small refactor for
the config maps required before we can add that, so this commit
only enabled cloud events for task runs for now.
Partially addresses #2082
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.
Double check this list of stuff that's easy to miss:
cmd
dir, please updatethe release Task to build and release this image.
Reviewer Notes
If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.
Release Notes
/cc @n3wscott
/cc @bobcatfish
/kind feature