Hacker News Reader

Comment by taurath | Hacker News Reader

How people manage git and source control tells you a lot of things about a company's culture. They said that most commits go directly onto master and this works for them, which indicates:

- 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

View on HN

I found this very curious - by their own admission, this also means that most code _does not get reviewed_ before it lands in production. To me, this is quite scary, and I would be very hesitant to adopt this for any large-scale project or company.

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.

Shouldn't the types of mistakes you're worried about be caught by the automated tests? IMO code review is more about bigger picture design, e.g. if a piece of code would yield the correct result but with sub-optimal performance (which can also be caught by automated tests but not always).

There's a lot that fits between automated tests and code review. We have an extensive system of static analysis and code quality bots that run, but there's still a lot of design patterning and higher level functionality that machines (or, at least, our machines) don't always catch.

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.

Prior work in CompSci on various engineering techniques showed code review to be among most effective at any part of lifecycle. Testing can miss lots of things. So, best to do both.

You can commit code code directly to master and still code review all code before production if you want to.

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

What are cases? Is it short for use case?

Pretty sure he means like an issue. A github issue, jira ticket, etc.

i'm guessing a scrum use case.

Yes, we use the word case for issue here

If we want a code review on anything risky, we may push a branch or we may just post the commit in chat for review before we build out. Which is chosen depends on how big or blocking the change may be.

We ask for code reviews all the time, we simply don't mandate them - I think that's the main difference.

Cool! That clarifies things a lot - the way I read the article sounded like you rarely asked for reviews.

> or we may just post the commit in chat for review before we build out.

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.

It only deploys to our development/CI environment automatically. Deploying out to the production tier is a button press still.

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.

> It only deploys to our development/CI environment automatically. Deploying out to the production tier is a button press still.

Ah missed / overlooked that!

Code reviews aren't just about catching problems either: the reviewer can learn new coding techniques from this, the reviewer will become more familiar with following the team guidelines by applying them this way and the reviewer is also made more familiar about parts of the codebase they might have not known about.

It's a false dichotomy to think you can either move fast or have good process. Etsy commits directly to master on their large, monolithic php webapp and they have a pretty strong code review process where code isn't deployed until it's reviewed. They still manage to move fast with autonomy and trust, at least they did when I worked there for the past couple of years.

I just happened to listen to an old Stack Overflow podcast where they talked about code reviews. They said they do in-person code reviews before committing code.

(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.)

We did in person code reviews at my company, and for me they are mostly useless. I like to spend some time to look at the code, and doing so on someone's screen or when the are looking at yours makes me (and fellow devs) want to finish it quickly, so we frequently missed design problems.

Most companies I work for, many in Fortune 500 roster, don't ever do code reviews.

Every time such process was put into place, it quickly faded out after a few months.

Long time dev at the company here.

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.

Just out of curiosity... you guys seems very pragmatical in most of things. Are you using any development methodology/principles (Agile, waterfall, etc.)?

Again, up to the individual teams to decide what's right for them. The most globally applicable principle we have is "Make sure to use your brain."

Currently most teams don't subscribe to a particular methodology, though there is a Scrum effort happening on one team.

The comprehensive test stack are mostly the users on meta, which gets the deployed code before the other sites.

From what I heard so far, Stack Overflow and the Stack Exchange sites don't have a significant amount of automated tests.

A team of developers all working off of master doesn't necessarily require much communication about who's working where.

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

The number of devs who screws up rebase, or worse merge, is astonishing. "I just used theirs" :|

You're right, there are a lot of devs like that. In this day and age of DVCS any developer worth their salt should be able to manage merges properly. It sounds like SO might not hire people who are incapable of understanding merging.

I agree with 2 but not 1 & 3. In my experience

- 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.

If you are frequently pushing to master which is deploying to prod, not having a good test stack is asking for trouble - their uptime indicates this isn't happening.

Not that I think it's an invalid argument, but almost all of our outages have been either a database issue or (far more often) CloudFlare's inability to reach us (the origin) across the internet. Deploying code rapidly very, very rarely causes an issue.

To us, the speed of deployment and overhead savings we get 24/7 is also absolutely worth those very rare issues.

Thats pretty amazing actually! While I certainly have seen a fair amount of code that passes (even good) automated testing but fails on validation, it feels somewhat strange but starts to make sense after a bit to value heavy monitoring more.

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!

In my experience, committing directly to master is a helpful way to decrease merge conflicts. They may happen more frequently, but you get little conflicts more often, rather than large conflicts or conflicts that need to be resolved multiple times.

Reply on news.ycombinator.com