meta: note re author check in COLLABORATOR_GUIDE #16360

vsemozhetbyt · 2017-10-21T13:29:46Z

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc, meta

I still feel uneasy about this incident (see also #16339 (comment): the author still is not promoted), so I am trying to add a note for a lander side as well, something like the #16340 has done for the authors' side. I hope this is not an overcaution.

Please, correct my wording if necessary.

I wish I had compared this data yesterday:

I am sorry, @hyades.

maclover7 · 2017-10-21T13:32:10Z

COLLABORATOR_GUIDE.md

+associated with the author's account and the first-time contributor
+will not be promoted to Contributor once their first commit is landed.
+If in doubt, please contact the author before landing, then abort and reapply
+the patch if the author have changed their username and email


have --> has

Done. Thank you.

maclover7 · 2017-10-21T13:32:34Z

COLLABORATOR_GUIDE.md

+If you land a somebody else's PR (especially a first-time contributor's PR),
+please make sure that a username and an email in the log match the ones
+in the author's GitHub account. Otherwise, the commits will not be properly
+associated with the author's account and the first-time contributor


should there be a comma after author's account?

maclover7

Not a collaborator, but 👍

lpinca · 2017-10-21T13:59:48Z

This adds yet another thing to address to our already complex landing procedure. I understand the importance but I think it should be Collaborator's responsibility not our.

Checking every single first time contributor account seems a bit too much. Also if the email is private, how do you know if it matches the one in the patch?

apapirovski · 2017-10-21T14:03:21Z

FWIW I think it's possible to check this simply within the PR by going to commits and seeing if their name is linked or not. This check could be a part of reviewing PRs from first-time contributors.

vsemozhetbyt · 2017-10-21T14:06:28Z

@lpinca

Also if the email is private, how do you know if it matches with the one in the patch?

Well, for this case I've added a suggestion to contact the author.

As I said, I am not sure if this is not an overcaution. If this is considered a tangible burden, then I will close the PR.

vsemozhetbyt · 2017-10-21T14:10:07Z

@apapirovski Can you suggest the place and the wording for this variant? Or maybe you can open a new PR, then I will close this one in favor of yours. I am still not very good at long English texts.

joyeecheung · 2017-10-21T14:12:04Z

I think a better way to avoid this kind of incidents is via automation. Probably can update node-review to do some checks (I have a rough idea, but need another sample to test).

apapirovski · 2017-10-21T14:13:53Z

@vsemozhetbyt I think we can keep this open, maybe a few more people will chime in with different suggestions.

And I agree re: automation. Either node-review could do it (does everyone use it though?) or — and I don't know how our bot works so take this with a grain of salt — it's probably something that a bot could be made to do.

vsemozhetbyt · 2017-10-21T14:27:17Z

cc @evanlucas re node-review (and core-validate-commit ?) and @nodejs/github-bot re github-bot

joyeecheung · 2017-10-21T14:28:53Z

Wait, I found an example (and oops, I landed that). I think it is simpler than I thought.

Just look at the icons, they don't match. I think we don't really need automation to identify this.

apapirovski · 2017-10-21T16:53:08Z

Just look at the icons, they don't match. I think we don't really need automation to identify this.

And yet we miss it all the time. :( Part of it is that in the PR above there are 4 commits and so it's a lot more obvious, but it's very easy to miss it when it's just one commit.

I think having a label and an automated comment from a bot would go a long way towards not having any commits land with the wrong email. I'll have a look at the bot this weekend to see if this is possible and if so, how much effort it would take.

And in all honesty, the more we automate, the less pressure there is on collaborators to get everything just right.

Trott · 2017-10-22T03:47:08Z

COLLABORATOR_GUIDE.md

@@ -452,6 +452,15 @@ Check number of commits and commit messages
 $ git log upstream/master...master
 ```

+If you land a somebody else's PR (especially a first-time contributor's PR),


land a somebody else's -> land somebody else's

joyeecheung · 2017-10-22T05:17:27Z

And in all honesty, the more we automate, the less pressure there is on collaborators to get everything just right.

Totally +1 @apapirovski intersted in nodejs/TSC#392 ? :D

gibfahn · 2017-10-22T09:02:01Z

I think this is fine as long as we understand that people are going to forget this until we automate it. We ask collaborators to check approvals, check CI was run, and to check core-validate-commit passes. That's already fairly complex, so anything more has to be secondary.

I also emailed Github Support about the problem, hopefully they can do something that improves the situation.

Refs: #16339 (comment)

joyeecheung · 2017-10-22T10:55:50Z

FWIW, I am playing with the Github GraphQL API today and looks like this is what the entry authoredByCommitter is for

gibfahn · 2017-10-22T12:29:28Z

I also emailed Github Support about the problem, hopefully they can do something that improves the situation.

GitHub said they'd look into it, but the two cases linked here (#16339 (comment) and #15976 (comment)) are already updated (since 5 hours ago). So it sounds like it's not the end of the world if we don't catch this before landing.

jasnell · 2017-10-22T16:28:22Z

Does adding the email to .mailmap help?

vsemozhetbyt · 2017-10-23T09:52:03Z

So maybe we can leave this PR open for a while as a reminder, till we try to automate this or fall back on a document solution with a consensus.

joyeecheung · 2017-10-31T06:05:40Z

Hey, we just implemented this check in get-metadata https://github.com/joyeecheung/node-core-utils/pull/21 , still have a few todos (e.g. incorporate .mailmap) but would love to see people give it a try and see if there are any edge cases that this tool does not cover.

vsemozhetbyt · 2017-11-03T23:45:58Z

Closing for now as nodejs/automation is evolving inspiringly.

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Oct 21, 2017

vsemozhetbyt added the meta Issues and PRs related to the general management of the project. label Oct 21, 2017

maclover7 reviewed Oct 21, 2017

View reviewed changes

maclover7 approved these changes Oct 21, 2017

View reviewed changes

gireeshpunathil approved these changes Oct 21, 2017

View reviewed changes

camantigue mentioned this pull request Oct 21, 2017

test: common.fixturesDir --> common.fixtures #15976

Closed

4 tasks

Trott reviewed Oct 22, 2017

View reviewed changes

gibfahn approved these changes Oct 22, 2017

View reviewed changes

meta: note re author check in COLLABORATOR_GUIDE

a3d479d

Refs: #16339 (comment)

vsemozhetbyt added the discuss Issues opened for discussions and feedbacks. label Oct 23, 2017

vsemozhetbyt mentioned this pull request Oct 23, 2017

src: operator[] checks bounds in debug mode #16002

Closed

2 tasks

vsemozhetbyt closed this Nov 3, 2017

vsemozhetbyt deleted the doc-check-author branch November 3, 2017 23:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

meta: note re author check in COLLABORATOR_GUIDE #16360

meta: note re author check in COLLABORATOR_GUIDE #16360

vsemozhetbyt commented Oct 21, 2017

maclover7 Oct 21, 2017

vsemozhetbyt Oct 21, 2017

maclover7 Oct 21, 2017

vsemozhetbyt Oct 21, 2017

maclover7 left a comment

lpinca commented Oct 21, 2017 •

edited

Loading

apapirovski commented Oct 21, 2017

vsemozhetbyt commented Oct 21, 2017 •

edited

Loading

vsemozhetbyt commented Oct 21, 2017 •

edited

Loading

joyeecheung commented Oct 21, 2017

apapirovski commented Oct 21, 2017

vsemozhetbyt commented Oct 21, 2017

joyeecheung commented Oct 21, 2017 •

edited

Loading

apapirovski commented Oct 21, 2017 •

edited

Loading

Trott Oct 22, 2017

vsemozhetbyt Oct 22, 2017

joyeecheung commented Oct 22, 2017

gibfahn commented Oct 22, 2017

joyeecheung commented Oct 22, 2017 •

edited

Loading

gibfahn commented Oct 22, 2017 •

edited

Loading

jasnell commented Oct 22, 2017

vsemozhetbyt commented Oct 23, 2017

joyeecheung commented Oct 31, 2017

vsemozhetbyt commented Nov 3, 2017

meta: note re author check in COLLABORATOR_GUIDE #16360

meta: note re author check in COLLABORATOR_GUIDE #16360

Conversation

vsemozhetbyt commented Oct 21, 2017

Checklist

Affected core subsystem(s)

maclover7 Oct 21, 2017

Choose a reason for hiding this comment

vsemozhetbyt Oct 21, 2017

Choose a reason for hiding this comment

maclover7 Oct 21, 2017

Choose a reason for hiding this comment

vsemozhetbyt Oct 21, 2017

Choose a reason for hiding this comment

maclover7 left a comment

Choose a reason for hiding this comment

lpinca commented Oct 21, 2017 • edited Loading

apapirovski commented Oct 21, 2017

vsemozhetbyt commented Oct 21, 2017 • edited Loading

vsemozhetbyt commented Oct 21, 2017 • edited Loading

joyeecheung commented Oct 21, 2017

apapirovski commented Oct 21, 2017

vsemozhetbyt commented Oct 21, 2017

joyeecheung commented Oct 21, 2017 • edited Loading

apapirovski commented Oct 21, 2017 • edited Loading

Trott Oct 22, 2017

Choose a reason for hiding this comment

vsemozhetbyt Oct 22, 2017

Choose a reason for hiding this comment

joyeecheung commented Oct 22, 2017

gibfahn commented Oct 22, 2017

joyeecheung commented Oct 22, 2017 • edited Loading

gibfahn commented Oct 22, 2017 • edited Loading

jasnell commented Oct 22, 2017

vsemozhetbyt commented Oct 23, 2017

joyeecheung commented Oct 31, 2017

vsemozhetbyt commented Nov 3, 2017

lpinca commented Oct 21, 2017 •

edited

Loading

vsemozhetbyt commented Oct 21, 2017 •

edited

Loading

vsemozhetbyt commented Oct 21, 2017 •

edited

Loading

joyeecheung commented Oct 21, 2017 •

edited

Loading

apapirovski commented Oct 21, 2017 •

edited

Loading

joyeecheung commented Oct 22, 2017 •

edited

Loading

gibfahn commented Oct 22, 2017 •

edited

Loading