-
-
Notifications
You must be signed in to change notification settings - Fork 229
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
✨(deploy) slack notify authors of deployments #3101
Conversation
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
8b43774
to
7ce27c0
Compare
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 couldn't test it on staging because of an error (perhaps it needs to be rebased or the staging server needs to be restarted?). I couldn't make it work on local, but my setup is a bit screwed right now.
The code looks good though! I'm just wondering why don't you send the notification directly (since you already have a connection to slack web api) and instead go through buildkite? (I'm not saying it's a bad thing, you must have good reasons to do it like that)
(On a side note, it'd be nice to have slack info in users table so that we don't have to do the conversion. But it's obviously out of scope and not a priority.)
@Marigold thanks for the review!
💯
Yes it's true that using the slack API in two different places seems a bit redundant. I prefer going through BK because it makes the pipeline more self-contained. It also makes dealing with errors in this side-effect straightforward: we see them straightaway in context, for free (no additional reporting to set up, and no need to worry about what to report, and what it means for the rest of the code execution). Another reason was reusability: we might want to notify through slack from other pipelines, and it's convenient to just grap that one-liner and paste it in. When and if we do save slack IDs in the DB, the reliance on the slack API at bake time would disappear, which might make the slacktee approach even more palatable.
I rebuilt the server and set up the relevant Note that lightning content updates are currently broken, so they will appear as regular full bakes but support for lightning bakes is there: ![]() |
Thanks for the explanation; agree with all your points. |
Send Slack notifications when a new content deployment finishes.
To trigger a new notification locally:
BUILDKITE_DEPLOY_CONTENT_PIPELINE_SLUG=testing-notify-authors-on-slack
BUILDKITE_API_ACCESS_TOKEN
yarn startDeployQueueServer
This relies on:
owid-lxc create -t owid-cloudflare-prod matthieu-slack
, after replacing the deploy queuecloudflare deploy
withnotify-slack
(see test pipeline)