nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
106.64k stars 29.07k forks source link

meta: create subteams for inactive and active collaborators #20367

Closed joyeecheung closed 5 years ago

joyeecheung commented 6 years ago

Refs: https://github.com/nodejs/node/issues/18879 Refs: https://github.com/nodejs/build/issues/744

There are currently 33 collaborators who have not authored a commit on master for more than a year, 9 who have not authored a commit on master for more than 2 years. There have been quite a few changes to our process during the last two years so if they want to use the commit or CI access again, chances are they will need to refresh their knowledge to avoid mistakes. Also granting access to inactive collaborators may pose a security risk. On the other hand a lot of them are still very active on the issue tracker and helping out so it does not make sense to move them to Emeritus or out of the collaborators team either - it doesn't do their service justice and they will no longer receive the notification.

I propose that we create two subteams under @nodejs/collaborators , one for active collaborators and one for inactive collaborators. We move people between these two teams periodically using automation tools. The active team will have the write access to the repo and CI access. Whoever wants their access back can request it in the private discussion page of the collaborators team. The criteria of "being active" can be something like "have posted anything in the nodejs/node issue tracker in the last three month".

Any thoughs?

cc @nodejs/tsc @Trott

benjamingr commented 6 years ago

There are currently 33 collaborators who have not authored a commit on master for more than a year, 9 who have not authored a commit on master for more than 2 years

That is not the only criteria for contribution by a collaborator. Triaging issues, reviewing pull requests, landing and participation in discussions are some others I can name on top of my head.

We also have people who mostly participate on other repos.

The fact this has never been a problem only further motivates not wanting to alienate or exclude certain collaborators.


What I'm OK with on the other hand is actively reaching out to inactive collaborators and asking them if they'd still like to be a collaborator and participate or they'd like to step down. @Trott did this which moved several collaborators to Emirati status. Trott said he'll do this every year.

joyeecheung commented 6 years ago

@benjamingr I agree commits are not the only criteria for contribution. That's why I pointed out in the OP:

On the other hand a lot of them are still very active on the issue tracker and helping out so it does not make sense to move them to Emeritus or out of the collaborators team either - it doesn't do their service justice and they will no longer receive the notification.

Also I am not suggesting to moving people out of the collaborators team. I am suggesting creating subteams, one with access and one without. Whoever wants it can simply ask to be moved to the active collaborators team to gain access. Only collaborators know about the membership and I don't think it's alienating if all they need to do is post something in the private page to regain access.

joyeecheung commented 6 years ago

@benjamingr

What I'm OK with on the other hand is actively reaching out to inactive collaborators and asking them if they'd still like to be a collaborator and participate or they'd like to step down. @Trott did this which moved several collaborators to Emirati status. Trott said he'll do this every year.

BTW this proposal stems exactly from the discussion around this action for this year as an alternative or supplement, because out of the candidates to be asked, some of them match the description that I quoted in the last reply.

ryzokuken commented 6 years ago

+1 to this as long as the process of getting back write access (getting transferred to the "active" team) is fairly simple and straightforward, in which case, this would take pretty much the same amount of time and effort on the part of the "inactive" contributor (reaching out to another team member in order to refresh their know-how) while still taking care of our potential security risk.

Personalized mini-onboarding sessions, anyone?

Qard commented 6 years ago

I like the idea, and so would also suggest that when regaining commit access one would go through a quick re-training.

I know in my case I mainly just contribute to the diagnostics working group and have probably forgotten bits of the process for contributing directly to nodejs/node or missed changes in the workflow. I feel like I wouldn’t be very confident I was doing the right thing if I tried to just go and land a PR right now.

benjamingr commented 6 years ago

Keep in mind - people who do reviews need to be able to start the CI for those. That doesn't show up in the commit log.

benjamingr commented 6 years ago

I feel like I wouldn’t be very confident I was doing the right thing if I tried to just go and land a PR right now.

If you do exactly what you did 2 years ago - it would be absolutely fine. We still pull from master, apply the PR as a patch, squish and adapt to format and push. The fun part is that there's an automated tool that takes care of more of the boilerplate for us (like the Reviewed-By message stuff)

richardlau commented 6 years ago

