IMO, code review is a cornerstone of code quality and production stability - the number of dumb (and smart!) mistakes in my code that have been caught in CR are numerous, and it's a big portion of my workflow. There are times when I feel it's redundant (one-line changes, spelling mistakes, etc), but I wouldn't trade those slowdowns for a system where I only got review when I explicitly wanted it.
Of course, for pre-production project and/or times when speed is of the utmost concern, dropping back to committing to master might make sense, but for an established and (I'm assuming) fairly large/complex codebase, I would think that it would be best for maintainability and stability to review code before it's deployed.
We do.
Our tooling will tell us what cases have introduced between our current deployment version and the to be released version. It will tell us that all cases have been reviewed and tested and ready to go between those two versions. We usually constantly deploy, so all cases ready are deployed asap.
Only issue is that you can get 'blocking' cases but that's fairly rare. Big cases get a feature switch
We ask for code reviews all the time, we simply don't mandate them - I think that's the main difference.
Isn't that 'after the fact', considering your teamcity polls the gitlab repo a lot, so a commit will trigger a build right after it, and if everything goes well, deploy it too?
So you have to know up front whether a thing is 'risky', but that's a subjective term.
So yes, it will build to dev, but we're using this in situations where we're very confident the changes are correct already. I'd argue blind pushes are the problem otherwise. If the developer is not very certain: they can open a merge/pull request or just hop on a hangout to do a review.
Ah missed / overlooked that!
(I find this is the most valuable way of doing code reviews vs pull requests/sending comments back and forth. In-person conversation about the code is so much higher bandwidth.)
Obviously, it depends widely on the codebase and the number/quality of engineers working on code, but it's been my experience that a team reviewing each other's code is still something that can't be 100% replaced with automated tests.
Every time such process was put into place, it quickly faded out after a few months.
Yes, Yes, and No.
#1 kind of happens naturally as we're all working on different things. There are "a lot" (< 100) of people slinging code/design/SRE/IT, but only a few work in the same areas at the same time and rapid good communication generally happens among those subsets of the overall team. We also seem to have perpetuated a tendency toward good citizenship, so we generally talk to people before traipsing through areas of the code we aren't familiar with.
#2 Absolutely. This is some basic principle stuff. We don't hire people to not trust them.
#3 Related to #2: Automated acceptance testing is done as is deemed appropriate by the person developing the system. I've been on teams that valued and developed automated testing more, and less. My personal experience at the company has been it is neither necessary nor sufficient for success.
Much more important than any pre-deploy automated testing is our effort to monitor what's deployed (both in terms of software/hardware metrics and business goals). Bosun (http://bosun.org), developed by our SRE team, gives us some pretty great introspection/alerting abilities. I'd be incredibly sad to not have it. Bosun monitoring combined with the ability to have a build out in <5 minutes keeps me pretty happy.
Currently most teams don't subscribe to a particular methodology, though there is a Scrum effort happening on one team.
From what I heard so far, Stack Overflow and the Stack Exchange sites don't have a significant amount of automated tests.
If your code is well organized into modules broken down by functional area, it should reduce the number of potential conflicts.
Also, fear of merge conflicts is somewhat unjustified; most conflicts can be resolved and rebased against using git rebase without that much work; the git rerere option [1] and git imerge [2] can also help with this.
If developers would actually learn how to resolve merge conflicts, and not be afraid of the occasional conflict resolution which requires understanding the other change and how to write new code that incorporates both changes, it's less overhead than communicating about pending changes.
[1] https://git-scm.com/docs/git-rerere [2] https://github.com/mhagger/git-imerge
- Committing to master directly is the simplest thing to do. That is why most people chose to do that. It works well if everyone is working on their own pieces not touched by others. Out of all the teams I have worked with in past almost 4 in 5 did this.
> They have a comprehensive test stack, which helps verify the above point and keep it sane
This may or may not be true.
To us, the speed of deployment and overhead savings we get 24/7 is also absolutely worth those very rare issues.
Good modularization of code I think makes this more possible - the point of a big test suite is to catch unintended consequences of a change, and the less coupling the less likely this is to happen. Stuff like routers, session object and other stateful/lower level logic I'd imagine is more tricky to change without a test suite
Thanks again for the post and the candid answers!
- Good rapid communication about who is working where. People are generally not touching the same code or else you'd run into frequent collisions (solvable via rebasing of course but they would be doing more branching if it were a thing to happen very frequently I'd suspect)
- The developers are given autonomy and have assumed some level of mastery over whatever their domain is. Trust in each developer's ability to commit well-formed and considered code.
- They have a comprehensive test stack, which helps verify the above point and keep it sane