-
Notifications
You must be signed in to change notification settings - Fork 94
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
✨ support cloudevents for manifestworkreplicaset #352
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #352 +/- ##
==========================================
+ Coverage 61.33% 61.37% +0.04%
==========================================
Files 132 132
Lines 14036 14051 +15
==========================================
+ Hits 8609 8624 +15
Misses 4672 4672
Partials 755 755
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
c01659e
to
ddb6a35
Compare
f761d93
to
be2e698
Compare
/assign @qiujian16 |
/assign @morvencao |
pkg/work/hub/manager.go
Outdated
return err | ||
const ( | ||
sourceID = "mwrsctrl" | ||
sourceClientID = "mwrsctrl-client" |
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.
nit: Is this safe to have the same hard-code clientID for mwrs controller?
does this mean anytime, only one mwrs controller instance is working?
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.
maybe need to expose these with flags
/hold |
f812b02
to
9d54908
Compare
/unhold |
@@ -46,6 +46,7 @@ spec: | |||
args: | |||
- "/work" | |||
- "manager" | |||
- "--work-driver=kube" |
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.
add --cloudevents-client-id=$(POD_NAME)
?
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.
currently, we make the cloudevents-client-id
optional, it only need when we use cloudevents, current case is kube, we can omit it
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.
that makes sense!
/lgtm |
/hold the sdk-go has a bug, I will fix it, then update the sdk-go again |
b0278e4
to
81c5b40
Compare
Signed-off-by: Wei Liu <liuweixa@redhat.com>
/unhold |
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qiujian16, skeeey 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 |
b1b734a
into
open-cluster-management-io:main
Summary
Related issue(s)
Fixes #