Define code review workflow

pablodip commented 6 years ago

Description

I think it would be nice to make explicit a code review workflow, since now there are more people working on the SOB, and we also probably aim to set an example to work better with bounties.

User Stories

As a developer I want to have other devs review my code so that the code that goes to production is checked by other people and they can spot improvements or mistakes I didn't.

As a developer I want to be able to review commits before they go to production, so that I want all changes be made through pull requests.

As a developer I want that an automatic tool helps me follow the code review workflow so that I can get used to them and follow them better.

As a new developer/external contributor I want to read about the code review workflow in the docs so that I know how things work here and start contributing better.

As a product manager I want devs to review their code to each other so that better code is done.

Expected Behaviour

Have a defined code review workflow and be able to read it somewhere, could be in the README.

Actual Behaviour

No defined workflow.

Proposed workflow

Make all changes through pull requests against QA (develop).
If there is some urgent bug that needs to be fixed in production (master), make a pull request against develop if it is not polluted with other changes, or against master directly otherwise. If so, make a pull request to merge master with develop so that develop is keep up to date when it is bypassed.
Have at least two request approvals in pull requests before merging them. There are going to be four devs now (including @dmitryn that currently is more with status-react), so I think it is doable.
If there is a pull request to fix something very urgent in production and there aren't other devs online to review it, merge it directly.
Enable "Require pull request reviews before merging" and "Dismiss stale pull request approvals when new commits are pushed" on Github: https://help.github.com/articles/about-required-reviews-for-pull-requests/

What do you think @arash009 @dmitryn @tpatja @siphiuel ? :)

arash009 commented 6 years ago

I'm happy with the defined approach. My only comment would be on point 3 to have two approvals before a merge. This might slow the pace of work. Maybe one approval is enough for simple changes, and two approvals for large complex implementations.

@oskarth if you have any learnings/lessons learnt from processes by other swarms.

pablodip commented 6 years ago

@arash009 On one hand I agree, since that could move pull requests faster into production. On the other I don't agree, since by requiring two request approvals we ensure that the knowledge and changes are better distributed in the team. Let's see anyway if @oskarth says something about this.

One more thing I just though of that could be defined in the workflow.

Who merges the PRs, the creator or the reviewers? I think that if the creator is internal, then the creator. Otherwise the reviewer, since the creator won't have permissions. This is because the creator is the one responsible to make it arrive to production.

oskarth commented 6 years ago

Sounds good!

Maybe one approval is enough for simple changes, and two approvals for large complex implementations.

This is what we have in practice at status-react as well. Usually it is 2 reviewers, but for very quick fixes 1 works out OK in practice too (build on fire kind of stuff).

On the other I don't agree, since by requiring two request approvals we ensure that the knowledge and changes are better distributed in the team. Let's see anyway if @oskarth says something about this.

This is a good point too. So at least as a soft requirement. We try to request reviews from 3 people so at least 2 can review it in normal case.

You can see how we do it at https://wiki.status.im/Developer_Documentation#Contributing (PR/CR) and https://github.com/status-im/status-react/projects/7

Make all changes through pull requests against QA (develop). If there is some urgent bug that needs to be fixed in production (master), make a pull request against develop if it is not polluted with other changes, or against master directly otherwise. If so, make a pull request to merge master with develop so that develop is keep up to date when it is bypassed.

We practice trunk based in status-react (https://trunkbaseddevelopment.com), might be interesting for SOB to consider too. I know develop and master often gets out of date. The only reason status-react needs releases and can't do continuous delivery 100% yet is because our deployment target is App Store, which has a long review process and we can't force clients to update. In SOB case it is a web service + site, so it makes sense to move to continuous deployment as soon as possible IMO.

Who merges the PRs, the creator or the reviewers? I think that if the creator is internal, then the creator. Otherwise the reviewer, since the creator won't have permissions. This is because the creator is the one responsible to make it arrive to production.

As a rule of thumb this seems reasonable, but I wouldn't disallow reviewer from merging (say, maybe creator is asleep). Encourage permission-less participation etc.

pablodip commented 6 years ago

Agree with some requirements being soft and permission-less participation.

Thanks for sharing your view, @oskarth, really appreciated. :)

churik commented 6 years ago

For end-to-end (automated tests): I believe one approval from any QA will be enough for merging to master.

pablodip commented 6 years ago

@churik Agree with that! :)

status-im / open-bounty