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.

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

Feature/improve rc notif 177492071 #5

Merged
merged 10 commits into from
Apr 2, 2021

Conversation

Treyone
Copy link
Contributor

@Treyone Treyone commented Apr 1, 2021

PT

#177492071

Description

As discussed we want to publish 2 types of information in the #release-candidate Slack channel.

1. Notice that a new version is available on dev
The notification should be sent after the check-version job, to ensure that dev as been upgraded to the new version.

Message should be something like:

New build available on `https://dev.stoic.com/` (version 1.2.3)

2. Notice of what was merged on dev
Here ideally we want one notification by branch that was merged.
For instance if we have a PR on 3 repos, as soon as one repo is merged (usually all of them should be merged pretty much at the same time), we send a notification. We don't send a new notification for the other 2 repos, so we don't duplicate the information (in this channel what is important is to understand the the work that was delivered).

Example message

<icon> <feature|bug|chore> merged from <repo 1, repo 2, ...> repos.
[PT title](PT link)

The PT id comes from the branch name (we ignore if the PR was about multiple PTs).

@Treyone Treyone requested a review from AurelienRibon April 1, 2021 14:14
@Treyone Treyone merged commit 000b45e into master Apr 2, 2021
Copy link
Member

@AurelienRibon AurelienRibon left a comment

Choose a reason for hiding this comment

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

Awesome!! It's a pleasure to add handlers to the bot!

src/civ2.js Show resolved Hide resolved
default: '#testing-ci',
releaseCandidates: '#release-candidates'
};
const MAX_PRDATE = 1000 * 60 * 5;
Copy link
Member

Choose a reason for hiding this comment

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

MAX_PR_DATE would be easier to read, I thought it was a typo for MAX_UPDATE at first :)
Also, don't use DATE as suffix, prefer the real unit for the constant. MINS? SECS? It would document the computation.

src/civ2.js Show resolved Hide resolved
src/civ2.js Show resolved Hide resolved
src/civ2.js Show resolved Hide resolved
test/civ2.test.js Show resolved Hide resolved
test/civ2.test.js Show resolved Hide resolved
test/civ2.test.js Show resolved Hide resolved
[{ method: 'reply', args: ['Destruction in progress.'] }]
));

it('should notify in an error occurs', async () =>
Copy link
Member

Choose a reason for hiding this comment

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

You should add a curly bracket at the start of the arrow. It would bring more consistency to this file, and it's often easier to read mocha tests when they all use blocks, as our eyes are very used to this pattern.

test/civ2.test.js Show resolved Hide resolved
@Treyone Treyone deleted the feature/improve-rc-notif-177492071 branch April 2, 2021 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants