samtools / htsjdk

A Java API for high-throughput sequencing data (HTS) formats.
http://samtools.github.io/htsjdk/
285 stars 243 forks source link

Pruning list of people with admin & write access to the project #923

Open tfenne opened 7 years ago

tfenne commented 7 years ago

After much discussion in issue #871 we are moving to a model whereby the project has three official maintainers (myself, @lbergelson & @jacarey) who are charged with defining governance and processes for the project.

In order to enact anything we first need to make sure that permissions within the project are sensible. Previously the default was to give anyone at Broad who needed/wanted it write access to the project. This has led to a large number of people with write access (not all, but mostly Broad folks) who are not active within the project and haven't been for some time. This is absolutely not an indication that contributions are unwelcome from these users (or anyone else) but that we'd prefer a smaller group have write access and that others fork the project on GitHub much as you would with any other project, and make PRs from your own fork.

We'd like to remedy this by doing the following:

  1. Remove all but the three new maintainers from the admin team. That means removing @alecw, @nh13, @ktibbett and @bradtaylor from the admin team.
  2. Removing the following users from the developers team, which will lead to loss of write access to the project (no pushing, merging, assigning of issues etc. but still able to create issues & PRs and comment on things): @abaumann, @alecw, @bradtaylor, @bw2, @davidbenjamin, @eitanbanks, @EllenEWinchester, @featureunicorn, @gbggrant, @geoffjentry, @hensonc, @jmthibault79, @kbergin, @kshakir, @ktibbett, @ldgauthier, @ronlevine, @snovod, and @vruano.

This will leave the following users with write access within the project:

Once the project permissions have been updated we'll also be setting master as a protected branch so that only project maintainers can merge PRs to master. It will then be on the maintainers to both actively review PRs and to request reviews from other members of the project and ultimately make the merge decision based on those reviews.

Hopefully no-one is offended by this or finds it too draconian. We'll be making these changes soon but are happy to adjust if something isn't working!

droazen commented 7 years ago

"Once the project permissions have been updated we'll also be setting master as a protected branch so that only project maintainers can merge PRs to master. "

Having only 3 people able to merge PRs into master is not going to be practical/workable for us, I'm afraid! We often require urgent fixes at the htsjdk level, and 3 people is far too small of a pool from which to draw to get a needed fix in.

Instead, we'd prefer that anyone with write access would be able to merge PRs into master, as in the past, and that governance by admins be confined to dispute resolution rather than heavy-handed micro-management of every change that goes into master.

I'll discuss with @lbergelson when he gets back from vacation in a week -- for now I'd request that these new policies be put on hold pending further discussion.

tfenne commented 7 years ago

@droazen Just so you're aware - these changes were discussed in detail between @lbergelson, @jacarey and myself, and we are all in agreement that this is the way to go.

In addition there's nothing in the proposed changes that would stop you or anyone else with write access from creating a gatk_develop branch that mirrors master, using that for your own purposes and then PR'ing into master.

eitanbanks commented 7 years ago

I'm okay trying this new system out for a little while, provided the admins are willing to reassess in a few months and make sure the community remains happy with it. Is that cool?

vdauwera commented 7 years ago

I second @droazen's opinion that having only 3 people with merge access is too restrictive. It's ridiculously easy to end up in a situation where one is sick, one is on vacation and one is working on an urgent deadline, and suddenly no one is available to administer master.

Having to switch to using branches instead of master can cause process disruption at several levels. That's not always going to be a practical solution.

What is the problem that this restriction is trying to fix, and is it not addressed by the substantial slimming down of the write team in the first place?

droazen commented 7 years ago

I am trying to contact @lbergelson now -- I know from my conversations with him in the past that he would never support instituting a 3-person chokepoint for all merges. That would be crazy and completely unworkable for our dev team.

jacarey commented 7 years ago

From the production pipelines perspective I am much more in favor of controlled access to merges to master. The fact that we couldn't want for the approval of 3 maintainer/admins for "emergency" or urgent releases is troubling to me in and of itself (especially if it is so frequent that we feel this is unworkable without even trying it). For us performance and stability is paramount.

droazen commented 7 years ago

@jacarey Access could still be controlled indirectly by admins, since they choose who gets to have write access. Widening the pool of those with merge permission slightly would eliminate the "chokepoint" nightmare scenario where everyone is on vacation or otherwise unavailable when a critical regression is discovered.

We cannot release GATK to maven central with a dependency on an htsjdk snapshot/branch, so this scenario would be a potential blocker for urgent bug-fix releases. We need someone on our side who is always able to patch htsjdk when necessary -- and Louis is not always around. @lbergelson understands this very well, which is why I don't believe that this proposal actually has the approval of all 3 maintainers.

vdauwera commented 7 years ago

