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.

Sign up for GitHub

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

Jump to bottom

doc: clarify fast-track of reversions #17332

Merged
merged 1 commit into from
Nov 28, 2017
Merged

doc: clarify fast-track of reversions #17332

merged 1 commit into from
Nov 28, 2017

Conversation

refack
Copy link
Contributor

@refack refack commented Nov 26, 2017

Checklist
Affected core subsystem(s)

doc,meta

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Nov 26, 2017
@refack
Copy link
Contributor Author

refack commented Nov 26, 2017 •
edited
Loading

/CC @jasnell @gireeshpunathil @tniessen @mhdawson
Since this is a followup to #17056

bnoordhuis
bnoordhuis requested changes Nov 26, 2017
View reviewed changes
COLLABORATOR_GUIDE.md Outdated
@@ -127,7 +127,7 @@ can be fast-tracked and may be landed after a shorter delay:
* Focused changes that affect only documentation and/or the test suite.
`code-and-learn` and `good-first-issue` pull requests typically fall
into this category.
* Changes that revert commit(s) and/or fix regressions.
* Changes that revert recently landed commit(s) and/or fix regressions.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just "Changes that fix regressions"? That covers reverts.

It doesn't matter how old the commit is. If it introduces a regression that can't be fixed speedily, first order of business should be to revert.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 28, 2017
@MylesBorins
Copy link
Contributor

was there a previous discussion around this regarding reverts? Is the idea that reverts are included in regressions?

@addaleax
Copy link
Member

@MylesBorins #17332 (comment)

@MylesBorins
Copy link
Contributor

it would seem that perhaps we want to be specific about them being recent regressions? Do we want to fast track any bug fix??

@targos
Copy link
Member

targos commented Nov 28, 2017

@MylesBorins your message made me think a little bit more about this. I agree we should be more specific because it should not be about fast-tracking any regression.
Here are the two situations that IMO justify fast-tracking:

  • Regressions that break contributor's workflow (like red CI, broken compilation)
  • Regressions that happen right before a release. If the fix is obvious -> fast track. Otherwise the commit introducing the regression can be backed out of the release branch.

@joyeecheung
Copy link
Member

@targos There is another type of fast-tracking that we do in practice: commits fixing regressions that have just been released and reported soon after the release. We usually do a quick release for them soon after they land.

joyeecheung
joyeecheung reviewed Nov 28, 2017
View reviewed changes
COLLABORATOR_GUIDE.md
@@ -127,7 +127,7 @@ can be fast-tracked and may be landed after a shorter delay:
* Focused changes that affect only documentation and/or the test suite.
`code-and-learn` and `good-first-issue` pull requests typically fall
into this category.
* Changes that revert commit(s) and/or fix regressions.
* Changes that fix regressions.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better if we include the cases listed by @targos and in #17332 (comment) here.

mhdawson
mhdawson approved these changes Nov 28, 2017
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@refack
Copy link
Contributor Author

refack commented Nov 28, 2017

I'd rather we land this first, since it's small and focused change, with wide approval, and continue iterating later.
Also this is not the entire language of this section, fast tracking a PR still requires the discretion and consensus of at least 3 people.

@refack
doc: clarify fast-track of reversions
41c45fd 
PR-URL: nodejs#17332
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@refack refack force-pushed the COLLABORATOR_GUIDE-fast-track-reversions branch from d5d523e to 41c45fd Compare November 28, 2017 22:40
@refack refack merged commit 41c45fd into nodejs:master Nov 28, 2017
@refack refack mentioned this pull request Nov 28, 2017
Closed
2 tasks
@refack
Copy link
Contributor Author

refack commented Nov 28, 2017

follow up on comments is in #17379

@refack refack added meta Issues and PRs related to the general management of the project. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Nov 28, 2017
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@refack @MylesBorins
doc: clarify fast-track of reversions
016691c 
PR-URL: #17332
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@refack @MylesBorins
doc: clarify fast-track of reversions
bd55a79 
PR-URL: #17332
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
Merged
gibfahn pushed a commit that referenced this pull request Dec 19, 2017
@refack @gibfahn
doc: clarify fast-track of reversions
8c2707b 
PR-URL: #17332
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
gibfahn pushed a commit that referenced this pull request Dec 19, 2017
@refack @gibfahn
doc: clarify fast-track of reversions
90ee2ee 
PR-URL: #17332
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Closed
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
@refack @gibfahn
doc: clarify fast-track of reversions
0605f01 
PR-URL: #17332
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Merged
@MylesBorins MylesBorins mentioned this pull request Dec 20, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joyeecheung joyeecheung joyeecheung approved these changes

@mhdawson mhdawson mhdawson approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@aqrln aqrln aqrln approved these changes

@tniessen tniessen tniessen approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@cjihrig cjihrig cjihrig approved these changes

@targos targos targos approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

@danbev danbev danbev approved these changes

@gibfahn gibfahn gibfahn approved these changes

@maclover7 maclover7 maclover7 approved these changes

Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.