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

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

Open
tfenne opened this issue Jun 29, 2017 · 11 comments
Open

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

tfenne opened this issue Jun 29, 2017 · 11 comments

Comments

@tfenne
Copy link
Member

tfenne commented Jun 29, 2017

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

droazen commented Jun 29, 2017

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

tfenne commented Jun 29, 2017

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

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

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

droazen commented Jun 29, 2017

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

jacarey commented Jun 29, 2017

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

droazen commented Jun 29, 2017

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

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

tfenne commented Jun 29, 2017

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

yfarjoun commented Jun 29, 2017 via email

@droazen
Copy link
Contributor

droazen commented Jun 29, 2017

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants