Skip to content

fix(docker images): only notify when images build is for automated-commits,master,main #354

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

Open
zaquestion wants to merge 1 commit into
base: master
Choose a base branch
from
+7 −5
Open

fix(docker images): only notify when images build is for automated-commits,master,main #354

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 7 additions & 5 deletions jobs/update-devserver-static-images.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,11 +94,13 @@ def publishResults() {
// that it can bring down our server! See
// https://khanacademy.slack.com/archives/C096UP7D0/p1738681917185069
onWorker('build-worker', '10h') {
notify([slack: [channel: params.SLACK_CHANNEL,
failureChannel: "#local-devserver",
sender: 'Devserver Duck',
emoji: ':duck:',
when: ['SUCCESS', 'FAILURE', 'UNSTABLE', 'ABORTED']]]) {
def notifyParams = params.GIT_BRANCH == "automated-commits" || params.GIT_BRANCH == "master" || params.GIT_BRANCH == "main" ?
Copy link
Copy Markdown
Member

@csilvers csilvers Oct 10, 2025

Choose a reason for hiding this comment

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

I think this is a good idea, but I wonder if instance of just suppressing the failure (or success) message when folks run this manually, we should send the message somewhere else. Like we could send it to the slack channel of the person who send the message. (Can bots send to individual people, or just to channels? I don't actually know.)

We could do this in a few ways:

  1. Have the failure-channel be a parameter to the job, and just tell people to change it when manually deploying. And maybe fail the job if they select #local-devserver for a non-master branch, to help them remember :-)
  2. Hard-code a failure-channel that we use for non-master.

We can start with (2) and improve from there, something like:

Suggested change
def notifyParams = params.GIT_BRANCH == "automated-commits" || params.GIT_BRANCH == "master" || params.GIT_BRANCH == "main" ?
# When manually run, we want to limit who is notified.
# TODO(zaquestion): find a better target than #bot-testing.
# Maybe send directly to the person running the job?
def failureChannel = params.GIT_BRANCH == "automated-commits" || params.GIT_BRANCH == "master" || params.GIT_BRANCH == "main" ? "#local-devserver": "#bot-testing";
notify([slack: [channel: params.SLACK_CHANNEL,
failureChannel: failureChannel,
sender: 'Devserver Duck',
emoji: ':duck:',
when: ['SUCCESS', 'FAILURE', 'UNSTABLE', 'ABORTED']]]) {

Copy link
Copy Markdown
Contributor Author

@zaquestion zaquestion Oct 10, 2025

Choose a reason for hiding this comment

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

If the goal is to notify the person who started the 1 off job, I'd think that they could see that on the job itself? Or observe that their branch didn't get the expected update to the image sha. It seems like a reasonable expectation for manual runs to observe the job themselves. These notifications always seemed most useful for the cron/recurring scenarios where a human isn't involved at the start.

I'm a little resistant to send to #bot-testing mostly because it just seems like it will be new noise, and I'd feel obligated to socialize it despite mostly feeling they should monitor in jenkins not slack. If you feel strongly, and the above changes are what you need to move forward, I'm open to that path. I don't feel that strongly about resisting, so I'm just voicing my perspective. Let me know where you're landing after hearing these thoughts.

Copy link
Copy Markdown
Member

@csilvers csilvers Oct 11, 2025

Choose a reason for hiding this comment

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

The issue is there's a long time between when you start the job and it finishes, so people will have to remember to come back. And this isn't something folks do often, so the more manual process we put in, the less likely it is people will get it right. I think folks would definitely appreciate getting a notification of how it went, but we could ask some people who have run this lately, to see what they think.

It sounds like a slack DM to the person who started the job would be ideal. But I don't think we're set up to do that.

I don't feel strongly enough about this to block on it, and I'm out tuesday, so I'll approve. But I do think there should be a TODO to do some sort of notification in the not-master case.

[slack: [channel: params.SLACK_CHANNEL,
failureChannel: "#local-devserver",
sender: 'Devserver Duck',
emoji: ':duck:',
when: ['SUCCESS', 'FAILURE', 'UNSTABLE', 'ABORTED']]] : [:];
notify(notifyParams) {
stage("Setup webapp") {
_setupWebapp();
}
Expand Down