Do not automatically check off closed deploy blockers

[mountiny]

Coming from this fireroom.

Problem

When the deploy blocker is automatically closed when PR fixing it linked with the fixes keyword, if the deployer or engineer who is handling the fix fails to CP the PR (forgets about it or it fails to build), then the deploy workflow will not stop them, because the closed deploy blocker is autochecked off and the flow is not looking for closed issues with DeployBlockerCash label.

Solution

When App or Mobile-Expensify PRs include the fixes <issue-link> format (or similar auto-closing formats), update the PR body with $ format to avoid the auto closure. Do this no matter what the format if the PR body is.

@Expensify/mobile-deployers Curious for your take.

Issue Owner

Current Issue Owner: @jnowakow

[melvin-bot]

Triggered auto assignment to @strepanier03 (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[luacmartins]

Sounds like a good improvement. 👍

[thienlnam]

I imagine if a deploy blocker gets closed, we should consider it "done" - could we just update the logic so it doesn't close issues with that deploy-blocker label?

[blimpich]

+1 to Jack's comment. Having to manually close the issue instead of manually removing the label makes more sense to me.

[marcaaron]

I don't really understand what the problem is. Could you rephrase it as a sequence of events?

When the deploy blocker is automatically closed when PR fixing it linked with the fixes keyword

I would imagine that this only closes the deploy blocker issue when the PR is deployed. What happens today?

[marcaaron]

deployer or engineer has to consciously take the label off

I do this for any issues I'm actively managing. But seems like it would add more mental load for the deployer when engineers who are assigned to deploy blockers fail to remove the label.

[roryabraham]

FWIW, we already have a webhook comment that warns you not to use the fixes keyword (or other equivalent) in PR descriptions when you open them. It also breaks the BZ lifecycle automations for the issue. My vote would be to just trust that comment to usually work and :donothing: here.

[blimpich]

It didn't work here though, and we had a fire. So the webhook comment wasn't sufficient in this case, right?

[roryabraham]

Equivalently, we've had this behavior for years with thousands of blockers and as far as we know it's only been a problem once, meaning it works 99.99% of the time, which is pretty good.

[roryabraham]

If we do want an automated solution to reduce the likelihood of human error, I'd propose something different:

---

Problem

When the deploy blocker is automatically closed when PR fixing it linked with the fixes keyword, if the deployer or engineer who is handling the fix fails to CP the PR (forgets about it or it fails to build), then the deploy workflow will not stop them, because the closed deploy blocker is auto-checked off and the flow is not looking for closed issues with DeployBlockerCash label.

Solution

Whenever an E/App issue is automatically closed by the fixes keyword, check if it was closed by PR or commit, and if so, reopen it and reapply the DeployBlockerCash, Hourly labels if the issue previously had them.

This solves the problem by ensuring that the issue remains an open hourly deploy blocker, and ensures it will not be auto-checked-off the checklist in error. It also ensures that the bugzero lifecycle of the issue (such as C+ payment and BZ checklist) happen as expected.

---

Though again, my gut instinct is that this is more work than it's worth, and we should :donothing: and instead #focus on higher value work.

[arosiclair]

Is this something we can actually change? I would've guessed auto checking GH issues in lists was a feature that can't be turned off. I also feel like it doesn't work consistently 🤔

Either way, I agree with Jack - if an issue is closed that should mean it's done. If an issue gets closed prematurely, I think it's the the assignee's responsibility to re-open it (we should get notifications when this happens). I think not auto-closing deploy blockers is a decent safety measure, but doing nothing is also fine by me.