feat: make slack notification explictly opt in#60
Conversation
metalwarrior665
left a comment
There was a problem hiding this comment.
Looks good, just small nits
bin/main.ts
Outdated
There was a problem hiding this comment.
Why not just like this? (just format it after me :) ) Not sure why we would log error if we choose to not notify.
Suggested change
if (notifySlack) {
await notifyToSlack({
changedFiles,
commits,
changelog,
repository,
dryRun,
author,
});
};
There was a problem hiding this comment.
I thought that since not sending the notification is the default, having a message that texplicitly says that you can actually send the notification can be informative, for either people that do not know how to set it up and people who simply forgot to add the flag
bin/test-report.ts
Outdated
| console.error('SLACK:', slackMessage); | ||
| console.error('\tblocks:', blocks.join('\n\t\t')); | ||
|
|
||
| if (!notifySlack) { |
There was a problem hiding this comment.
I would move this above the channel selection as it is part of the slack logic
There was a problem hiding this comment.
Hm, I wanted to leave this in because I liked the message, but I guess the diff will be enough to evaluate the results 👍
bin/main.ts
Outdated
| (args) => | ||
| args | ||
| .option('report-file', { type: 'string', demandOption: true }) | ||
| .option('notify-slack', { type: 'boolean', default: false }) |
There was a problem hiding this comment.
If we would want to make this more usable outside of our team, we would probably need to make the Slack channels configurable etc. but for now this will do
There was a problem hiding this comment.
since this is already a breaking change, what if we added now 2 new cli arguments for defining slack channels instead of this flag?
So instead of
apify-test-tools --notify-slackwe'd have
apify-test-tools --report-slack-channel '#notif-google-maps' --release-slack-channel '#delivery-public-actor'#delivery-public-actorcould be a variable at the org level#notif-google-mapswe'd take it from the env: `"#notif-$(echo $GITHUB_REPOSITORY | cut -d '/' -f 2)"- use
catto remove theapify-store/part
- use
In the GH workflow file, we would have:
npx apify-test-tools release --push-event-path ${{ github.event_path }} --release-slack-channel ${{ var.release_slack_channel }} --report-slack-channel "#notif-$(echo $GITHUB_REPOSITORY | cut -d "/" -f 2)"There was a problem hiding this comment.
I wouldn't worry about breaking changes, this is used only with us and we control https://github.com/apify-store/github-actions-source :)
Feel free to go with the suggested change although I don't think it is really needed at this point. But it would be good to eventually look at the whole thing and try to restructure it so it is more generally usable. Like now it would still rely on the slack tokens in env.
There was a problem hiding this comment.
Personally I like passing the channel name instead of a notify-slack flag which feels very generic. That being said, I did the changes but I can just revert them if you think there is too much stuff going on for now :)
metalwarrior665
left a comment
There was a problem hiding this comment.
Looks correct to me. We should figure out how to test this in the wild, probably manually adjust the beta version https://github.com/apify-store/testing-repo-for-github-actions and in the actions make sure we don't override the beta
|
@ruocco-l Here is how to test it after you merge.
If you have idea how to automate this whole thing, that would be golden :D (AI might know) |
Closes #55
Now
report-testsandreleasecommands must include--notify-slackor it will not trigger the notify notification, but just show the failed errors in the terminal.