[FOLIO-842] Block direct commits to master branch/Require Reviewers for PRs Created: 19/Sep/17 Updated: 12/Nov/18 Resolved: 06/Oct/17 |
|
| Status: | Closed |
| Project: | FOLIO |
| Components: | None |
| Affects versions: | None |
| Fix versions: | None |
| Type: | Task | Priority: | P2 |
| Reporter: | Jakub Skoczen | Assignee: | John Malconian |
| Resolution: | Done | Votes: | 0 |
| Labels: | ci, montreal, sprint23 | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | 2 hours, 30 minutes | ||
| Original estimate: | Not Specified | ||
| Attachments: |
|
||||||||
| Issue links: |
|
||||||||
| Sprint: | |||||||||
| Description |
|
As decided during the plenary session we will want to use PRs for all activity on the master branches. This will allow us to run the build, tests and perform code review (where applicable) before the code ends up on the master branch and breaks dependent projects, CI jobs or deployments (e.g demo). |
| Comments |
| Comment by John Malconian [ 21/Sep/17 ] |
|
I've enabled this on folio-org/okapi. See attached screenshot for configuration. I've also enabled required PR reviews. Jenkins tests must also pass in PR before merge. Let's see how this works out. Adam Dickmeiss You have Github admin privs in case you need to revert changes. |
| Comment by Adam Dickmeiss [ 21/Sep/17 ] |
|
Great. I checked if I could push straight to master and that failed as it should. adam@fry:~/folio/okapi$ git push X11 forwarding request failed on channel 0 Counting objects: 3, done. Delta compression using up to 4 threads. Compressing objects: 100% (3/3), done. Writing objects: 100% (3/3), 303 bytes | 0 bytes/s, done. Total 3 (delta 2), reused 0 (delta 0) remote: Resolving deltas: 100% (2/2), completed with 2 local objects. remote: error: GH006: Protected branch update failed for refs/heads/master. remote: error: At least one approved review is required by reviewers with write access. To git@github.com:folio-org/okapi.git ! [remote rejected] master -> master (protected branch hook declined) |
| Comment by John Malconian [ 21/Sep/17 ] |
|
I've also enabled this and required PR reviews and status checks on folio-org/stripes-core. Mike and Jason are granted the ability to dismiss PR reviews as necessary. Not to be abused |
| Comment by John Malconian [ 26/Sep/17 ] |
|
enabled for the following projects:
Let's see how this goes for a few days before enabling across other projects. |
| Comment by John Malconian [ 27/Sep/17 ] |
|
Enabled protected branches and required PR reviews for the following additional projects:
|
| Comment by John Malconian [ 28/Sep/17 ] |
|
I've enabled protected 'master' branch across all FOLIO frontend and backend projects. I've rolled back enforcement of 'passing status checks option before merge'. Too risky right now with the implementation of Sonarqube in the CI process and other changes taking place. We can re-enable that when that piece becomes more stable. Only frontend projects, okapi, and cql2pgjson require PR reviews currently. We should probably think harder about enabling this on backend modules since most backend modules currently have only one or two maintainers or contributors. This could cause bottlenecks in workflow. |
| Comment by Matt Connolly [ 02/Oct/17 ] |
|
I thought we had decided that code reviews would be optional, at least for the core team. Am I wrong about that? |
| Comment by Mike Taylor [ 02/Oct/17 ] |
|
Apparently. I know, at least, that I can't merge any of my own PRs. |
| Comment by Matt Connolly [ 02/Oct/17 ] |
|
That's what I was anticipating being able to do. It's thrown my workflow a bit out of kilter. |
| Comment by Mike Taylor [ 02/Oct/17 ] |
|
Yeah. That's why we now have comments like Cate's most recent on UIU-130:
|