feat(#896): store notification status in workflowrun #897
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Assign the PR to them by writing 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 |
|
LGTM |
| // sendNotifications send notifications for workflowruns when: | ||
| // * its workflow has notification config | ||
| // * finish time after workflow controller starts | ||
| // * notification status of workflowrun is not nil |
There was a problem hiding this comment.
notification status of workflowrun is nil ?
| // * notification status of workflowrun is not nil | ||
| // If the returned notification status is nil, it means that there is no need to send notification. | ||
| func (h *Handler) sendNotifications(wfr *v1alpha1.WorkflowRun) (map[string]v1alpha1.NotificationStatus, error) { | ||
| // No need to send notifications for workflowruns finished before workflow controller starts. |
There was a problem hiding this comment.
Doesn't describe all the conditions in the if clause.
There was a problem hiding this comment.
Remove this, have included in method comments.
| return err | ||
| } | ||
|
|
||
| if !reflect.DeepEqual(latest.Status.Notifications, status) { |
There was a problem hiding this comment.
Does latest.Status.Notifications == nil suffices ?
| // If the returned notification status is nil, it means that there is no need to send notification. | ||
| func (h *Handler) sendNotifications(wfr *v1alpha1.WorkflowRun) (map[string]v1alpha1.NotificationStatus, error) { | ||
| // No need to send notifications for workflowruns finished before workflow controller starts. | ||
| if wfr.Spec.WorkflowRef == nil || wfr.Status.Notifications != nil || |
There was a problem hiding this comment.
wfr.Spec.WorkflowRef == nil is an abnormal condition, I think we should report error instead of keep silent
|
/lgtm |
ab38b82 to
bcd9ea7
Compare
|
/lgtm |
What this PR does / why we need it:
Add your description
Which issue(s) this PR is related to (optional, link to 3rd issue(s)):
Fixes #896
Reference to #
Special notes for your reviewer:
/cc @zhujian7 @cd1989
Release note: