Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Meta: revert grace period #34701

Closed
mmarchini opened this issue Aug 10, 2020 · 6 comments
Closed

Meta: revert grace period #34701

mmarchini opened this issue Aug 10, 2020 · 6 comments
Labels
meta Issues and PRs related to the general management of the project.

Comments

@mmarchini
Copy link
Contributor

It has been suggested that we might need/want a grace period after a PR lands where a PR could be reverted as a retroactive objection. There's some discussion on the PR on whether this is necessary or not. Since the PR was landed, I'm opening this issue to continue discussion.

cc @guybedford


Original suggestion:

I was wondering is if we should allow a grace period for PR reversion when collaborators miss something? For example, something like:

If a Collaborator is unaware of a PR landing that they would otherwise have blocked, a 48 hour grace period is provided in which previously unstated objections may be retroactively made allowing for the PR to be reverted. After that time period the process of reverting requires full consensus to land.

@mmarchini
Copy link
Contributor Author

Copying my response from the PR since it's the discussion is kinda spread there and can be hard to follow:

We already have review times, unless/until we remove those I don't think that's necessary (definitely something to think about if we ever land #33879). We don't have rules against reverting commits, so any collaborator can open a PR asking to revert a change by providing reasoning. Blocking such a PR follows the same rules as objecting to any other PRs, so appropriate reason should be provided (I wouldn't consider "we landed this already" an appropriate reason for example, but using the same reasoning as to why the original PR was opened would be valid). If a revert is requested because process was not followed, it can always be fast-tracked (and likely will be, like the recent incident).

and

The issue is that the revert might not be able to land as it might not have consensus since the criteria to land have flipped around.

If there are objections in the revert PR and the reverter and objectors can't reach a consensus, the issue would be escalated to TSC and the PR would either land or be closed, just like the original PR. Unless it's more likely for TSC to rule one way or the other depending if it's a PR or a revert, which I don't think would be the case for most PRs (unless the original PR was included in a release). Again, "we already landed this PR" is not, in my opinion, a valid reason to object to a revert PR.

I see your point on the grace period, but then again, there's no reasonable wait time we can add to prevent someone from eventually missing a PR. (...)

@mmarchini mmarchini added the meta Issues and PRs related to the general management of the project. label Aug 10, 2020
@devsnek
Copy link
Member

devsnek commented Aug 10, 2020

I don't think we've had any big problems with this. The only examples that come to mind are my semver version comparison api prs, and those happened over a year ago.

I think the bigger picture here though, is that if we were wanting of a revert grace period, we should just make the initial review time requirement longer.

@mmarchini
Copy link
Contributor Author

I think the bigger picture here though, is that if we were wanting of a revert grace period, we should just make the initial review time requirement longer.

Agreed, with our current process there's no reason to have a grace period since we already have wait times for PRs to land. Personally I think making the wait times longer would hurt the project more than help (we had longer wait times before in some cases, and we reduced it because it was a pain to deal with).

@jasnell
Copy link
Member

jasnell commented Aug 10, 2020

Let's please not even consider longer wait times. For the more obscure and specialized parts of the codebase the wait time is already extremely painful.

@guybedford
Copy link
Contributor

The act of merging is useful information. In this way a grace period does provide more information to the collaborators than just having a longer wait time. I guess from this perspective a grace period can also act as a way to allow faster merges if there is less risk.

But since no one seems to be pushing for anything right now, perhaps this isn't necessary.

If there ever were a push in future to extending merge times, perhaps considering a grace period at that point could be a way to find a middle ground.

@tniessen
Copy link
Member

This discussion hasn't been brought up in nearly three years, so I am going to assume that this issue can be closed for now. If that's incorrect, please feel free to reopen or comment, or open a new issue.

@tniessen tniessen closed this as not planned Won't fix, can't repro, duplicate, stale Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

No branches or pull requests

5 participants