I feel like I wouldn’t be very confident I was doing the right thing if I tried to just go and land a PR right now.

This is why we write up the process so we can refer to it later, and why it's important that we keep the documented process accurate and up to date.

joyeecheung commented 6 years ago

Keep in mind - people who do reviews need to be able to start the CI for those. That doesn't show up in the commit log.

@benjamingr I was not suggesting to use the commits as the criteria of "being active".

The criteria of "being active" can be something like "have posted anything in the nodejs/node issue tracker in the last three month".

I am pretty sure people who do reviews and need the access would at least show up in this issue tracker from time to time - even if they have not showed up at all in months and suddenly need access, all they need to do is just post something in the private discussion page.

joyeecheung commented 6 years ago

If you do exactly what you did 2 years ago - it would be absolutely fine. We still pull from master, apply the PR as a patch, squish and adapt to format and push. The fun part is that there's an automated tool that takes care of more of the boilerplate for us (like the Reviewed-By message stuff)

@benjamingr

Yes, the patch format has not changed much, but the criteria of when a patch is ready to land changes all the time - even the new author ready label does not imply that the patch is actually ready to land. The git-node tool can hopefully do enough checks but it may make mistakes in edge cases as well (although since it's code you can PR with a test to make sure it does not happen again, whereas it's hard to do the same thing for a human).

Also the proposal it not just about refreshing the knowledge, it's also about the principle of least privilege. For example in a in-production service you would not want to give unlimited write access to everything on servers to everybody in the team, or give access that does not expire when it's not renewed. It does not even matter if the person is active in the design or implementation of the service - if they do not need the access in sufficient amount of time, then access should expire, but the process of granting new access should be fairly trivial for people who are trusted, so that it does not get in their way of doing their job. And being temporarily without access does not mean they are not part of the team anymore, because it's the fact that it's trivial for them to renew access that implies they are trusted and part of the team.

benjamingr commented 6 years ago

For example in a in-production service you would not want to give unlimited write access to everything on servers to everybody in the team, or give access that does not expire when it's not renewed.

I agree in principle though keep in mind that members (or even owners) don't actually have any potential to cause damage (other than to the CI) except for people who build node from master.

Scary permissions are:

I'm not sure the benefit of keeping a tight ship for collaborators outweighs alienating a collaborator every now and then.

When we did something similar in the modules team (move members to observer - still part of the team after no participation for a while) some people were furious.

Trott commented 6 years ago

Regarding comments along the lines of "what's the harm of leaving people with push access?": The CI access is the bigger issue. It should not be minimized. Although we've not (yet) had a problem there, let's think for a moment about what happens if we do have an incident. I want to be able to say that we took reasonable steps to prevent it. I do not want to have someone correctly say "You chose to not bother taking reasonable precautions." Leaving people with CI access when they have not needed it for literally years can fairly be criticized as irresponsible IMO.

When we did something similar in the modules team (move members to observer - still part of the team after no participation for a while) some people were furious.

Yeah, we should definitely look at what happened there and try to make sure this proposal wouldn't run into the same issues. I don't think it would because the timelines are longer and stuff like that, but it's worth looking at more closely. (I didn't follow the modules brouhaha too closely but I'm somewhat familiar with it.)

Overall, I like this proposal if someone is willing to do the work to implement it.

benjamingr commented 6 years ago

Worth mentioning from the modules WG is that what ended up working is reaching out to people in person and asking them to consider stepping down. I think we should start with that before we start doing things that might be perceived as alienating collaborators.

lance commented 6 years ago

A perspective from an intermittent contributor...

