tools: change inactive limit to 12 months

[anonrig]

I'm proposing reducing the inactive collaborator contribution limit.

Referencing nodejs/TSC#1524, I recommend reducing the inactive collaborator duration to 12 months from 18 months.

cc @nodejs/tsc

[mhdawson]

I think 6 months is too short, but would be ok with 12 months

[anonrig]

I think 6 months is too short, but would be ok with 12 months

@mhdawson Can you elaborate? I'd like to find the middle ground between 6 and 12 months.

[panva]

The proposed change will affect 17 collaborators.

Just to put the total in a relative term too, that's ~17% of current collaborators.

[mhdawson]

adding comment here as well as original discussion issue:

@anonrig I can easily see people taking a break (for example to care for a new child etc.) who will come back getting removed by the 6 month check. A 12 month check makes this a lot less likely in my opinion.

[anonrig]

@anonrig I can easily see people taking a break (for example to care for a new child etc.) who will come back getting removed by the 6 month check. A 12 month check makes this a lot less likely in my opinion.

@mhdawson I understand but they can always be added back. There is no limiting factor for us to re-add a ex-collaborator as far as I know. I think we should ask ourselves how we feel about having a contributor with 11 months and 29 days inactivity blocking a highly important (or not) pull-request. In my honest opinion, I think 6 months is already a long duration to forget a large portion of a highly active project code, but it wouldn't be in our favor to lose half of our contributors in a single pull-request.

[anonrig]

Just to put the total in a relative term too, that's ~17% of current collaborators.

@panva Thanks. I've added it to the pull-request description.

[tniessen]

I agree with @mhdawson. I do understand the security argument made in nodejs/TSC#1524.

[...] a contributor with 11 months and 29 days inactivity blocking a highly important (or not) pull-request [...] In my honest opinion, I think 6 months is already a long duration to forget a large portion of a highly active project code

Has this happened? I don't really see how the period of inactivity is relevant in these cases. Collaborators might still have valuable insights and might block based on design decisions that don' require an up-to-date understanding of "highly active project code", and in any case, I certainly don't expect every collaborator to keep up with changes to our code base.

[GeoffreyBooth]

The security risk is related to the commit bit, right? So why not just get rid of the commit bit for everyone except a small subset, like the release team? I haven't used the commit bit in years; I rely on the commit-queue label. The rest of us could do the same, and that would improve security, if the concern is the proliferation of the commit access.

[joyeecheung]

I think the commit queue + GitHub bot still goes down/malfunctions frequent enough that we need a fallback when they stop working. It also doesn’t help that the automation isn’t actively maintained by anyone, and the CI is still too flaky (so whenever the automation fails you need to multiple the problem with a lot more CI runs). We should at least have a more stable group of volunteers that look after the automation before considering fully relying on it.

[joyeecheung]

I think we should ask ourselves how we feel about having a contributor with 11 months and 29 days inactivity blocking a highly important (or not) pull-request.

I think in terms of decision making whether a contributor is an active collaborator or not does not make that much a difference. Even if a collaborator has already been inactive in the project for, say, 10 years, if they have strong opinions about a pull request then we should still consider their views and find a compromise. If a compromise cannot be made then we can still go with a TSC vote, and the inactive collaborator (or they don’t even need to be a past collaborator) can still present their perspectives to the TSC before we make an informed decision.

[aduh95]

Regarding who is using the commit bit, excluding release commits and onboarding commits:

echo "$(
    git log --since='1 year ago' --pretty=format:'%H$%aN$%cN$%s' | \
      awk -F'$' '{ if($3 != "GitHub" && !match($4, /^202[0-9](-[0-9]{2}){2}, Version/) && !match($4, /^doc: add [^ ]+ (as a collaborator|to collaborators)$/)) {print $3} }'
  )\n$(
    gh pr list --state merged --limit 9999 --json mergedBy,mergeCommit,headRefOid  --base main \
      --search "merged:>$(date -v-1y -Idate)" \
      --jq 'map(select(.headRefOid != .mergeCommit.oid)) | map(.mergedBy.name) | join("\n")'
  )" | sort | uniq -c

