Help Wanted: Document what to do if a merged PR has a bug that was missed during PR reviews

[matterdoc]

Mattermost user jason.blais from https://community-release.mattermost.com has requested the following be documented:

If we were to find something wrong with a merged PR (made by us), what is the standard procedure? Create a new issue and new PR to solve the problem? Somehow reopen the PR?

Answer: I'd recommend creating a GitHub issue + PR that fixes the issue. If it's a major problem, then reverting the changes instead (and reopening a PR with the fix) is the safer approach.

See the original post here.

This issue was generated from Mattermost using the Doc Up plugin.

[TomerPacific]

@cwarnermm , I'd like to help out here. What would you like to be done exactly?

[cwarnermm]

@jasonblais - I realize that you posted this a LONG time ago, but any guidance and advice you could offer for this ticket would be very much appreciated!

[jasonblais]

Hi @cwarnermm and thanks @TomerPacific for your interest helping :)

Scenario: A contributor (either staff or community member) submits a PR, it is reviewed and merged into the codebase.

Few days later the community notices a mistake with the PR. The question is, what should the standard procedure be? Few options include:

• creating a new issue for the identified issue, and submitting a new PR to solve it
• reverting the PR
• something else?

Proposed change:

Document the proposed procedure. This may depend on the size of the changes made, or how long it took to find the mistake. E.g. if it's a small code change and the issue is found a day later, reverting the change may be easiest.

As a note, given this issue is from a long time, we may have already documented the procedure, or there may be an undocumented procedure that we follow. It may be good @cwarnermm to connect @TomerPacific with a couple of the staff engineers if either is the case.

Let me know if this helps with clarification :)

[TomerPacific]

Thanks @jasonblais for the added clarification. @cwarnermm , would you like me to wait for you to contact several staff engineers to understand if there is a procedure that has become common practice?

[cwarnermm]

Thanks so much, @jasonblais! Really appreciate the context here and I agree that this may already be documented (or understood).

@TomerPacific - Really appreciate your willingness to help us with this! Let's see if we can get some confirmation on whether this process has been noted and proceed from there.

@m1lt0n @Willyfrog - Can either of you confirm whether we have processes documented somewhere for how to address situations that Jason describes above? If it is documented, can you point me to where I could find it, please?

[Willyfrog]

I've never seen such documentation (that doesn't mean it doesn't exist) but the usual way to go for this depends on the impact:

• low impact (some non-critical functionality affected): create a ticket and resolve it based on priority. Usually assigned to the same person who introduced the bug if still available (sometimes we detect the bug way later)
• high impact: two scenarios:
  • if the feature is behind a feature flag and it is not affecting other functionality: turn FF off.
  • any other case: revert the PR and let the original person know so they can work on it.

[m1lt0n]

Also on my side I haven't seen such documentation, but I agree with @Willyfrog :

1. Low impact, it can be a follow-up PR that fixes the wrong behaviour (if it doesn't affect end users in any significant way, eg some small styling issue or something)
2. For high impact, people are going in the two ways that Guillermo mentions. My take on this is to always opt in for a revert immediately when there is an incident (so, it should qualify as an incident) and work on a proper fix and reintroduce the change (and have it go through the process of reviewing/QA review etc) with the fix in place.

[cwarnermm]

Thank you @Willyfrog and @m1lt0n for your thoughtful input.

@TomerPacific - This information would fit well in the Developer documentation under the Contribute code section. Pages on this site are formatted in Markdown.

If you don't see an existing page to update with these process details, feel free to create a PR that introduces a new page, and we'll help you get it published.

[TomerPacific]

Thanks @Willyfrog , @m1lt0n and @cwarnermm. I'll start working on this.

[TomerPacific]

@cwarnermm - I have opened a PR and I am awaiting your feedback.