As someone who loves being a Collaborator on Node.js, but who is only an intermittent committer, I have wondered how long it would be before my status changed. I don't think it's ever been a full year between landing commits, but certainly months have passed. During those quiet periods, I am still actively skimming (can you actively skim?) nodejs/* notifications from GitHub. That's how I saw this issue. Typically, I am llooking out for issues that may immediately affect my work at Red Hat. But often, I'm just much too busy to spend more than a little time every day or so doing this - thus my occasional long hiatus.

Being a Collaborator means a lot to me - both personally and for my job. If my status were to change I would be upset about it, but not devastated. I think the most important thing here is communication and @benjamingr is correct to say that reaching out personally to individual Collaborators early on is critical. Waking up to find out I no longer had commit rights without being told personally would be confusing and alienating.

Along the same lines, if there is a policy put into place, I think it would be good to include this information in the onboarding guide so that new, incoming folks are aware.

My 2¢ for the day.

benjamingr commented 6 years ago

Thanks for weighing in @lance 🤗

No pressure, contributing should be fun. I personally would be against any policy change that one-sidedly removes collaborators without communicating with them first and asking them about it.

For what it's worth I think consensus was that we should automate contributions through the CI and work towards a better pipeline so collaborators wouldn't have to git push origin master at all. Personally I lean towards wanting to blur the boundaries of what's "core" - there shouldn't be levels or rankings of contribution level - just people working on interesting stuff they care about.

A nice way to contribute that is low-stress and doesn't require a ton of ongoing commitment is reviewing and landing PRs and fixing flakey tests. These are one time thing that doesn't take a ton of commitment per-item but it helps a lot. PRs aren't the only measure.

joyeecheung commented 6 years ago

I understand the sentiment from @lance , this proposal is just an alternative to automating the CI and commit landing - if we do not need human to do that, then there's no problem of leaving commit bits around. The remaining problem would be:

  1. The project still is not getting the help that it seems to have: we list > 100 people in the README, but only about half of them showed up on the issue tracker in the past week, and most collaborators don't do much triage work. AFAIK a lot of our users are not aware that most people listed there are volunteers who don't get paid by working on this project, so it's totally OK for them to not show up in months. That means for someone opening issues or PRs in the project for the first time, this gives them the false impression that out of those >100 people at least someone would jump in and respond in one or two days, but that's not always the case, and there are many PRs stalled because of lack of reviews or because there are just no collaborators keeping an eye on it (launching CIs, .etc). This can probably be resolved by just deleting the list from README and attribute individual contributions for each release in blog posts/change logs.
  2. Someone would need to step up and actually make the automation happen. Having consensus on using automation to do the work is not enough. Again, if I were new to this project, I would expect that this is not an issue if we have >100 people listed in the README that seem to be actively contributing to the project, especially when there is a Collaborator Emeriti section for people who do not actively work on this project anymore. (I would neither expect that with that many people listed in a significant place in the README, the CI of this project is red at least 70% of the time!)

There are a lot of reasons as to why collaborators stop being active, but leaving the status quo as-is do not help with that.

mcollina commented 6 years ago

I think the list in the README is a critical part of the recognition of a Node.js developer. I do not think that moving it somewhere less prominent would be good for our community. It sets an example of recognizing the work of the individuals that have contributed to Node.js the most.

I think we should reach out to the individuals that have not contributed (in any form) for more than 1 year and ask them if they want to be moved to Emeritus. If they do not reply, they will be moved. I am ok in moving the list of Emeritus to a separate files (linked from the README).

I'm definitely +1 in making more prominent who contributed to a release. This is especially important for LTS lines.

joyeecheung commented 6 years ago

I think by listing people in the README it actually builds a wall between "people who are in the README" and "people who are not in the README", which is also part of the problem of our definition of collaborator: people would feel alienated if they are not an collaborator anymore while in fact we should make everybody, collaborator or not, feel welcomed when they want to weigh in. If not having the commit bit makes you feel you are less than others, then we have a problem with our culture.

Trott commented 6 years ago

This can probably be resolved by just deleting the list from README

Coincidentally, I chatted today with a developer (who does mostly Ember.js and is not involved in Node.js core at all) and, without prompting, they offered that by listing all the people in the README, it makes Node.js approachable and welcoming. They were explaining that if they wanted to talk to someone about the feasibility of getting a feature added to PHP core, they wouldn't know who to talk to and would probably be intimidated if they did. But with Node.js, even if you don't want to post publicly on GitHub or anything, we provide a list of over 100 names and email addresses. You can try to find someone to talk to privately by doing a web search a bit about a few people, or maybe try to find someone with a domain name in their email indicating that they work at the same company you do, or whatever. The fact that likely very few people ever actually do that doesn't matter. It still makes the project seem inviting and more transparent.

This was not something I had ever considered before (I don't think--if I have, I've forgotten that I had ever thought about it).

joyeecheung commented 6 years ago

@Trott But I don't think it makes that much difference if we put the list into a json file in the root directory and link to that in the README?

Also I wonder how come having a list of >100 names and emails help anyone if they want to add a feature to core. Do they send emails to every one of them? How would you know which one of them happen to work on the things that you want to add to core? It probably makes more sense if we have a list of subsystem team members and owners.

EDIT: Oh, I see, you can search for company emails. That makes sense, I've done that (and turns out no one from my ex-company use their company emails for that because we don't require CLA or CCLA), but does it make a difference if we put the list in a JSON?

mcollina commented 6 years ago

The fact that likely very few people ever actually do that doesn't matter. It still makes the project seem inviting and more transparent.

This happens to me from time to time.

Overall, I think that listing people in the readme is an emotional thing, and it makes Node.js special.

joyeecheung commented 6 years ago

I am actually surprised that other people find the list welcoming, because before I started contributing I saw that list as a statement of "this is a party, and you are invited". It wouldn't have mattered if there was no list (or at least not in a significant place like that), but because there was a list, there was a difference between people on the list and people who were not. Maybe that's just me coming from a background where most people just watch instead of participating because of the language barrier, so it could just be a culture shock.

benjamingr commented 6 years ago

I think we're getting a little off-topic with the README part, the two issues:

Aren't really too related - it appears that some people (me included) feel there is value in listing collaborators in the README.

I'd like to point out that I'd still really like the former addressed for the reasons Joyee has listed out:

I think the issue is mostly if someone gains access to an inactive collaborator's account. It's not a huge deal - but I do think we should formalise and empower the work @Trott has been doing and encourage inactive collaborators to either step up or step down (temporarily) until they are in a capacity to contribute.

Something like:

Trott commented 6 years ago

@benjamingr I appreciate the desire to formalize the process, but I'd propose that we don't do that until all the necessary tooling is in place. Otherwise, you run into situations where it's obvious someone should be moved to Emeritus but it stalls out on process. "Did you run a query for comments?" "Did you run a query for starting CI jobs?" "Did you run a query for reviewing PRs?" "Did you check all the repos in the org or just core?" "Did you look to see if they commented on the discussion boards? "Did you check to see if they were active in IRC?" Unless all the necessary things can be explicitly listed and there is a tool for checking each of them, I'd vastly prefer our current informal practice, which is basically "TSC decides once a year (or more often if they wish) who might be appropriate to move to Emeritus and someone from TSC emails those people asking if they think they should be moved."

benjamingr commented 6 years ago

Well, an informal process has more tendency for unfairness and bias but given we are dealing with people who volunteered their time to help us and a human touch helps I’m personally fine with it for this.

boneskull commented 6 years ago

@benjamingr @Trott

A good compromise might be to write down Ben's list as guidelines.

I'm not worried about unfairness if the legwork has been done, and we've found 0 PR's and 0 comments across the org over the past 6 months.

This only means "send an email to ask if they are still interested", after all.

I'm happy to send something for this, but unsure where it belongs.

Trott commented 6 years ago

I'm happy to send something for this, but unsure where it belongs.

If you mean tooling to automate the queries that need to be done, maybe write up the tooling and when you have something, we can move it into the org as its own repo? (And if that is what you mean, thanks for even wanting to do that work!)

boneskull commented 6 years ago

Nope. 🙂 Rather just to write up the guidelines.

joyeecheung commented 6 years ago

By the way there is https://github.com/nodejs/node-core-utils/pull/186 which queries the activity of a given id/the list of collaborators in the whole org (needs a rebase, though) with a sort-by-last-date added you can find out the list of people not showing up in the last n months/years

joyeecheung commented 6 years ago

Just looked at that branch, I probably pushed with ad-hoc changes at one point, to query activities in the whole org the query should be involves:${user} org:nodejs

Trott commented 5 years ago

It seems like perhaps this should be closed. Feel free to re-open (or leave a comment requesting that it be re-opened) if you disagree. (Or open a PR?) I'm just tidying up and not acting on a super-strong opinion or anything like that.