doc: update collaborator guide #19116

BridgeAR · 2018-03-04T00:40:26Z

This also updates some parts of the onboarding. It is mainly about
how to handle pull requests, how and when to start a CI, when to add
the ready label and some other minor changes.

I believe that adopting these suggestions will improve our general PR handling a lot.

It can probably be further polished though, so I am looking forward to your comments.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc

This also updates some parts of the onboarding. It is mainly about how to handle pull requests, how and when to start a CI, when to add the `ready` label and some other minor changes.

BridgeAR · 2018-03-04T00:41:20Z

Lite-CI https://ci.nodejs.org/job/node-test-pull-request-lite/205/

Trott

Lots of tiny nits, but I like where you're going with this.

Trott · 2018-03-04T00:42:03Z

COLLABORATOR_GUIDE.md

-or a pull request.
-[See "Who to CC in issues"](./doc/onboarding-extras.md#who-to-cc-in-issues)
+Collaborators should take full responsibility for managing issues and pull
+requests they feel qualified to handle. Just make sure this is done while being


Nit: Get rid of Just.

Trott · 2018-03-04T00:42:23Z

COLLABORATOR_GUIDE.md

-[See "Who to CC in issues"](./doc/onboarding-extras.md#who-to-cc-in-issues)
+Collaborators should take full responsibility for managing issues and pull
+requests they feel qualified to handle. Just make sure this is done while being
+mindful of these guidelines, the opinions of other Collaborators and guidance of


Nit: add a comma after Collaborators

Trott · 2018-03-04T00:43:30Z

COLLABORATOR_GUIDE.md

@@ -75,47 +77,86 @@ Collaborators or additional evidence that the issue has relevance, the
 issue may be closed. Remember that issues can always be re-opened if
 necessary.

+### Ready to land pull requests
+
+A pull request that is still awaiting the minimum review time is considered


Nit: is considered -> may be labeled

In this sentence my intention was to explain what ready stands for and not that we should label the PR accordingly. That is done in the sentence afterwards. Do you still want me to change that?

It's a nit; if you think it's better the way it is, then that works for me.

Trott · 2018-03-04T00:43:56Z

COLLABORATOR_GUIDE.md

+### Ready to land pull requests
+
+A pull request that is still awaiting the minimum review time is considered
+`ready` as soon as the CI got kicked off, it got at least one approval and


Nit: Change first got to has been and the second got to has

Nit: Add a comma after approval

Trott · 2018-03-04T00:44:09Z

COLLABORATOR_GUIDE.md

+
+### Handling own pull requests
+
+If you as a collaborator open a pull request, please always start a CI right


Nit: Collaborator

Trott · 2018-03-04T00:50:20Z

COLLABORATOR_GUIDE.md

+   [build](https://github.com/nodejs/build/issues)) or not and open new issues
+   in case they are not. If not CI was run or the run is outdated because code
+   was pushed after the last run, please first start a new CI and wait for the
+   result. If no CI is required, please leave a comment in case non is already


Typo: non -> none, I think

Trott · 2018-03-04T00:53:01Z

COLLABORATOR_GUIDE.md

@@ -655,12 +701,12 @@ hint: See the 'Note about fast-forwards' in 'git push --help' for details.
 ```

 That means a commit has landed since your last rebase against `upstream/master`.
-To fix this, fetch, rebase, run the tests again (to make sure no interactions
-between your changes and the new changes cause any problems), and push again:
+To fix this pull with rebase from upstream and run the tests again (to make sure


Nit: Comma after this

Trott · 2018-03-04T00:53:48Z

doc/onboarding.md

@@ -86,6 +86,10 @@ onboarding session.
      `semver-major` label
    * When adding a `semver-*` label, add a comment explaining why you're adding
      it. Do it right away so you don't forget!
+  * Please add the `ready` label for PRs where the CI was just kicked off and


Suggestion: was just kicked off -> has been started?

Trott · 2018-03-04T00:54:17Z

doc/onboarding.md

@@ -86,6 +86,10 @@ onboarding session.
      `semver-major` label
    * When adding a `semver-*` label, add a comment explaining why you're adding
      it. Do it right away so you don't forget!
+  * Please add the `ready` label for PRs where the CI was just kicked off and
+    that are good to go (no outstanding review items), pending the CI outcome


Suggestion: are good to go (no outstanding review items) -> have no outstanding review items

Trott · 2018-03-04T00:54:53Z

doc/onboarding.md

+  * Please add the `ready` label for PRs where the CI was just kicked off and
+    that are good to go (no outstanding review items), pending the CI outcome
+    and possibly the 48/72 hour waiting rule and the necessary LGs for
+    semver-major.


This sentence can use some editing for clarity. If all else fails, maybe change the last part to a bulleted list or something?

BridgeAR · 2018-03-04T14:20:46Z

Comments addressed :-)

BridgeAR · 2018-03-04T14:21:15Z

Lite-CI https://ci.nodejs.org/job/node-test-pull-request-lite/214/

BridgeAR · 2018-03-05T14:23:02Z

@nodejs/collaborators PTAL as this is important for each one of us.

mcollina · 2018-03-05T14:29:39Z

COLLABORATOR_GUIDE.md

+`ready` as soon as the CI has been started, it has at least one approval, and it
+has no outstanding review comments. Please always make sure to add the
+appropriate `ready` label to the PR in that case and remove it again as soon as
+that condition is not met anymore.


I thought ready was ok to land, so CI has run and someone has checked if this could land. So ready should mean "ready to land", not "check CI because this could be ready to land".

Also, it should include a CITGM run for semver-major.

@mcollina When I introduced that label, I didn’t intend it to have that meaning.

Practically speaking, it’s easier this way; you can kick off CI and add the label, because you’re planning to go through the list of open PRs with that label, and can then check CI.

In particular, it avoids having to keep track of which PRs have CIs started and which not.

As @addaleax explained it is meant to be the way it is described right now. This has helped me (and probably others just as much, but I can not speak for others) quite a lot already to keep a better overview of PRs that should be checked again.
If everything was already checked there is little point besides the waiting time to have the label at all because in that case the PR should just be merged instead.

I agree that it is easier and it's what we want, and it's probably the right moment to apply a tag. However, I don't think a PR is "ready to land" if it does not have CI passing, so the "ready" term is very misleading to me: I've landed something that should not have landed because of it. It's definitely not ready to land if CI has not been checked for which failures are infrastructure or not.

I'd have to agree with @mcollina on this. Perhaps ready in this case should really mean ready to consider landing. Before the PR can actually land, a look at the CI results is required.

mcollina · 2018-03-05T14:31:07Z

doc/onboarding.md

@@ -86,6 +86,13 @@ onboarding session.
      `semver-major` label
    * When adding a `semver-*` label, add a comment explaining why you're adding
      it. Do it right away so you don't forget!
+  * Please add the `ready` label for PRs where:
+    * the CI has been started,


The CI has been successfully run.

If CI was required, as some of this documentation indicates it may not be.

addaleax · 2018-03-05T14:41:13Z

Not a fan of requiring all PRs to constantly have CI runs, tbh. With our current setup, doing that & checking the outcomes takes significant time. At least when a PR has just been opened, it’s wasteful because there almost certainly are change requests coming up.

What matters in the end is that PRs have a recent CI run available for most of the time, not always.

BridgeAR · 2018-03-05T15:22:29Z

@addaleax my reasoning here is the following:

it is a significant overhead in case someone has to go through the PRs and start CI after CI. It will fill up the queue immediately and slow down our machines. If we have a CI run right away the workload is distributed, even though there are more runs overall.
there should not be any overhead by extra CI outcome checks. I think the intermediate ones are mainly to make sure we can tick the ready label on the PR and to see if the linter fails / if all CIs fail (CI status). That way we have a early signaling instead of having to check that again later.
about having to change something because of comments: there are multiple PRs where it would be fine with the first CI even though it is not the average case. But especially other OS failures can be detected right away instead of seeing this later on and having to comment again when someone wants to land the PR.

mcollina · 2018-03-05T16:17:36Z

COLLABORATOR_GUIDE.md

+Landing your own pull requests distributes the work load for each Collaborator
+equally. If it is still awaiting the
+[minimum time to land](#waiting-for-approvals), please add the `ready` label to
+it so it is obvious that the PR can land as soon as the time ends.


This sentence assumes that a "ready to land" means it has a passing CI. However, in the previous paragraph it's defined as "a PR with CI started", which definitely does not meet our criteria to landing them (we want CI passing, right?).

BridgeAR · 2018-03-05T17:00:57Z

So one thing got pretty clear for me: I think the name ready is confusing and we should find a new name for it. The reason is that it is not obvious that there are still steps left to do if the label sticks on a PR.

So far I could not come up with a great name but maybe something like pre-land or verify-ready?

gibfahn · 2018-03-05T17:06:00Z

So one thing got pretty clear for me: I think the name ready is confusing and we should find a new name for it. The reason is that it is not obvious that there are still steps left to do if the label sticks on a PR.

I think I missed the original discussion, but what is the benefit of the ready label? You still have to check that the PR is ready before landing, as things may have changed since ready was added.

I guess it means you can check back to when the ready tag was last added to see if anything is outstanding since then, but I'm not sure how useful that actually is.

BridgeAR · 2018-03-05T17:10:16Z

@gibfahn the label is a helper in case you want to go ahead and know which PRs are likely good to go. The label can be attached before the waiting time is over and you just check which ones are either fast tracked or over the time and then you check the CI result. If it is green, go ahead and land the PR. Otherwise remove the label and comment that something is wrong.

That is much much less work than randomly selecting a PR that might look ready. Because first, you have to see if there are outstanding comments, second you have to probably start a new CI and wait at least an hour for the result and then come back to check if it was green to actually be able to land the PR.

addaleax · 2018-03-05T17:10:38Z

I think I missed the original discussion, but what is the benefit of the ready label?

@gibfahn When I added it, my workflow was basically going through open PRs without that label, checking whether there was anything needed to be done for them, kick off CI if necessary, adding the label & then either landing it or pointing out CI failures.

The reason is that it is not obvious that there are still steps left to do if the label sticks on a PR.

It’s mostly outward communication: The naming basically indicates that there is nothing left to do on the author’s side.

mcollina · 2018-03-05T17:15:40Z

how about "author ready" then?

GlenTiki

I've left comments on the "exception" to running CI. It might be good to document when that exception can be made.

GlenTiki · 2018-03-05T17:12:36Z

COLLABORATOR_GUIDE.md

+All modifications to the Node.js code and documentation should be performed via
+GitHub pull requests, including modifications by Collaborators and TSC members.
+A pull request must be reviewed, and must also be tested with CI, before being
+landed into the codebase. There may be exception to the latter.


"There may be exception to the latter". When? Clarity would be useful here.

GlenTiki · 2018-03-05T17:16:58Z

doc/onboarding.md

@@ -86,6 +86,13 @@ onboarding session.
      `semver-major` label
    * When adding a `semver-*` label, add a comment explaining why you're adding
      it. Do it right away so you don't forget!
+  * Please add the `ready` label for PRs where:
+    * the CI has been started,


If CI was required, as some of this documentation indicates it may not be.

addaleax · 2018-03-05T17:23:11Z

Also, one more thought: For collaborators that want to get a PR into Node, they can do pretty much everything they need to do for that on their own.

That includes running CI, and the assumption should be that they want to merge that PR so they are motivated to keep a PR going anyway, with the extra rules from here in place or not.

If we want to reduce overhead in keeping track of PRs, a better solution might be easier starting and checking of CI runs; I think @joyeecheung was doing a decent amount of work for the automatic tooling?

For PRs from non-collaborators, that’s a different story, because they can’t start CI anyway. I would very much be +1 on recommending the first approver of a PR to kick off CI for it.

BridgeAR · 2018-03-05T17:29:30Z

I am definitely +1 on automating the CIs for collaborators! We could actually go so far and check the changed files to see if we have to start a lite CI (documentation changes) or a regular one.

addaleax · 2018-03-05T17:31:08Z

I’ve changed the name to author ready. Renaming labels is cheap! :)

gibfahn · 2018-03-05T19:50:24Z

@gibfahn the label is a helper in case you want to go ahead and know which PRs are likely good to go. The label can be attached before the waiting time is over and you just check which ones are either fast tracked or over the time and then you check the CI result. If it is green, go ahead and land the PR. Otherwise remove the label and comment that something is wrong.

@gibfahn When I added it, my workflow was basically going through open PRs without that label, checking whether there was anything needed to be done for them, kick off CI if necessary, adding the label & then either landing it or pointing out CI failures.

Thanks for the explanations, that does make sense I guess. I agree that an automated solution would probably be better, but this does seem useful in the interim.

BridgeAR · 2018-03-06T23:46:56Z

Comments addressed. PTAL.

CI https://ci.nodejs.org/job/node-test-pull-request-lite/234/

BridgeAR · 2018-03-08T11:32:30Z

@nodejs/collaborators PTAL. If there are no further comments, I would like to land this soon.

mcollina

LGTM

vsemozhetbyt · 2018-03-08T12:53:13Z

COLLABORATOR_GUIDE.md

+   * The merge method will add an unnecessary merge commit.
+   * The squash & merge method has been known to add metadata to the commit
+     title (the PR #).
+   * If more than one author has contributed to the PR, keep the most recent


Is this point a bit confusing? If this is an advice, should we get it out of "Reasons for not using..." part? If this is a green button artifact, should the "keep" become "keeps" and should the culprit method be mentioned?

In fact, I'd strictly be in favor of keeping code by authors who are not core collaborators in PRs even if they are not the latest. I think that if there are multiple contributors we should keep commits from all of them (perhaps with the same commit message)

It requires judgment, particularly around how significant the contribution is. If someone contributed something significant, squashing commits in a way that removes them as an author should certainly be avoided. That can look like (or can actually be) stealing credit for someone else's work. On the other hand, we do have to make sure that all tests pass with each commit, and sometimes squashing is the most reasonable way to achieve that.

I see the point that this is confusing but I would really like to keep this out of the scope of this PR. I did not change this part, so I guess that is fine?

Oh, yes, hadn't noticed that this content has not changed and this is just a formatting/white-space change to this part. Yeah, the comments should be addressed in another PR IMO.

vsemozhetbyt · 2018-03-08T13:18:33Z

doc/onboarding.md

@@ -86,6 +86,13 @@ onboarding session.
      `semver-major` label
    * When adding a `semver-*` label, add a comment explaining why you're adding
      it. Do it right away so you don't forget!
+  * Please add the `author-ready` label for PRs where:


All this list seems confusing grammar-wise. "where:" prefix does not combine smoothly with points like "that have no outstanding review comments and". It seems we have to unify the grammatical pattern of the points or reword the introductory sentence.

Yeah, the list is a bit confusing. the CI has been started and pending the CI outcome seem redundant with each other, for example. Or maybe there's a nuance there that I'm missing?

About the CI has been started and pending the CI outcome: It should be clear that a) the CI has to be started and b) it does not have to be done when adding the label. Do you maybe have a better wording for that?

I think CI has been started is sufficient. But if you want to be explicit, maybe CI has been started (not necessarily finished) or something like that?

mhdawson · 2018-03-08T22:52:45Z

COLLABORATOR_GUIDE.md

+after (see [testing and CI](#testing-and-ci) for further information on how to
+do that) and post the link to it as well. Please also make sure that you start a
+new CI after each update (due to e.g., a change request in a review or due to
+rebasing).


I'm not 100% sure starting a CI right after opening is the best way to go. I often wait until there are a few reviews to see if I'm going to have to make any updates.

I would like to keep it this way until we get that part automated. Do you feel strongly about it?

See the former discussion for the reasons, e.g., #19116 (comment).

@BridgeAR Can we hold off landing this before we have consensus here?

Or, alternatively, could you reword this as a suggestion?

(That is a general feeling I have about this PR, btw: I don’t think we should impose our own workflows on other people.)

I personally am not a huge fan about starting a CI always either ^^. The only reason why I think it is good, is that it improves the handling of the PRs as far as I see it.
I also believe that we all want to have a automation of this for collaborators, so I think this is just a intermediate step. I am going to open a issue in build to get some insight in how we might be able to automate it.

This is, by all means, not meant to impose a workflow on someone. Even with the current wording I do not see it as something mandatory, just as something that is really nice to have. I am going to tone down a wording a bit.

@addaleax @mhdawson is the new wording OK for you? If not: do you have a suggestion how to reword it?

GlenTiki

LGTM

BridgeAR · 2018-03-11T01:43:57Z

As far as I see it there is no one against landing this as is right now, right?

Because in that case I would like to land this on Monday.

BridgeAR · 2018-03-11T22:55:11Z

Lite-CI https://ci.nodejs.org/job/node-test-pull-request-lite/250/

joyeecheung · 2018-03-12T20:46:50Z

doc/onboarding.md

+  * Please add the `author-ready` label for PRs where:
+    * the CI has been started (not necessarily finished),
+    * no outstanding review comments exist and
+    * at least one collaborator approved the PR.


I personally would not add the label if it's semver-major and has not received 2 TSC approval yet...probably just a different interpretation of the label though.

I have no strong opinion either way. It was just my current interpretation. Because I feel the author is ready as soon as a collaborator approved it. We have to make sure it gets enough LGs one way or the other and we know that we have to trigger the TSC in case some are missing.

Shall I change it?

@BridgeAR We can revisit that later, probably need to update the label description as well if we are going to change its meaning

This also updates some parts of the onboarding. It is mainly about how to handle pull requests, how and when to start a CI, when to add the `ready` label and some other minor changes. PR-URL: nodejs#19116 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Chen Gang <gangc.cxy@foxmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Glen Keane <glenkeane.94@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

BridgeAR · 2018-03-21T00:01:38Z

Thanks a lot to everyone :-)

Landed in 1b8746f

This also updates some parts of the onboarding. It is mainly about how to handle pull requests, how and when to start a CI, when to add the `ready` label and some other minor changes. PR-URL: #19116 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Chen Gang <gangc.cxy@foxmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Glen Keane <glenkeane.94@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

doc: update collaborator guide

0988b69

This also updates some parts of the onboarding. It is mainly about how to handle pull requests, how and when to start a CI, when to add the `ready` label and some other minor changes.

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Mar 4, 2018

Trott approved these changes Mar 4, 2018

View reviewed changes

JungMinu approved these changes Mar 4, 2018

View reviewed changes

BridgeAR added 2 commits March 4, 2018 14:19

fixup: address comments

b77b56a

fixup

0652f24

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 4, 2018

mcollina reviewed Mar 5, 2018

View reviewed changes

devsnek approved these changes Mar 5, 2018

View reviewed changes

MoonBall approved these changes Mar 5, 2018

View reviewed changes

jasnell approved these changes Mar 5, 2018

View reviewed changes

mcollina reviewed Mar 5, 2018

View reviewed changes

GlenTiki reviewed Mar 5, 2018

View reviewed changes

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 5, 2018

fixup address comments

12ef5df

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 6, 2018

mcollina approved these changes Mar 8, 2018

View reviewed changes

vsemozhetbyt reviewed Mar 8, 2018

View reviewed changes

mhdawson reviewed Mar 8, 2018

View reviewed changes

fixup

a9e01da

vsemozhetbyt approved these changes Mar 9, 2018

View reviewed changes

GlenTiki approved these changes Mar 9, 2018

View reviewed changes

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 11, 2018

fixup: address comment

79c48f5

joyeecheung approved these changes Mar 12, 2018

View reviewed changes

addaleax approved these changes Mar 12, 2018

View reviewed changes

BridgeAR closed this Mar 21, 2018

BridgeAR mentioned this pull request Apr 25, 2018

Author ready label not used #20282

Closed

BridgeAR deleted the update-collaborators branch April 1, 2019 23:39


		### Handling own pull requests

		If you as a collaborator open a pull request, please always start a CI right

doc: update collaborator guide #19116

doc: update collaborator guide #19116

Conversation

BridgeAR commented Mar 4, 2018

Checklist

Affected core subsystem(s)

BridgeAR commented Mar 4, 2018

Trott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Mar 4, 2018

BridgeAR commented Mar 4, 2018

BridgeAR commented Mar 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Mar 5, 2018

BridgeAR commented Mar 5, 2018

Choose a reason for hiding this comment

BridgeAR commented Mar 5, 2018

gibfahn commented Mar 5, 2018

BridgeAR commented Mar 5, 2018 • edited Loading

addaleax commented Mar 5, 2018

mcollina commented Mar 5, 2018

GlenTiki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Mar 5, 2018

BridgeAR commented Mar 5, 2018

addaleax commented Mar 5, 2018

gibfahn commented Mar 5, 2018

BridgeAR commented Mar 6, 2018

BridgeAR commented Mar 8, 2018

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GlenTiki left a comment

Choose a reason for hiding this comment

BridgeAR commented Mar 11, 2018

BridgeAR commented Mar 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Mar 21, 2018

BridgeAR commented Mar 5, 2018 •

edited

Loading

Trott Mar 8, 2018 •

edited

Loading

Trott Mar 8, 2018 •

edited

Loading