Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tools: change inactive limit to 12 months #52425

Merged

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Apr 8, 2024

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

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Apr 8, 2024
@mhdawson
Copy link
Member

mhdawson commented Apr 8, 2024

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

@anonrig
Copy link
Member Author

anonrig commented Apr 8, 2024

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
Copy link
Member

panva commented Apr 8, 2024

The proposed change will affect 17 collaborators.

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

@mhdawson
Copy link
Member

mhdawson commented Apr 8, 2024

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
Copy link
Member Author

anonrig commented Apr 8, 2024

@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
Copy link
Member Author

anonrig commented Apr 8, 2024

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
Copy link
Member

tniessen commented Apr 9, 2024

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.

@tniessen tniessen added the meta Issues and PRs related to the general management of the project. label Apr 9, 2024
@GeoffreyBooth
Copy link
Member

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
Copy link
Member

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
Copy link
Member

joyeecheung commented Apr 9, 2024

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
Copy link
Contributor

aduh95 commented Apr 9, 2024

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
Copy link
Member

tniessen commented Apr 9, 2024

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.

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

aduh95 commented Apr 9, 2024

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
Copy link
Member

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?

@MoLow MoLow linked an issue Apr 10, 2024 that may be closed by this pull request
@anonrig anonrig added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Apr 10, 2024
@ruyadorno ruyadorno changed the title tools: change inactive limit to 6 months tools: change inactive limit to 12 months Apr 10, 2024
@targos
Copy link
Member

targos commented Apr 10, 2024

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

@anonrig anonrig force-pushed the update-inactive-collaborators branch from dd5ce3d to 4217324 Compare April 10, 2024 15:32
@anonrig
Copy link
Member Author

anonrig commented Apr 10, 2024

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

Done @targos

@anonrig anonrig added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 10, 2024
@MoLow
Copy link
Member

MoLow commented Apr 10, 2024

CC @nodejs/tsc

@tniessen
Copy link
Member

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

@anonrig anonrig force-pushed the update-inactive-collaborators branch from 4217324 to cec8474 Compare April 10, 2024 15:56
@anonrig
Copy link
Member Author

anonrig commented Apr 10, 2024

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

You're right. Updated GOVERNANCE.md @tniessen

@anonrig anonrig added the fast-track PRs that do not need to wait for 48 hours to land. label Apr 10, 2024
Copy link
Contributor

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

@anonrig
Copy link
Member Author

anonrig commented Apr 10, 2024

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

@aduh95 aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. and removed fast-track PRs that do not need to wait for 48 hours to land. labels Apr 10, 2024
@aduh95
Copy link
Contributor

aduh95 commented Apr 10, 2024

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

@targos
Copy link
Member

targos commented Apr 10, 2024

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
Copy link
Contributor

aduh95 commented Apr 10, 2024

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 nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 10, 2024
@nodejs-github-bot nodejs-github-bot merged commit 423ad47 into nodejs:main Apr 10, 2024
24 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 423ad47

marco-ippolito pushed a commit that referenced this pull request May 2, 2024
PR-URL: #52425
Reviewed-By: Ruy Adorno <ruy@vlt.sh>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
marco-ippolito pushed a commit that referenced this pull request May 3, 2024
PR-URL: #52425
Reviewed-By: Ruy Adorno <ruy@vlt.sh>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal to reduce inactive collaborator duration