doc: clarify "Reviewed-By" iff "LGTM"

[bengl]

Checklist

• documentation is changed or added
• the commit message follows commit guidelines

Affected core subsystem(s)

doc

Description of change

As per conversation with @Trott, make it clear that Reviewed-By lines
should only be added for collaborators who've actually put a LGTM on the
PR.

[MylesBorins]

LGTM

[evanlucas]

lgtm

[cjihrig]

LGTM

[jasnell]

LGTM

[JacksonTian]

LGTM

[rvagg]

I've been pretty liberal in the past including others who have reviewed and I know add value to the process yet are not collaborators. This is particularly helpful when you have specialist issues that require an understanding not widely shared across the team but you want assurance. It's also helpful getting bug reporters reviewing stuff.

I don't mind the "at least one collaborator must sign off" rule, but I don't see a reason not to include outsiders in the review process and acknowledge them as such in the commits.

[Trott]

I don't mind the "at least one collaborator must sign off" rule, but I don't see a reason not to include outsiders in the review process and acknowledge them as such in the commits.

News to me! (Not saying it's wrong; just saying I was unaware!) Perhaps that's a sufficiently atypical edge-case that it doesn't need to be covered in onboarding? Maybe it can be mentioned in the onboarding_extras doc for people who like to read everything?

[Fishrock123]

Likewise, I've sometimes included people who have reviewed it in a clearly positive way once others have given lgtm signoff.

[jasnell]

Given that there have been several complaints made (generally via private conversations) about overly "liberal" use of sign off, I think being clearer and stricter about this is generally a good thing. One possible way to allow for additional recognition is to use a separate metadata label for others who helped with the review but aren't collaborators who gave a LGTM. For instance, Thanks: ...

[mhdawson]

Would those complaints have been addressed with "at least N L.G.T.Ms from collaborators" ?. I'd lean towards updating to require at least some number of collaborators and then possibly identifying them by adding (collaborator) after their entry in the Reviewed-by.

[Trott]

Would those complaints have been addressed with "at least N L.G.T.Ms from collaborators" ?

The current ground rules are that one LGTM from a collaborator is enough, but more is always better. I think it should stay that way because there are a few dark corners of the code where it is difficult to get more than one collaborator to confidently pronounce on it if there is anything complex being changed/added.

The ground rules are also that no matter how many LGTMs you amass, one objection from a collaborator is enough to stop the PR from merging. (There's always the escalate-to-CTC option if there's an impasse, but we strongly encourage messy consensus seeking first.)

[jasnell]

Would those complaints have been addressed with "at least N L.G.T.Ms from collaborators" ?

I don't believe so. The complaints were about the "quality" of the LGTM's. Without going into too much detail, the gist is that several individuals felt that "Reviewed-By" sign-offs should only be given by people who were "qualified" to fix any potential issues / regressions that occur as a result of the PR.

[bengl]

The intent of this PR was not to establish who has reviewing/LGTM rights, but simply to confirm that "Reviewed-By" sign-offs should only be added for a person when they have LGTM'd, rather than having simply commented. To that end, would a simple s/collaborators/those/ suffice, deferring the qualifications for reviewership to be handled elsewhere?

[jasnell]

My vote is for this PR as it is currently worded: Reviewed-By is for collaborator LGTM's only. To recognize non-collaborator reviews, let's use a different label (e.g. the Thanks: ... I suggested)

[reqshark]

as a non-collaborator, LGTM!

[rvagg]

A Thanks: line works for me, but we'd better updating tooling to do this because keeping track of who is and who isn't a collaborator is becoming very difficult and the GitHub UI isn't much help on this either. I'll try and get https://github.com/rvagg/iojs-tools/tree/master/pr-metadata updated, @evanlucas I think you have something similar too?

[MylesBorins]

@rvagg I think you are referring to https://github.com/evanlucas/core-get-reviewers

[saghul]

I'd just like to point out (as a collaborator), that my judgement in ~80% of Node would be meaningless. I just don't have enough kowledge to take the judgement call. Using common sense, I just don't LGTM things that I don't feel qualified to, but it looks like this is already a problem. That said, a non-collaborator could be a lot more qualified to detrmine if something is right or not that an actual collaborator.

As an example, in libuv I (pretty much blindly, when I can't make sense of it) trust Alexis (not a collaborator) for Windows related reviews. His LGTM is a lot more valuable than mine on that realm.

Alas, I also don't have an answer for a better system, but I thought I had to get this in writing.

[Fishrock123]

Almost need both Signed-off-by and reviewed-by now by the sounds of it, lol

[Trott]

Can we land this as is and punt the "what metadata to use for non-collaborators" to another issue?

This clarifies a rather important gap in the onboarding doc. As longtime collaborators. most of the folks commenting here have simply internalized that an LGTM by a collaborator translates into a Reviewed-by: in the metadata. Let's get that fact explicitly into the onboarding doc because (as demonstrated by the fact that @bengl had to ask in IRC about this) it's not necessarily obvious to newcomers.

The whole "whether and how to note non-collaborators who provide significant review" issue is important. But for the onboarding document, that is a bikeshed conversation in comparison to the hole this plugs.

[Trott]

@bengl: Maybe remove the word Only so it says: include collaborators who have commented "LGTM"? That would accommodate the apparent diversity of opinion about whether non-collaborators should be included and how.

I'd also consider moving it to the first thing in that bulleted list.

[jasnell]

Landed as is in 5d6d3ee. @Trott, i know you had some additional suggestions but those can be handled in a separate PR.

[yorkie]

Does CTC members should be included as well?

[jasnell]

Yes, certainly. All of the ctc members are also collaborators.

On Saturday, August 6, 2016, Yorkie Liu notifications@github.com wrote:

In doc/onboarding.md
#7183 (comment):

@@ -169,6 +169,7 @@ Landing a PR
* Reviewed-By: human <email>
* Easiest to use git log then do a search
* (/Name + enter (+ n as much as you need to) in vim)

•  \* Only include collaborators who have commented "LGTM"
  

Does CTC members should be included as well?

—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
https://github.com/nodejs/node/pull/7183/files/f645b4c8da57891c7cf5b59987548e87e32479fb#r73786541,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAa2eXgC6ANpp1R5Da2oSUQu7NEtHcdmks5qdJUvgaJpZM4Ivauh
.

[yorkie]

But currently the CTC members & collaborators are different lists in our README.md#Current Project Team Members, I thought that might have something confused to new comers :-)

[jasnell]

Oh good point! I hadn't considered that. It might be worthwhile clarifying
that in the readme, I think :)

On Saturday, August 6, 2016, Yorkie Liu notifications@github.com wrote:

In doc/onboarding.md
#7183 (comment):

@@ -169,6 +169,7 @@ Landing a PR
* Reviewed-By: human <email>
* Easiest to use git log then do a search
* (/Name + enter (+ n as much as you need to) in vim)

•  \* Only include collaborators who have commented "LGTM"
  

But currently the CTC members & collaborators are different lists in our README.md#Current
Project Team Members
https://github.com/nodejs/node#current-project-team-members, I thought
that might be have something confused to new comers :-)

—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
https://github.com/nodejs/node/pull/7183/files/f645b4c8da57891c7cf5b59987548e87e32479fb#r73786660,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAa2eQ5zIYoRSJARismkpHzT-BoE0xn2ks5qdJhUgaJpZM4Ivauh
.

[yorkie]

Oh good point! I hadn't considered that. It might be worthwhile clarifying
that in the readme, I think :)

@jasnell submitted a pull-request #7996 for this.

[jbergstroem]

How about expanding it as well?

• Only include collaborators who have commented "LGTM" (Looks Good To Me)