search

extendr / rextendr

An R package that helps scaffolding extendr-enabled packages or compiling Rust code dynamically
https://extendr.github.io/rextendr/
Other
184 stars 27 forks source link

Github Automation: Setting up protected branches and PR requirements #196

Closed Ilia-Kosenkov closed 2 years ago

Ilia-Kosenkov commented 2 years ago

In light of recent discussions, let us consider setting up branch protection rules to make PR merging process more... strict. Once a branch is protected, PRs into this branch can be configured. I suggest the following configurations:

  1. ✔️Require PR before merging
  2. ✔️Require 1 approval
  3. ❌Dismiss approvals upon new commit
  4. ❌Require status checks
  5. ✔️Require branches to be up to date
  6. ✔️Require conversation resolutions

Taken into account:

clauswilke commented 2 years ago

I wasn't part of the discussions, so maybe there's something I'm missing, but I'd suggest to reconsider bullet point 3: It can create a lot of friction, where even trivial changes need to constantly be re-reviewed. We're not using it in ggplot, and I think it would be cumbersome if we did.

Ilia-Kosenkov commented 2 years ago

We had no real discussion, so this issues was opened to encourage one.

clauswilke commented 2 years ago

Ok, in this case I'd probably start with fewer constraints and expand if the ones in place turn out to be not sufficient.

Bullet points 1 and 2 are really good ideas, and bullet point 6 seems also like a good idea. I'm not sure exactly what bullet points 4 and 5 imply, so I have no opinion on them.

Not sure if this is covered by any of the bullet points, but you should not prevent merging when automated tests fail. It can make it really cumbersome to fix things when something with the testing infrastructure is off.

Ilia-Kosenkov commented 2 years ago

Not sure if this is covered by any of the bullet points, but you should not prevent merging when automated tests fail. It can make it really cumbersome to fix things when something with the testing infrastructure is off.

I think this falls under category 4, github terminology is sometimes confusing. @yutannihilation also expressed similar concern during a short discussion on the Discord.

yutannihilation commented 2 years ago

I was just a bit hesitant about bothering others to review my trivial tweaks on CI settings :)

I don't have strong concern because it's different from ggplot2's case in that we have the admin permission on this repo so we can revert or modify the repository settings if it doesn't work.

Ilia-Kosenkov commented 2 years ago

Pinging @CGMossa @multimeric

multimeric commented 2 years ago

The settings look fine. I guess I would say yes to #5, and I'm happy to relax #2 to no. Although technically I do prefer multiple rounds of review, it's a bit of a big ask for volunteers.

CGMossa commented 2 years ago

I really don't have strong opinions on this nor experience to pull from. Good discussion though.

Ilia-Kosenkov commented 2 years ago

Also pinging @malcolmbarrett

multimeric commented 2 years ago

Does this specification apply to the Rust repo? Incidentally something that I've just noticed as I'm compiling the changelog is that very few of our PRs are updating it. I would love for this to be a requirement for the PR to be accepted.

Ilia-Kosenkov commented 2 years ago

Good idea. I am not sure if this can be automated, do you have any experience?

multimeric commented 2 years ago

They certainly do it in Python repos, but I'm not sure how we've do it in the Rust/R ecosystem. Actually it seems to be an ugly bash script that I encountered: https://github.com/scikit-learn/scikit-learn/blob/main/.github/workflows/check-changelog.yml. Maybe we can just add "updated the changelog" to the PR template, and make sure reviewers check for it? I'll certainly hound everyone to do so.

Ilia-Kosenkov commented 2 years ago

Yep, the script is ugly. I am certainly in for a PR template. If we want to take automation further, we can perhaps check if Readme file has any changes, and then complain if it is unmodified. A simplified version of what scikit does.

Ilia-Kosenkov commented 2 years ago

So it's been a couple of weeks since I started this discussion. So far it seems that the best set up is the one I summarized in https://github.com/extendr/rextendr/issues/196#issue-1237585099, based on the input of other contributors. I intend to apply it to this repo and then observe how it affects our workflow. We can keep this issue open to collect feed back once these settings are in place.

multimeric commented 2 years ago

Sounds good. I think all of this applies to both repos, doesn't it? Can we apply it to extendr too?

Ilia-Kosenkov commented 2 years ago

@multimeric, I guesss we can, but we need a separate discussion there. Also, it seems 5. does not work without 4., I will probably not use it and stick with the remaining options for now.

Ilia-Kosenkov commented 2 years ago

So, for now the rules are the following (if I did everything correctly):

  1. ✔️Require PR before merging
  2. ✔️Require 1 approval
  3. ✔️Require conversation resolutions

I also enabled code review limit and set it to 'Limit to users explicitly granted read or higher access', which I was not aware of before. This means that

  1. ✔️Only select users can Approve a PR.