Changed merge policy #6050
Changed merge policy #6050
Conversation
|
Just to be clear, are we saying that merges are now a 24/5 free-for all with no core hours, as long as they've simmered for 2 hours? Or that people can only merge when it's daylight relative to them? (Which is super confusing because it's 6am here and core hours in Poland right now.) |
|
(The "business day" language threw me off. I'm assuming that just means M-F and isn't related to hours.) |
|
@zmerlynn In my opinion the first option is better as long as merger will be available for some time to fix possible problem. Let's see what other folks think. "business day" means M-F excepting holidays. If you think there is a better word, I won't argue. |
|
Nah, I'm happy with 24/5. Y'all have also been pretty carefully watching Business Day is fine. "Monday-Friday" would be precise and simple, too.
|
| ## Code reviews | ||
|
|
||
| All changes must be code reviewed. For non-maintainers this is obvious, since you can't commit anyway. But even for maintainers, we want all changes to get at least one review, preferably from someone who knows the areas the change touches. For non-trivial changes we may want two reviewers. The primary reviewer will make this decision and nominate a second reviewer, if needed. Except for trivial changes, PRs should sit for at least 2 hours to allow for wider review. | ||
| All changes must be code reviewed. For non-maintainers this is obvious, since you can't commit anyway. But even for maintainers, we want all changes to get at least one review, preferably (for non-trivial changes obligately) from someone who knows the areas the change touches. For non-trivial changes we may want two reviewers. The primary reviewer will make this decision and nominate a second reviewer, if needed. Except for trivial changes, PRs should sit for at least 2 hours to allow for wider review. |
There was a problem hiding this comment.
Propose to strike the "at least 2 hours" language and replace with
Except for trivial changes, PRs should not be committed until relevant parties (e.g. owners of the subsystem affected by the PR) have had a reasonable chance to review them.
|
Let's de-emphasize exact times and instead focus on intent - relevant parties must sign off on important PRs. The less complex the PR, the less the rules matter. Nobody should be merging on their own nights and weekends. Nobody should be merging without giving people a reasonable shot at review. |
|
Yeah, I'd summarize the intent "Relevant parties should have the opportunity to look at a PR in their local business hours" or some such. The goal is that no one feel they need to check the open PR list at all hours. Can you add a sentence at the start to indicate something along these lines? Thanks! |
|
LGTM. Will merge during business hours. (or feel free to do the same ;) |
brendandburns
added
the
lgtm
Changed merge policy
Follow up of our discussion with Tim.
cc @thockin @brendandburns @rsokolowski @fgrzadkowski @wojtek-t @jszczepkowski @gmarek