-
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
Refactor events to decouple k8s event and cloud event #5817
Conversation
Skipping CI for Draft Pull Request. |
f1721ff
to
4e66060
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
06ec0f8
to
d6a4947
Compare
cc @lbernick |
The following is the coverage report on the affected files.
|
} | ||
|
||
// EmitCloudEventsWhenConditionChange emits CloudEvents when there is a change in condition | ||
func EmitCloudEventsWhenConditionChange(ctx context.Context, beforeCondition *apis.Condition, afterCondition *apis.Condition, object runtime.Object) { |
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'm not sure if we should merge this function with EmitCloudEvents
, the only difference is to check the condition.
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 think it's ok to do that in a separate PR.
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.
👍
} | ||
|
||
// EmitCloudEventsWhenConditionChange emits CloudEvents when there is a change in condition | ||
func EmitCloudEventsWhenConditionChange(ctx context.Context, beforeCondition *apis.Condition, afterCondition *apis.Condition, object runtime.Object) { |
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 think it's ok to do that in a separate PR.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lbernick 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 |
The following is the coverage report on the affected files.
|
@Yongxuanzhang can we add ad test for EmitCloudEventsWhenConditionChange? |
Sure! It is actually tested from the Emit() function, is it ok to just copy some test cases there? |
This commit refactors the code in events to decouple the k8s events emit and cloud events emit. This commit fixes tektoncd#4404.
599fec7
to
3b1b392
Compare
The following is the coverage report on the affected files.
|
/lgtm |
Changes
This commit refactors the code in events to decouple the k8s events emit
and cloud events emit. This commit fixes #4404. This could avoid circular dependencies. The
events
intest
is also moved to k8sevent pkg.This also pave the path for #5770
/kind cleanup
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
/kind <type>
. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes