-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat(1572): [2] Add FIXED status to the build #30
Conversation
index.js
Outdated
@@ -147,11 +155,20 @@ class SlackNotifier extends NotificationBase { | |||
buildData.event.commit.message; | |||
const isMinimized = buildData.settings.slack.minimized; | |||
|
|||
// When using multiple notification plugins,Do not change the `buildData.status` directly |
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.
Let me suggest.
// When using multiple notification plugins,Do not change the `buildData.status` directly | |
// Do not change the `buildData.status` directly. | |
// It affects the behavior of other notification plugins. |
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.
I fixed it
@@ -136,6 +139,11 @@ class SlackNotifier extends NotificationBase { | |||
buildData.settings.slack.statuses = DEFAULT_STATUSES; | |||
} | |||
|
|||
// Add for fixed notification |
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.
Is it nicer?
if (buildData.isFixed) {
buildData.settings.slack.statuses.push('FIXED');
}
let notificationStatus = buildData.status;
if (buildData.settings.slack.statuses.includes('FAILURE')) {
if (notificationStatus === 'SUCCESS' && buildData.isFixed) {
notificationStatus = 'FIXED';
}
}
if (!buildData.settings.slack.statuses.includes(notificationStatus)) {
return;
}
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 looks good!
We'll modify it that way.
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 for fixing. Is there any problem on updating with buildData.settings.slack.statuses.push('FIXED');
for multiple plugins? I think it is OK if it does not update DB.
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 okay because it's only slack that's set up.
buildData.settings.[slack].statuses
For example, for the email plugin, this is set as follows
buildData.settings.[email].statuses
There is no process to update the DB in each notification plugin, so I think okay.
Context
feat: screwdriver-cd/screwdriver#1572
Objective
This PR adds the status "FIXED" when a build has failed in the past and the build succeeds again.
Notification-slack-plugin receives a build status label (
isFIxed
) from screwdriver api.Therefore, The "FIXED" status uses the same
COLOR_MAP
andSTATUSES_MAP
as the "SUCCESS" status.In order for users to receive FIXED notifications, "FAILURE" must be set to screwdriver.yaml.
The image below is a FIXED notification sent by slack.
References
issue: screwdriver-cd/screwdriver#1572
[1] notifications-email screwdriver-cd/notifications-email#22
[3] screwdriver-api screwdriver-cd/screwdriver#2182
[4] guide screwdriver-cd/guide#407
License
I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.