Binderhub: Procedure for reviews / merges

[choldgraf]

Hey all - we should come up with some guidelines for how to get commits merged. Now that BinderHub has grown, we need to make sure that decisions are made w/ community input and without jumping too far ahead of everybody.

I propose something like the following (as a starting point for discussion):

1. PRs that touch a non-trivial amount of code require 2 approvals. If there are 2 approvals then anybody can merge.
2. PRs that touch only documentation require 1 approval for merge.
3. PRs that touch a small amount of code (think bugfixes) and are critical require 1 approval to merge.
4. If on a PR anything other than points 1-3 is requested, that will supercede the rules above (e.g. requesting review from a specific person)

What do you guys think about this? If we agree on edits etc, I can change the top-level comment.

@willingc @yuvipanda @minrk

[yuvipanda]

Are there general guidelines written down somewhere that rest of the Jupyter project follow?

[willingc]

I prefer a much simpler set of general guidelines that I shared with @fperez and @ellisonbg:

• We will share all relevant data received with the entire team.
• We will be sure that each member has the opportunity to provide input to any decision that affects the team.
• I will not take sole credit for something the team did together.
• The entire team should be cc’d on all emails.
• We will refrain from self merging PRs unless open for more than 24 hours or is a disaster recovery situation.
• We will make an effort to submit detailed, actionable issues. Side discussions should be recapped.

The general guidelines of the Jupyter project have been to encourage reviews, discourage self merges unless necessary (i.e. recovery from an outage, has been open over 48 hours without comment/review), and respect the input of all team members.

cc: @minrk

[yuvipanda]

+1 to what @willingc said.

[yuvipanda]

I think it would be great to have this written down somewhere! Wikimedia had https://www.mediawiki.org/wiki/Gerrit/%2B2 and https://www.mediawiki.org/wiki/Gerrit/Code_review (and other documents), and it was very easy to see when you were transgressing social norms (or were about to).

[choldgraf]

Those sounds great to me as well - agree with what yuvi said as well...as somebody that has only recently started intersecting with the jupyter dev world, I'm still getting a feel for how the group operates

[fperez]

Strong +1 to @willingc's short but to the point list. I think it's an excellent outline of practical things to follow for good team dynamics.

And I agree we should elevate them to project-wide status!

[ellisonbg]

+1

The other aspect that is implicit in the above points is this: Who is the
team?

There is benefit in talking and establishing a clear answer to that
question - even if it is relatively obvious. We have found it was very
helpful in JupyterLab to make it explicit, but also informal and open. To
help with that we have this "Team" section in our README:

https://github.com/jupyterlab/jupyterlab#team

That lists the folks who self-elect to be listed as maintainers and
self-describe what they are working on. It has helped us in talking to
others and helped the team better understand who the "we" is at any given
time for communication purposes.

On Mon, Oct 9, 2017 at 11:38 AM, Fernando Perez [e-mail]
wrote:

Strong +1 to @willingc https://github.com/willingc's short but to the
point list. I think it's an excellent outline of practical things to follow
for good team dynamics.

And I agree we should elevate them to project-wide status!

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/jupyterhub/binderhub/issues/178#issuecomment-335250525,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABr0CMCEBeVF_1h42PZ-gHT-pMG-Zs0ks5sqmg0gaJpZM4PypWs
.

--
Brian E. Granger
Associate Professor of Physics and Data Science
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
[email protected] and [email protected]

[minrk]

Excellent points @willingc! I agree that the looser guidelines are a good fit, especially for a team as small as ours. It is a good idea to add that to the guides around here.

[yuvipanda]

@minrk @willingc @fperez @ellisonbg do you think we should just make these guidelines 'official' for JupyterHub + BinderHub related repos first, and then bring it up for community discussion? Or since there's enough BDFL support JDI for the whole project?

[ellisonbg]

I have mixed feelings about applying specific mechanics to the whole
Jupyter org - even though the principles are general. We have enough
semi-independent teams working, and I think it is healthy for the given
teams to "own" the mechanics/details of their development process, while
following the overall spirit. At the same time, our teams/repos are very
dynamic and it is helpful to have org-wide standards that also help new
contributors. I found that same tension when I submitted this PR about
label/milestone to the governance repo:

https://github.com/jupyter/governance/pull/29

I still need to return to that and see if we can extract principle and
ideas that apply to the entire org, while still allowing different teams
flexibility on the details.

On Wed, Oct 11, 2017 at 8:50 AM, Yuvi Panda [e-mail]
wrote:

@minrk https://github.com/minrk @willingc https://github.com/willingc
@fperez https://github.com/fperez @ellisonbg
https://github.com/ellisonbg do you think we should just make these
guidelines 'official' for JupyterHub + BinderHub related repos first, and
then bring it up for community discussion? Or since there's enough BDFL
support JDI for the whole project?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/jupyterhub/binderhub/issues/178#issuecomment-335856797,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABr0HFG458NkHVL4bf3a8Z21eCQSCNmks5srOPKgaJpZM4PypWs
.

--
Brian E. Granger
Associate Professor of Physics and Data Science
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
[email protected] and [email protected]

[betatim]

My two cents: made good experiences for small-ish projects by requiring that someone else merge your PR, never be in a hurry with a decision (no automatic 48h rule or such), and discuss everything in issues (in person doesn't count until someone writes it up in an issue). It does sometimes feel like this slows you down, but if you know about that constraint it makes you plan accordingly which has the nice benefit of making things less stressful. Empirically the case of "in a hurry + didn't regret rushed decision" seems to be incredibly rare.