Reversion Policy

[jasnell]

On the call it became apparent that we need to document the Reversion Policy (see https://github.com/jasnell/dev-policy/blob/master/meetings/2015-04-09.md#accepting-modifications-through-a-consensus-seeking-process).

Specifically, if a given bit of code lands in master, then becomes controversial for any reason, what is the process for reverting? This policy needs to consider pre-release reversions, post-release reversions, and reversions that occur during the release process.

[jasnell]

Related: #37

[mikeal]

I'll re-iterate my suggestion from the call:

We already have a provision for Release Managers to commit without a PR for metadata changes. We should extend this to reversions that break tests or the build that they can use at their discretion during a release.

[jasnell]

@mikeal : correct me if I'm wrong, but specifically this is: while in the process of cutting a release, a release manager would be given the discretion to revert any prior commit that is causing either the build or a test to fail. Are there any additional requirements around this? Do we need to say anything about giving the responsible collaborator an opportunity to fix the issue before reverting? Is there a post-release process to consider on these?

[chrisdickinson]

And my note from the call: this actually happened in io.js! An example of "landed in master, was released, revert was controversal":

The sequence of events:

1. An issue is opened – require('.') does not work as expected. There is general consensus that fixing it would be a good idea.
2. Someone issued a pull request to fix the behavior.
3. A release happened.
4. This breaks someone's workflow.
5. A reversion PR is issued. Contention about the fix arises.
6. It is noted that accepting the revert will break existing code and thus constitutes a major.
7. It's added to the 4/8 TC meeting.
8. At the meeting, a course of action is agreed upon after debate.

[jasnell]

I believe the existing documented resolution process addresses this. Whether pre-release or post-release, a revert is just another kind of PR (albeit it may be one with a higher priority depending on the circumstances). If the revert is controversial or would cause a semver-major change, then it would be reviewed by the TSC (either in a meeting or via discussion). We have a bit more leeway with pre-release reverts but Post-release reverts obviously need to be sensitive to the stability policy.

[Fishrock123]

As a "release manager" I'm not sure the "let us revert at-will" is good.

[jasnell]

@Fishrock123 .. with great power comes.. oh, nevermind ;-). As a release manager, what would you prefer the reversion-during-release policy to be?

[Fishrock123]

I'm not sure it should differ between what it already is.

Currently we have provisions to make two commits, like these: [1] & [2].

Anything that breaks the build should first of all be caught in nightlies.
If it's not caught there, it's probably either the build setup, or a much deeper issue.

[chrisdickinson]

I've come around to @Fishrock123's point of view here – if something needs reverted in a release it's probably best that it go through the full cycle. The only exception I can think of is if there's a particular known-bad commit holding up an urgent security patch, but! we might delegate that responsibility to the Security-WG-to-be ("Security WG may expedite security releases by reverting known bad commits during a release cycle." or something like that?) Or, alternatively, we take the release-reverting wording out entirely and wait to see if we need it?

[jasnell]

I'd vote for leaving out any release-reversion details at all at this point and focus primarily on pre and post release reversions only.. and then only to say that they are handled like any other modification.

[Fishrock123]

The only exception I can think of is if there's a particular known-bad commit holding up an urgent security patch,

Again:

• If a commit will cause a release build to fail, it will probably cause the CI to fail.
• Otherwise, the nightly should fail, it should be caught and reverted then.
• If neither of those caught it, it probably needs more investigation.

For these cases, I'd actually prefer to hold a release for review. This has not yet happened in io.js, and due to the first two cases should never happen at all.

[mhdawson]

My thinking is along the same lines as James' last comment. Handle them like other modifications until we experience specific issues that make us want to call out specific cases.

[jasnell]

We appear to have reached a good consensus on this item then. Anyone object to closing the issue?

[Fishrock123]

Hmmmm, well, not quite yet. Here is a real example: nodejs/node#1421 & nodejs/node@fd90b33...50e9fc1

cc @rvagg - thoughts?

[mhdawson]

Related is the requirements before a change is allowed in. Currently for Node with the Jenkins job committing changes, all tests must pass across platforms before the commit is landed. If this is not the case, the one case to call might be the auto-revert if not all tests pass on all platforms.