meta: clarify pr objection process further #59096
Conversation
Based on some recent confusion around the objection process for PRs, this commit adds some additional clarification to the collaborator guide. Specifically, it clarifies that: * Objections must be made in the PR itself * All objections are considered equal... no special additional weight is given to objections from TSC members. * When mistakes happen and a PR lands despite having an unresolved objection, any revert or fixup PR is subject to the same regular objection process, albeit with a callout that fast-tracking is possible if uncontroversial.
jasnell
added
the
meta
|
Review requested:
|
There was a problem hiding this comment.
I 100% agree with the first two points, but the third one doesn't make sense in my opinion. If a PR was wrongly merged due to not solving all objections, we must revert that and reach a consensus - even if the consensus would be to a TSC vote. From my perspective, the repository source code must always represent the project agreement.
Co-authored-by: Yagiz Nizipli <[email protected]>
It's not always immediately clear that a revert actually is the correct solution. A follow up PR that simply addresses the concern might be the best approach. It depends entirely on what the nature of the objection is. For instance, if it's "we shouldn't have this API", then yes, revert would be appropriate. If it's, "This needed an additional test or documentation" then a follow up PR is actually better than a revert. We should also recognize that there's exceedingly little urgency involved in these things in the typical case. Changes don't really become an issue until they are shipped in a release. Most of the time rushing to revert something is often simply unnecessary. But that's my opinion, and I thank you for yours :-) ... Let's get some more points of view from more collaborators and see where the majority consensus lies. |
|
@jasnell see my proposal at #59096 (comment). |
| the objection. Such reverts / follow-ups will typically be subject to | ||
| fast-tracking, particularly if they are uncontroversial, but an | ||
| automatic, immediate revert is not always necessary and, in some | ||
| cases might not be ideal. |
There was a problem hiding this comment.
I see that my original review was marked as resolved but it still applies:
| the objection. Such reverts / follow-ups will typically be subject to | |
| fast-tracking, particularly if they are uncontroversial, but an | |
| automatic, immediate revert is not always necessary and, in some | |
| cases might not be ideal. | |
| the objection. Such reverts / follow-ups will be fast-tracked to keep the | |
| main branch in a state where the governance of the project is aligned. |
There was a problem hiding this comment.
I addressed the comment by editing the text based on the subsequent discussion. As I indicated, I disagree with requiring fast-track for these. So I'm -1 on this suggestion or any alternatives that require fast-tracking these.
There was a problem hiding this comment.
I strongly believe that reverting a mistake should be different than a regular pull-request.
There was a problem hiding this comment.
Ok, I doubt we're going to come to agreement on this point naturally so we'll leave this point unresolved for now. I've tagged tsc-agenda but also encourage further input from other collaborators.
There was a problem hiding this comment.
I do still tend to agree here per my previous comment - that if there was a process issue and something was merged without following proper process, the revert should only be possible to object to based on the reason that the initial PR objection did not follow the correct process to be considered a formal and active objection.
There was a problem hiding this comment.
At this point I'm not sure what exactly we're agreeing to disagree about @RafaelGSS ... in fact, based on your comments above I think we are actually in agreement, at least as far as what this PR is saying. If you have concrete suggestions on how to improve the text further, please do provide them!
There was a problem hiding this comment.
1. If a PR is merged despite an unresolved substantive objection or required change, it must be reverted.
Regardless of what our current process says, I still believe this should be our first recommendation. We’ve discussed it and don’t share the same view, which is why I said we agree to disagree.
There was a problem hiding this comment.
Ok, thank you for clarifying! And yeah, we disagree on that a bit... I think largely only around the interpretation of "substantive" and the use of "must". What I'm wanting to get to is a solid compromise on the language here so and get a sense of where we do agree rather than just giving up on parts where we don't. To that end,
-
We agree that if a PR is merged with outstanding unresolved objections, then corrective action must be taken. (intentionally not clarifying the type of objection, or type of corrective action, and very intentionally using "must" here).
-
We appear to agree that for some kinds of unresolved objections, ones that I infer based on your comments you'd consider non-substantive, or minor, the corrective action could be a simple follow-up PR rather than a revert. I think we might disagree about what kinds of changes count as "non-substantive", but that's ok.
-
We appear to agree that, at least in more substantive cases, a revert is the appropriate corrective action. I think where we disagree here is only on the use of the word "should" vs. "must".
-
I think we might disagree on the timing of the corrective action, maybe? This I'm not sure about. There are some who believe the correction should be absolutely as quickly as possible regardless of situation/context, while I tend to hold that there's no real urgency to any of these kinds of situations so it's ok if the corrective action doesn't happen immediately.
There was a problem hiding this comment.
About the timing, my perspective is that as soon as the mistake is recognised, it should be reversed. Other than that, your understanding of our agreement/disagreement is correct.
There was a problem hiding this comment.
@anonrig ... please take a look at the most recently updated text and let me know if you're still objecting.
jasnell
added
the
tsc-agenda
|
Adding to |
Co-authored-by: Jordan Harband <[email protected]>
|
@nodejs/tsc ... just pinging for additional input on this. I'd actually prefer to see if we can get consensus on this through discussion in the PR rather than having to take it to a TSC meeting. |
Co-authored-by: Antoine du Hamel <[email protected]>
Co-authored-by: Guy Bedford <[email protected]>
Based on some recent confusion around the objection process for PRs, this commit adds some additional
clarification to the collaborator guide.
Specifically, it clarifies that:
@nodejs/tsc