Granular PR requirements for different branches

[w-k-jones]

We currently have a requirement for a minimum of two reviewers on all pull requests. This works well for fully developed features that are ready to be added to a release candidate. However, it also makes working on new features collaboratively difficult.

In the developer meeting today, we discussed whether our PR policy should have different requirements for different branches in order to promote working on new features collaboratively. This could involve something like the following:

• Release candidate branch: 2 reviewers, as currently
• Development branch: Only 1 review required, when complete merge into RC with 2 reviews
• (?!) Experimental branch: No reviews, core contributors can push directly, allows rapid, collaborative work on specific problems

[JuliaKukulies]

I think we did not talk about main but I think it makes sense to keep the rule of 2 reviewers?

As of now, we do not have any automatic protection rule for the RC branch or do I misunderstand this setting? At least it is not listed under protected branches. Should I go ahead and change this using the branch name pattern RC_v* ?

[w-k-jones]

Good point, we should definitely keep the current rules on main

And yes, it would be good to add RC_ and hotfix to the protected branches list

[JuliaKukulies]

Done.

[w-k-jones]

We could also have a rule that a dev branch must be created from an RC_ branch, and an exp branch from a dev branch so that we don't just have branches with reduced protection floating around

[JuliaKukulies]

Agreed, that sounds good.

[JuliaKukulies]

I had a quick look into that now and it is not clear to me how to adapt such a rule. Do you have an idea @w-k-jones ?

[w-k-jones]

I don't think this a rule we can create on github, but we could perhaps add it the developer guide

[JuliaKukulies]

Ah OK, right. That makes sense. I will keep it in mind when we will start working on the developer guide!

[w-k-jones]

Looking at how we've started implementing so far, I'm wondering if we actually have a use for the intermediate tier (i.e, 1 approving code review). It feels like it doesn't add much value, given that 2 reviews are still required to merge into the release candidate and it merely slows down collaborative development. Maybe we should do away with the exp_* branch idea, and make dev_* require no reviews?

[JuliaKukulies]

I actually thought about the same when I created the exp_* and dev_* branches for the developer guide. Feels a bit redundant and extra complicated. On the other hand, when we work on larger developments such as the migration to xarray/dask, it could be good to have some mechanism to ensure that things are smartly implemented from the beginning. But since we would collaboratively work on this and discuss in the development, this should be fine? What do you think @freemansw1, @kelcyno, @fsenf ? Another question is whether you think it is fine to directly push commits on the branch where we require no reviews or whether you would still go via PRs from fork?

[fsenf]

I agree with @w-k-jones & @JuliaKukulies to skip review requirements until the collaborative stuff is merged via PR into a release candidate. So, we actually do not need a dev_* branch and can just stay with the exp_* branch ....

I also thought to push directly against exp_* branch because otherwise this would require someone else to press the merge button which also mean an implicit approval the suggested updates.

[JuliaKukulies]

OK, I changed the branch protection rules again and deleted the exp_* branch for the developer guide. dev_developer_guide is thus the branch we are working on with no reviews required. So, it is the same as you said @fsenf, sticking with the branch with no protection rules but calling it dev_* instead of exp_*. The name can also be changed if you prefer exp_*.