-
Notifications
You must be signed in to change notification settings - Fork 0
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.
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
base: master
Are you sure you want to change the base?
Conversation
…mmits,master,main to reduce the noise of developers running the job on review/deploy branches. Included main just for good measure, even those its not a branch for webapp
csilvers
left a comment
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'm Requesting Changes because I think folks will want to know if the job succeeded or failed, as part of their testing for the product they're working on. I'm agnostic exactly how that is achieved, I put in a suggestion below for something that's simple to implement, but we could maybe do something better without too much effort.
As for how to test: you can go to a previous jenkins job that used a personal branch, and hit "replay", and replace the main groovy script (in the first textbox) with your new code. If you're lucky the build will fail and you'll see the message show up where expected! But honestly I think as long as this compiles you're fine. Not that it's so easy to test that either. You could try putting it into a groovy sandbox and seeing if the sandbox complains about the syntax.
| 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" ? |
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 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:
- 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-devserverfor a non-master branch, to help them remember :-) - Hard-code a failure-channel that we use for non-master.
We can start with (2) and improve from there, something like:
| 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']]]) { |
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.
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.
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.
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.
Summary:
to reduce the noise of developers running the job on review/deploy branches. Included main just for good measure, even those its not a branch for webapp
Test plan:
Confession: I don't know how to test this...