Recommend (or require?) PR workflow for contributing to repositories

[manics]

Proposed change

Recommend (or require if there's support) that code changes to repos in the JupyterHub org should almost always be made using a pull request.

I just saw a conversation pass by on jupyterhub/nativeauthenticator#164
Now that we've started adding code guidelines such as linting/auto-formatting I think we should consider guidelines on how to contribute code.

If we wish we could extend this to "requirements for accepting a repo into the JupyterHub org" but that's probably best discussed on a separate issue.

Alternative options

• Do nothing 🙂.
• Only enforce guidelines for new repos.

Who would use this feature?

Provides a standard workflow across all JupyterHub repos, which makes it easier for people to contribute code, and to understand how their PRs will be handled.

This could be extended to whether or not an issue should be open alongside a PR. For example many other orgs require this which is why we sometimes see an issue and PR opened together by new contributors, but we could optionally say in our guide we prefer to avoid this.

(Optional): Suggest a solution

Add some guidelines to our team-compass docs

[betatim]

I support this. If only because I thought that is how we already were working.

Related bit of info that might be interesting https://repo2docker.readthedocs.io/en/latest/contributing/contributing.html#process-for-making-a-contribution has some info/guidelines/hints (what ever you want to call it) that grew out of https://github.com/scikit-optimize/scikit-optimize/blob/master/CONTRIBUTING.md which in turn is based on ideas from the Mozilla Open Leaders training and mentoring I did (way back when).

[manics]

I'm aware of the repo2docker guidelines, but it's never been clear to me whether that applies across the org or just for r2d .

[lambdaTotoro]

As the two sides to the conversation in question, both @consideRatio and I agree that this is the way to go for non-trivial changes like documentation restructuring or feature additions or bugfixes.
The point of "contention" was only if the whole rigamarole (open issue, fork, pull request, request code review, wait for code review, rocket-react to code review, merge pull request, close issue) also needs to be followed for trivial changes like correcting typos or adding slight improvements to the documentation.

I think it shouldn't. It makes the whole process taste less like dopamine and more like "soulless corporate customer support hotline". But Erik said not doing it this way makes him uncomfortable and he's been a huge help with NativeAuthenticator, so I'm willing to concede the point.

I accept that this is only my personal opinion, but I thought that might also be of relevance here.

[consideRatio]

Thanks for starting a process to formalize our workflow @manics and @betatim! I'm +1 to having a clear policy to say we should always go for a PR with the exception for commits made when following a RELEASE.md outlined process.

Regarding enforcing this with a GitHub rule, I think it would be fine for repo's where the RELEASE.md outlined process allows for it. I would then consider the key benefit of enforcing it to protect against mistakes and make it harder for anyone with a compromised account to sneak in changes unnoticed.

I opened #466 regarding the related but separate question of self-merging PRs.

---

My position is that always opening PRs is quite an easy thing to adjust to, but to always wait for another person to merge is a more problematic thing. I think self-merging PRs can often be the right call for some PRs in our organization where we suffer a lack of maintenance capacity overall. Examples of where I think a self-merge is perfectly fine would be a typo in documentation, fixing a broken link, a small bugfix where one is very confident about it and perhaps has let some time pass - nobody else has reviewed - and one still remain confident about it, a CI system bugfix or tweak to make it reliable, etc.

[manics]

@lambdaTotoro I think you've basically highlighted what it means for a repo to be part of a GitHub organisation. By being part of a GitHub org a repository gets the credibility and approval of the owner organisation, and the organisation takes on responsibility for maintaining the repo even if the original maintainers move on, or for things like ensuring code remains secure and that vulnerabilities are handled responsibly.

The downside is that members of the org need to understand what's in the repos and follow shared guildelines. See for example a previous discussion on linting/formatting where there were dissenting views #454

Edit: This is also a sign of growing pains (which is good!) 😄 . With a small team it's easy to resolve things informally, but as a project grows it's more important to have standards and guidelines.

[consideRatio]

Thanks for deliberating about this @lambdaTotoro!

The point of "contention" was only if the whole rigamarole (open issue, fork, pull request, request code review, wait for code review, rocket-react to code review, merge pull request, close issue) also needs to be followed for trivial changes like correcting typos or adding slight improvements to the documentation.

I think it shouldn't. It makes the whole process taste less like dopamine and more like "soulless corporate customer support hotline".

To me, being super strict about self-merge is the biggest concern here. I think it is a hefty price on the momentum and maintenance joy in my mind to never self-merge. But, I opened #466 to focus on that question so I'll not deliberate further here.

[consideRatio]

Question

Do you support defining a more explicit policy where we always opt for creating PRs, with the sole exception of following a procedure in a RELEASE.md or equivalent document that includes pushing a commit or two?

👍 yes
👀 not sure
👎 no

[minrk]

👍 to doing it as a practice (since we already do) and documenting it to help with clarity and communiation, 👎 to enforcing it technically (now that GitHub can do that), because it would prohibit publishing releases from something like tbump/bump2version, which require making and publishing the release commit. Repos that adopt versioneer/setuptools_scm/other versions-only-in-git tools can actually use the new branch protection rule.