Results at the time of writing

   2 Alex Yang
  46 Antoine du Hamel
   1 Chemi Atlow
   5 Cheng Zhao
   9 Chengzhong Wu
   3 Colin Ihrig
   1 Danielle Adams
   1 Darshan Sen
   5 Debadree Chatterjee
   4 Gabriel Schulhof
   2 Gireesh Punathil
   1 Guy Bedford
  20 James M Snell
   1 Jithil P Ponnan
  62 Joyee Cheung
   3 Keyhan Vakil
   1 Khaidi Chu
   6 LiviaMedeiros
  21 Luigi Pinca
  57 Michael Dawson
  84 Michaël Zasso
  14 Moshe Atlow
1187 Node.js GitHub Bot
   1 Paolo Insogna
  39 RafaelGSS
  13 Rich Trott
   3 Richard Lau
   1 Shelley Vohr
   5 Yagiz Nizipli

That's 30 of 98 (non-bot) collaborators, so about a third.

In my experience, the commit bit is useful when:

• There are phantom CI failures which make the CQ impotent (sometimes the GitHub UI reports a Jenkins failures, but Jenkins show the run as green/yellow).
• To "purple-merge" a multi-commit PR (the CQ would "red-merge" those).
• To amend the commit message (the CQ always use the first commit message, but sometimes that's not appropriate).
• Something was pushed on main by mistake.

N.B.: This list might be quite inaccurate as it's relatively easy to impersonate someone with git because we don't enforce signatures. The only reliable way would be to ask GitHub support I think. It also doesn't show use of the GitHub UI/API, which I have used certainly more than 10 times in the previous year.

[tniessen]

The security risk is related to the commit bit, right? So why not just get rid of the commit bit for everyone except a small subset, like the release team?

That's one concern, and I do think there's value in being able to force-push to remove the most recent commit. Aside from that, the commit bit is not the only security concern. For example, being able to start Jenkins CI jobs might be sufficient for compromising some of our CI infrastructure.

[joyeecheung]

Does this script actually move people out of the collaborators team? From a glance of it it seems to be only moving items from one section to another section in the readme?

[aduh95]

Does this script actually move people out of the collaborators team? From a glance of it it seems to be only moving items from one section to another section in the readme?

The script opens a PR editing the README. Once the PR has merged, it's the responsibility of TSC members to ensure the offboarding checklist has been completed.

[GeoffreyBooth]

That’s 30 of 98 (non-bot) collaborators, so about a third.

And the other two thirds are active, so by definition they’re landing commits, just via the bot?

Could we just make the commit bit opt-in, then? Active collaborators who want it can get it, but those who prefer to use the bot can forgo the commit bit and remove one potential attack vector?

[targos]

Please amend the commit message, or it will say 6 months

[anonrig]

Please amend the commit message, or it will say 6 months

Done @targos

[MoLow]

CC @nodejs/tsc

[tniessen]

I'd assume this also needs a documentation update? (On mobile, otherwise I'd check myself.)

[anonrig]

I'd assume this also needs a documentation update? (On mobile, otherwise I'd check myself.)

You're right. Updated GOVERNANCE.md @tniessen

[github-actions]

Fast-track has been requested by @anonrig. Please 👍 to approve.

[anonrig]

This pull-request was discussed on TSC meeting today. That's why I'm requesting fast-track upon suggestion.

[aduh95]

The PR has been opened for more than 48 hours, fast tracking can't make it land any faster :)

[targos]

The PR has been opened for more than 48 hours, fast tracking can't make it land any faster :)

It's true, but I would consider it rude tu make significant changes to a PR and land it quickly (without notice) because it was already open before.

[aduh95]

The PR has been opened for more than 48 hours, fast tracking can't make it land any faster :)

It's true, but I would consider it rude tu make significant changes to a PR and land it quickly (without notice) because it was already open before.

That makes sense, but in this case, Yagiz has complied with all the proposed changes, it seems fair to me to assume if someone had a problem with the 12 months proposal, they would have had enough time to express that.

[nodejs-github-bot]

Landed in 423ad47