-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Only trigger deploy blocker notifications on issues #4707
Conversation
nab: If I'm understanding correctly, we are not listening anymore to events related to pull request, maybe we should comment or remove the lines handling these events: App/.github/workflows/deployBlocker.yml Lines 26 to 31 in 8dc03e3
The may confuse another developer into thinking that this is still processing pull requests. Besides that, I don't know how to test this safely. The modified lines make sense to me, but I guess this should be also reviewed by someone else :) |
Nice find @aldo-expensify! The linked code is actually dead and I will update to remove it. |
@AndrewGable I'll hold off on reviewing until you've removed the dead code. Just ping me when it's ready! |
Updated |
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.
Looking good to me!
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by @roryabraham in version: 1.0.88-3 🚀
|
Going to use this PR and the linked issue to test this. |
Yep, that didn't do anything, which is what we want. We could maybe remove the label and leave a comment to explain that only issues (not pull requests) can be deploy blockers. But we don't have to – that's a nice-to-have. What do y'all think? |
This worked for issues: https://expensify.slack.com/archives/C01GTK53T8Q/p1630438694222000 So this is a QA-pass 🎉 |
🚀 Deployed to production by @roryabraham in version: 1.0.90-2 🚀
|
Details
We were triggering the deploy blocker notifications on PRs which could include forks, which was causing issues here: https://github.com/Expensify/App/actions/runs/1140938643
Tests
DeployBlockerCash
label to a PR, verify it does not ping in SlackDeployBlockerCash
label to a issue, verify it does ping in Slack