[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: PNG File Screen Shot 2017-09-21 at 2.29.25 PM.png    
Issue links:
Relates
relates to FOLIO-843 require reviewers for PRs Closed
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:

  • okapi
  • stripes-core

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:

  • stripes-components
  • stripes-redux
  • stripes-form
  • stripes-logger
  • stripes-react-hotkeys
  • stripes-util-notes
  • eslint-config-stripes
  • ui-requests
  • ui-users
  • ui-circulation
  • ui-checkout
  • ui-items
  • ui-instances
  • ui-organization
  • ui-scan
  • ui-plugin-find-user
  • ui-developer
  • ui-checkin
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:

@Jason - today FOLIO testing is up and running; therefor it would be awesome if you could find time to review Mike Taylor's change, so I can follow up, do test and we can close this jira. Thanks

Generated at Thu Feb 08 23:08:43 UTC 2024 using Jira 1001.0.0-SNAPSHOT#100246-sha1:7a5c50119eb0633d306e14180817ddef5e80c75d.