-
Notifications
You must be signed in to change notification settings - Fork 893
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
Reorganize events code and add execution event #800
Conversation
ecac827
to
98adeb4
Compare
/ping @RainbowMango |
/lgtm |
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.
Thanks for doing this.
Only some nits.
I'll re-organize the event to pkg/apis/
after this PR.
pkg/util/constants.go
Outdated
|
||
// Define event type used by karmada system. | ||
const ( | ||
//EventReasonCleanupWorkFailed indicates that Cleanup work failed. |
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.
It'd be better if comments start with a whitespace. like:
//EventReasonCleanupWorkFailed indicates that Cleanup work failed. | |
// EventReasonCleanupWorkFailed indicates that Cleanup work failed. |
pkg/util/constants.go
Outdated
EventReasonSyncWorkSucceed = "SyncWorkSucceed" | ||
//EventReasonAggregateStatusFailed indicates that aggregate status failed. | ||
EventReasonAggregateStatusFailed = "AggregateStatusFailed" | ||
//EventReasonAggregateStatusFailed indicates that aggregate status 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.
I don't know why golint
missed this check.
//EventReasonAggregateStatusFailed indicates that aggregate status succeed. | |
// EventReasonAggregateStatusSucceed indicates that aggregate status succeeds. |
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 will check later why golint
missed these checks
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.
It's not a big deal. Just thought weird.
By the way, we have deprecated golint
by #806 and using revive
now.
klog.Errorf("Stop sync work(%s/%s) for cluster(%s) as cluster not ready.", work.Namespace, work.Name, cluster.Name) | ||
msg := fmt.Sprintf("Stop sync work(%s/%s) for cluster(%s) as cluster not ready.", work.Namespace, work.Name, cluster.Name) | ||
klog.Errorf(msg) | ||
c.EventRecorder.Event(work, corev1.EventTypeNormal, util.EventReasonSyncWorkFailed, msg) |
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 guess we can define a separated event for Work
(not reuse RB/CRB).
like EventReasonApplyWorkFailed = ApplyFailed
/assign |
I’m thinking the way organize the events:
In the events.go file, the events sorted by types:
@wawa0210 Just echo my thought here, I'm ok if you do it in this PR. |
98adeb4
to
ada5ca6
Compare
5fadd11
to
4119e1d
Compare
pkg/apis/work/v1alpha2/events.go
Outdated
EventReasonSyncWorkSucceed = "SyncWorkSucceed" | ||
//EventReasonAggregateStatusFailed indicates that aggregate status failed. | ||
EventReasonAggregateStatusFailed = "AggregateStatusFailed" | ||
//EventReasonAggregateStatusFailed indicates that aggregate status 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.
//EventReasonAggregateStatusFailed indicates that aggregate status succeed. | |
// EventReasonAggregateStatusSucceed indicates that aggregate status 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.
Also, comments should start with a whitespace.
klog.Errorf("Stop sync work(%s/%s) for cluster(%s) as cluster not ready.", work.Namespace, work.Name, cluster.Name) | ||
msg := fmt.Sprintf("Stop sync work(%s/%s) for cluster(%s) as cluster not ready.", work.Namespace, work.Name, cluster.Name) | ||
klog.Errorf(msg) | ||
c.EventRecorder.Event(work, corev1.EventTypeNormal, workv1alpha1.EventReasonSyncWorkFailed, msg) |
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.
Here if cluster not ready
means the binding hasn't been scheduled yet, nothing to do with a failed
.
I don't think we need an event here.
klog.Errorf("Failed to sync work(%s) to cluster(%s): %v", work.Name, cluster.Name, err) | ||
msg := fmt.Sprintf("Failed to sync work(%s) to cluster(%s): %v", work.Name, cluster.Name, err) | ||
klog.Errorf(msg) | ||
c.EventRecorder.Event(work, corev1.EventTypeNormal, workv1alpha1.EventReasonSyncWorkFailed, msg) |
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.
corev1.EventTypeNormal
??
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.
updated
Please remove |
4119e1d
to
e79540b
Compare
e79540b
to
757e13d
Compare
pkg/apis/work/v1alpha2/events.go
Outdated
EventReasonSyncWorkSucceed = "SyncWorkSucceed" | ||
// EventReasonAggregateStatusFailed indicates that aggregate status failed. | ||
EventReasonAggregateStatusFailed = "AggregateStatusFailed" | ||
// EventReasonAggregateStatusFailed indicates that aggregate status 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.
The comment...
// EventReasonAggregateStatusFailed indicates that aggregate status succeed. | |
// EventReasonAggregateStatusSucceed indicates that aggregate status 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.
done
Signed-off-by: wawa0210 <[email protected]>
757e13d
to
b247085
Compare
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
/approve
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: RainbowMango 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 |
Signed-off-by: wawa0210 [email protected]
What type of PR is this?
/kind feature
What this PR does / why we need it:
Part of #398
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: