git-node does not include reviews from other teams

[ChALkeR]

E.g. at nodejs/node#21914, @nodejs/security-wg was cc-d, @SomeoneWeird (a member of @nodejs/security-wg) reviewed the change, but on landing with node-core-utils that review was not mentioned in the commit message.

@nodejs/tsc — btw, is there any good reason to not mention all the reviews from org members? Or just all the reviews from all the GitHub users?

I suppose that something like this could also happen with reviews from other WGs.

/cc @mcollina @nodejs/tsc

[mcollina]

I think because @SomeoneWeird is not a Node core collaborator. We should probably include reviews from all people in the org.

I’ve probably used an oldish version as well.

[joyeecheung]

Only collaborators in the README are included in the Reviewed-By field - because we only have those people's names and emails. It is possible to query names and emails from people's GitHub profile, but that does not always work as a lot of people simply don't put that in their profile, or use a no-reply email there.

[targos]

This is probably something we need to discuss in the core repo or with the TSC first. One of the privileges of the collaborators is to sign off the reviews and we nominated them because we trust them for their reviews. While it's good to have external input, I'm not sure we want to consider it the same as if it was from a collaborator.

[SomeoneWeird]

I agree, while not being an official core collaborator, it was more of a +1 on behalf of the security-wg who was pinged on the issue.

[ChALkeR]

@targos Yes, only collaborators could sign off the PR so that it could be landed, i.e. PRs can't land with out being reviewed by collaborators. I do not propose to change that, ncu should still require collaborator review before landing.

The thing that I am talking about is that reviewes by non-collaborators are not mentioned at all, but I think that they should be.

[joyeecheung]

@ChALkeR I personally see the Reviewed-By field more about accountability instead of attribution, though. But in the end if someone wants to thank non-collaborators for their reviews, they can always just say so in the commit message (via a Reviewed-By field or not, I'd prefer not using that field though), but it seems a bit out of scope for ncu.

Also for PRs that receive more traction than usual, including reviewers who don't have write access to the repo will result in a less readable metadata list. Considering some people approve PRs to show endorsement without actually looking into the code and review it thoroughly, this makes the field less useful because you cannot hold those reviewers accountable (that is, you cannot count on them to spend the time to fix regressions or open backports).

[github-actions]

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.