Self review option for merge script #3650
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
HebaruSan
added
Enhancement
DasSkelett
approved these changes
Aug 29, 2022
A further suggestion for the scope of acceptable usage of this option: it should probably also be limited to fixes of unreleased bugs, when we've broken something that was working in the latest release (and therefore the correct behavior is trivial to define and confirm). I think my usages have all been along those lines. Maybe eventually we can indicate that with GitHub labels, so the |
|
That sounds useful and appropriate. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
For a very small, uncontroversial fix like #3649, it may make sense to submit a pull request with documentation of the bug and the fix (so team members know what happened), but then merge it without review, to keep the
masterbranch working.Currently the merge script from #3263 can't do this because it absolutely requires approval by at least one reviewer. I've been making a live temporary edit to add
False andto bypass that check as needed, which is not a great solution.Changes
Now the merge script has a
--self-reviewoption that skips the approval check if present. The default isFalse(obviously). This should only be used for small fixes in which we have high confidence. This has already been tested in the merge of #3649.