Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(1572): [2] Add FIXED status to the build #30

Merged
merged 7 commits into from
Aug 19, 2020
Merged

feat(1572): [2] Add FIXED status to the build #30

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6b4e6b8
feat: Fixed status
ibu1224 Aug 16, 2020
8e31a50
fix: tests and implementation
ibu1224 Aug 17, 2020
52248fa
fix
ibu1224 Aug 18, 2020
bb809eb
fix
ibu1224 Aug 18, 2020
92d0a94
fix lint
ibu1224 Aug 18, 2020
0b788ae
fix
ibu1224 Aug 19, 2020
ee743fd
fix
ibu1224 Aug 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 22 additions & 5 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ const COLOR_MAP = {
BLOCKED: '#ccc', // Using 'sd-light-gray' from https://github.com/screwdriver-cd/ui/blob/master/app/styles/screwdriver-colors.scss
UNSTABLE: '#ffd333', // Using 'sd-unstable' from https://github.com/screwdriver-cd/ui/blob/master/app/styles/screwdriver-colors.scss
COLLAPSED: '#f2f2f2', // New color. Light grey.
FIXED: 'good',
FROZEN: '#acd9ff' // Using 'sd-frozen' from https://github.com/screwdriver-cd/ui/blob/master/app/styles/screwdriver-colors.scss
};
const STATUSES_MAP = {
Expand All @@ -29,6 +30,7 @@ const STATUSES_MAP = {
BLOCKED: ':lock:',
UNSTABLE: ':foggy:',
COLLAPSED: ':arrow_up:',
FIXED: ':sunny:',
FROZEN: ':snowman:'
};
const DEFAULT_STATUSES = ['FAILURE'];
Expand Down Expand Up @@ -68,7 +70,8 @@ const SCHEMA_BUILD_DATA = Joi.object()
id: Joi.number().integer().required()
}).unknown(true),
event: Joi.object(),
buildLink: Joi.string()
buildLink: Joi.string(),
isFixed: Joi.boolean()
});
const SCHEMA_SLACK_CONFIG = Joi.object()
.keys({
Expand Down Expand Up @@ -136,6 +139,11 @@ class SlackNotifier extends NotificationBase {
buildData.settings.slack.statuses = DEFAULT_STATUSES;
}

// Add for fixed notification
Copy link
Contributor

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;
}

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

if (!buildData.settings.slack.statuses.includes('SUCCESS') && buildData.isFixed) {
buildData.settings.slack.statuses.push('SUCCESS');
}

if (!buildData.settings.slack.statuses.includes(buildData.status)) {
return;
}
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me suggest.

Suggested change
// 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.

Copy link
Contributor Author

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

let notificationStatus = buildData.status;

if (buildData.settings.slack.statuses.includes('FAILURE')) {
if (buildData.status === 'SUCCESS' && buildData.isFixed) {
notificationStatus = 'FIXED';
}
}

let message = isMinimized ?
// eslint-disable-next-line max-len
`<${pipelineLink}|${buildData.pipeline.scmRepo.name}#${buildData.jobName}> *${buildData.status}*` :
`<${pipelineLink}|${buildData.pipeline.scmRepo.name}#${buildData.jobName}> *${notificationStatus}*` :
// eslint-disable-next-line max-len
`*${buildData.status}* ${STATUSES_MAP[buildData.status]} <${pipelineLink}|${buildData.pipeline.scmRepo.name} ${buildData.jobName}>`;
`*${notificationStatus}* ${STATUSES_MAP[notificationStatus]} <${pipelineLink}|${buildData.pipeline.scmRepo.name} ${buildData.jobName}>`;

const metaMessage = hoek.reach(buildData,
'build.meta.notification.slack.message', { default: false });
Expand All @@ -170,7 +187,7 @@ class SlackNotifier extends NotificationBase {
[
{
fallback: '',
color: COLOR_MAP[buildData.status],
color: COLOR_MAP[notificationStatus],
fields: [
{
title: 'Build',
Expand All @@ -183,7 +200,7 @@ class SlackNotifier extends NotificationBase {
[
{
fallback: '',
color: COLOR_MAP[buildData.status],
color: COLOR_MAP[notificationStatus],
title: `#${buildData.build.id}`,
title_link: `${buildData.buildLink}`,
// eslint-disable-next-line max-len
Expand Down
16 changes: 15 additions & 1 deletion test/index.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,8 @@ describe('index', () => {
},
sha: '1234567890abcdeffedcba098765432100000000'
},
buildLink: 'http://thisisaSDtest.com/pipelines/12/builds/1234'
buildLink: 'http://thisisaSDtest.com/pipelines/12/builds/1234',
isFixed: false
};
notifier = new SlackNotifier(configMock);
});
Expand All @@ -107,6 +108,19 @@ describe('index', () => {
});
});

it('when the build status is fixed, Overwrites the notification status title', (done) => {
serverMock.event(eventMock);
serverMock.events.on(eventMock, data => notifier.notify(data));
buildDataMock.isFixed = true;
serverMock.events.emit(eventMock, buildDataMock);

process.nextTick(() => {
assert.calledWith(WebClientConstructorMock.WebClient, configMock.token);
assert.calledThrice(WebClientMock.chat.postMessage);
done();
});
});

it('verifies that non-included status does not send a notification', (done) => {
const buildDataMockUnincluded = {
settings: {
Expand Down