Hi again everyone,
I’m glad we are talking about this important stuff. My past experience with “pre-push reviews” is to do a pull request workflow. Any contributors would code on a branch and then make a pull request when they want to submit that to the community. The review process can happen in GitHub rather than Fisheye.
My experience with this is that the process can go very slowly, and it can take days or weeks for other people to really use your code. It also causes lots of re-base or re-write work if it takes a week for the review to finish and then you find out that the master branch has changed since then, so you need to update your work (and then request a re-review!).
So my feeling is that the down-sides of this might out-weigh the advantages for now—at least for core team members like the SolDevelo team members who are all working together on the code base daily. I feel like for that kind of contributor, having direct repository access and making commits to master is allowing everyone to work more quickly, and yes that means things might break, but we notice it quickly and get it fixed quickly.
After the new version of the software has a public, stable release (like 3.0), then changing workflows to a pull request workflow might be worthwhile. Certainly for other contributors around the community (not core team members) this kind of pull request workflow will be much better.
You mentioned about “bad code” going into the product and spreading. I think the best way to deal with that is to give feedback during reviews and have developers go back and improve their code (before their ticket can be marked Done and before we claim those story points as done).
That is my 2 cents on this issue. I look forward to a healthy conversation. I definitely want us to have a process and tools that produce quality code and that helps developers get their jobs done.
On 11/16/16, 3:08 AM, "firstname.lastname@example.org on behalf of Sebastian Brudziński" <email@example.com on behalf of firstname.lastname@example.org> wrote:
as an outcome of our sprint retrospective meeting, I'd like to start the
discussion about current code-review workflow (with a few exceptions,
the general rule is to do reviews after the changes are pushed to the
master branch). It has been proposed to move the review process to
happen before the changes appear on master and only push them after they
have been approved during review. The main arguments for this were:
- more stable builds (we can enforce that the certain builds pass,
before the merge takes place)
- better quality of the codebase (since every change needs to go
through review first, whatever appears on master should already be of
high quality - this may help to prevent the "bad code" from spreading)
- better control over codebase (eg. code freeze can be easily achieved
without interfering with other people work)
- same code review process for everyone (SD & VR & community - direct
write access to the repositories is currently granted to the certain
people, which means community cannot jump right in and push their
changes - they would probably need to go through pull request and have
their changes reviewed first before the merge)
There are obvious drawbacks as well, the main one being having to wait
longer for the changes to appear on master (and consequently on the demo
server). Also if the ticket is blocked by another ticket, you either
have to wait for the one's ticket code to go through whole cycle of code
review before starting the work on the new one or take
incomplete/unreviewed changes and work on them.
Please feel free to share your thoughts about current process and your
experiences with working on pre-push reviews, if you have any. Do you
see any issues with the current process?
Please note that this is not a request to change the process. This is an
opportunity to share the feedback and experiences on code review process