-
Notifications
You must be signed in to change notification settings - Fork 156
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
Send notification about stage executions #5440
Conversation
Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>
Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>
Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>
Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>
Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>
Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>
Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5440 +/- ##
==========================================
- Coverage 26.24% 26.19% -0.05%
==========================================
Files 452 464 +12
Lines 48860 49815 +955
==========================================
+ Hits 12821 13049 +228
- Misses 35014 35733 +719
- Partials 1025 1033 +8 ☔ View full report in Codecov by Sentry. |
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
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 seems great!
I left a refactoring comment.
s.notifier.Notify(model.NotificationEvent{ | ||
Type: model.NotificationEventType_EVENT_STAGE_STARTED, | ||
Metadata: &model.NotificationEventStageStarted{ | ||
Deployment: s.deployment, | ||
Stage: ps, | ||
}, | ||
}) | ||
|
||
result = s.executeStage(sig, *ps, func(in executor.Input) (executor.Executor, bool) { | ||
return s.executorRegistry.Executor(model.Stage(ps.Name), in) | ||
}) | ||
|
||
switch result { | ||
case model.StageStatus_STAGE_SUCCESS, model.StageStatus_STAGE_EXITED: // Exit stage is treated as success. | ||
s.notifier.Notify(model.NotificationEvent{ | ||
Type: model.NotificationEventType_EVENT_STAGE_SUCCEEDED, | ||
Metadata: &model.NotificationEventStageSucceeded{ | ||
Deployment: s.deployment, | ||
Stage: ps, | ||
}, | ||
}) | ||
case model.StageStatus_STAGE_FAILURE: | ||
s.notifier.Notify(model.NotificationEvent{ | ||
Type: model.NotificationEventType_EVENT_STAGE_FAILED, | ||
Metadata: &model.NotificationEventStageFailed{ | ||
Deployment: s.deployment, | ||
Stage: ps, | ||
}, | ||
}) | ||
case model.StageStatus_STAGE_CANCELLED: | ||
s.notifier.Notify(model.NotificationEvent{ | ||
Type: model.NotificationEventType_EVENT_STAGE_CANCELLED, | ||
Metadata: &model.NotificationEventStageCancelled{ | ||
Deployment: s.deployment, | ||
Stage: ps, | ||
}, | ||
}) | ||
case model.StageStatus_STAGE_SKIPPED: | ||
s.notifier.Notify(model.NotificationEvent{ | ||
Type: model.NotificationEventType_EVENT_STAGE_SKIPPED, | ||
Metadata: &model.NotificationEventStageSkipped{ | ||
Deployment: s.deployment, | ||
Stage: ps, | ||
}, | ||
}) | ||
} | ||
|
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.
[IMO] How about extracting into a method?
I just want to simplify scheduler.Run()
for readability and maintenanceability.
For example:
s.notifier.Notify(model.NotificationEvent{ | |
Type: model.NotificationEventType_EVENT_STAGE_STARTED, | |
Metadata: &model.NotificationEventStageStarted{ | |
Deployment: s.deployment, | |
Stage: ps, | |
}, | |
}) | |
result = s.executeStage(sig, *ps, func(in executor.Input) (executor.Executor, bool) { | |
return s.executorRegistry.Executor(model.Stage(ps.Name), in) | |
}) | |
switch result { | |
case model.StageStatus_STAGE_SUCCESS, model.StageStatus_STAGE_EXITED: // Exit stage is treated as success. | |
s.notifier.Notify(model.NotificationEvent{ | |
Type: model.NotificationEventType_EVENT_STAGE_SUCCEEDED, | |
Metadata: &model.NotificationEventStageSucceeded{ | |
Deployment: s.deployment, | |
Stage: ps, | |
}, | |
}) | |
case model.StageStatus_STAGE_FAILURE: | |
s.notifier.Notify(model.NotificationEvent{ | |
Type: model.NotificationEventType_EVENT_STAGE_FAILED, | |
Metadata: &model.NotificationEventStageFailed{ | |
Deployment: s.deployment, | |
Stage: ps, | |
}, | |
}) | |
case model.StageStatus_STAGE_CANCELLED: | |
s.notifier.Notify(model.NotificationEvent{ | |
Type: model.NotificationEventType_EVENT_STAGE_CANCELLED, | |
Metadata: &model.NotificationEventStageCancelled{ | |
Deployment: s.deployment, | |
Stage: ps, | |
}, | |
}) | |
case model.StageStatus_STAGE_SKIPPED: | |
s.notifier.Notify(model.NotificationEvent{ | |
Type: model.NotificationEventType_EVENT_STAGE_SKIPPED, | |
Metadata: &model.NotificationEventStageSkipped{ | |
Deployment: s.deployment, | |
Stage: ps, | |
}, | |
}) | |
} | |
s.notifyStageEvent(model.StageStatus_STAGE_STARTED, ps) | |
result = s.executeStage(sig, *ps, func(in executor.Input) (executor.Executor, bool) { | |
return s.executorRegistry.Executor(model.Stage(ps.Name), in) | |
}) | |
s.notifyStageEvent(result, ps) |
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.
Thank you!
It's nice, so I changed the codes on this commit.
e7bc90a
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.
Thank you!
StageStart and StageEnd are really good.
Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>
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
What this PR does:
I attached the screen capture of the Slack notification above, but this PR adds webhook notifications, too.
Why we need it:
To know the deployment progress systematically.
Which issue(s) this PR fixes:
Maybe fixes #5393
Does this PR introduce a user-facing change?: Yes