Reducing to 3 just seems overly extreme. It seems to me that we could do a first pass of moderate restriction, and if that doesn't work, restrict further.

Nothing prevents production from using a protected branch in order to ensure maximal stability without limiting the rate of progress in other projects.

tfenne commented 7 years ago

The whole point of going through the process of deciding on a set of core maintainers was precisely to enable a small group to make an investment of effort and in doing so generate and enforce a set of processes, governance procedures and guiding documents for the project. Everyone had the opportunity to contribute to that discussion. A decision was made and now we all get to live with that decision, even if it is not the exact solution any individual preferred.

I would describe HTSJDK in it's current state as a repository for code that anyone who had access found convenient to put here. There is a lot of technical debt, and most of that occurred due to the wide range of committers, lack of standards and processes, lack of clear leadership and a general free-for-all. The quality of the project is in decline and will continue to decline unless it is actively managed.

Active management is work, and not a small amount of work. It is significantly more work if anyone with write access to the repository is not enforcing the same set of processes and standards as everyone else. Merging to master is a privilege, not a right. That privilege comes with a responsibility, and a workload. It is unfair to expect one without the other, especially since it creates more work for others. I am not at all opposed to, in time, adding more maintainers so that the list is longer, but those maintainers need to come a) after we've had time to write the first drafts of processes & standards and b) with compatible goals, expectations etc. to the existing maintainers.

My opening this issue seems to have been interpreted as the asking for permission to do this. This is not the case. @jacarey, @lbergelson and myself met and discussed this and came to an agreement that this is the way we will handle things for now. We will, of course, be sensitive to your concerns and if this is shown to be unworkable then we will correct that. We all agreed to try this model with three initial maintainers. Instead of second-guessing, I would hope that this community would give us space to try what we think best, knowing that we are trying to improve an important shared resource for everyone, and trust that we'll adjust as we go if things are not working.

yfarjoun commented 7 years ago

FWIW, I don't think that this is so draconian. We have many external collaborators that use this mechanism to get their PR's in. Perhaps if we all had to go through the same process it would work a little better.

To make htsjdk better we need careful review and the current status of effective free-for-all (for those of privilege) is part of what we are trying to resolve. It's also good to have a small set of folks that have some-what of a complete understanding of the entire codebase.

The fact that GATK has gotten accustomed to only running off of master (and having a free hand at merging) cannot be a reason for htsjdk to be run in a particular way, there are many other projects that use htsjdk and it needs to be catering to all of them.

We have 3 maintainers who have agreed unanimously on a way forward, let's try this out for a few months, see what works and what doesn't work, and then discuss.

On Thu, Jun 29, 2017 at 11:02 AM, Tim Fennell notifications@github.com wrote:

The whole point of going through the process of deciding on a set of core maintainers was precisely to enable a small group to make an investment of effort and in doing so generate and enforce a set of processes, governance procedures and guiding documents for the project. Everyone had the opportunity to contribute to that discussion. A decision was made and now we all get to live with that decision, even if it is not the exact solution any individual preferred.

I would describe HTSJDK in it's current state as a repository for code that anyone who had access found convenient to put here. There is a lot of technical debt, and most of that occurred due to the wide range of committers, lack of standards and processes, lack of clear leadership and a general free-for-all. The quality of the project is in decline and will continue to decline unless it is actively managed.

Active management is work, and not a small amount of work. It is significantly more work if anyone with write access to the repository is not enforcing the same set of processes and standards as everyone else. Merging to master is a privilege, not a right. That privilege comes with a responsibility, and a workload. It is unfair to expect one without the other, especially since it creates more work for others. I am not at all opposed to, in time, adding more maintainers so that the list is longer, but those maintainers need to come a) after we've had time to write the first drafts of processes & standards and b) with compatible goals, expectations etc. to the existing maintainers.

My opening this issue seems to have been interpreted as the asking for permission to do this. This is not the case. @jacarey https://github.com/jacarey, @lbergelson https://github.com/lbergelson and myself met and discussed this and came to an agreement that this is the way we will handle things for now. We will, of course, be sensitive to your concerns and if this is shown to be unworkable then we will correct that. We all agreed to try this model with three initial maintainers. Instead of second-guessing, I would hope that this community would give us space to try what we think best, knowing that we are trying to improve an important shared resource for everyone, and trust that we'll adjust as we go if things are not working.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/samtools/htsjdk/issues/923#issuecomment-311994716, or mute the thread https://github.com/notifications/unsubscribe-auth/ACnk0kGjScgOQ1fEf21R1tOeKzjHaIxdks5sI7yggaJpZM4OJLFt .

droazen commented 7 years ago

We still have to hear from @lbergelson, who is currently away on vacation. I've contacted him to relay my concerns that 3 people with merge access is simply not enough.