4 ms·

I've rarely seen the first description of it where people actually commit directly to main. Except in very early stage projects. But it does always feel the fas

I've rarely seen the first description of it where people actually commit directly to main. Except in very early stage projects. But it does always feel the fastest if you only review code "on-demand" in PRs/MRs instead of enforcing it for every change. I think in a team with good ownership, enforcing formal reviews slows down a lot. But of course in a larger code base where no single engineer can understand all the effects a change might have, having a bit of knowledge sharing and 4 eyes enforced is often the better approach than yolo-ing it. Then again I did build an SQL review tool for database access because I felt like "yolo-ing" production access was also not the way it should be. It's an interesting slider between full autonomy and strict review processes where each team needs to find their sweet spot: https://github.com/kviklet/kviklet/ https://github.com/kviklet/kviklet/

div3rs3 8mo ago
We're organized in small teams around specific products. That leads to mob programming being a good fit, so code reviews and knowledge sharing is organic. This gives us an opportunity to commit to main and do direct deploys to production. Treating every commit as deployable is key, but it drives good practices.
dxdm 8mo ago
As I understand it, trunk based development does not call for committing directly to main. It says to avoid long-lived branches for releases, whole features, etc. There's nothing wrong with small, short-lived branches that can be quickly reviewed and merged into main. That being said, I've been in a small team where the blessed style was to commit directly to main and do reviews "on demand". It quickly gets features deployed in a way that builds a lot of rot and debt into your project that people quickly lose a good understanding of. Then you just keep piling. There's probably a way to get this done properly with a tiny team of very experienced and well-aligned developers; but that's usually not what you have in an environment that pushes this kind of extreme no-review-interpretation of trunk-based development. Slow down, do reviews, avoid keeping branches open for more than a day.
- jascha_eng 8mo ago
  > well-aligned developers I think this is very key, if the development style and the direction of the project is clear, much less review and alignment is necessary. And also > avoid keeping branches open for more than a day Big +1 on that, fast reviews are extremely key. Most teams I have seen often took days or even weeks to merge branches though, often because you end up waiting too long for reviews in the first place. Or because of good old bike-shedding. But also because these code reviews often uncovered uncertainties that needed longer discussions. However usually code is easy to change, so defaulting to "just merge it" and creating followup tasks is often the cheaper approach than infinite review cycles.
  - dxdm 8mo ago
    I think it's still worth-while to do reviews. A second pair of eyes does wonders, and it spreads knowledge of what things exist and how they work. If changes are small, reviews can be quick. It's possible to keep building on top of code being reviewed, and even easy when using modern VCS tooling like jujutsu. Once the code is merged, chances are it will not get changed Those follow-up tasks will be displaced by more pressing work that will keep piling onto a slightly unstable foundation, increasing the tilt over time. There is an excluded middle between "no reviews" and "infinite review cycles": proper, timely and efficient reviews. They are worth investing the time to get right. They will start paying dividends months down the line, and boy will they keep paying. This is not about trying to get things perfect from the get go, but to get them done right while you're there. "We'll fix it later" is not gonna happen, and is much more expensive than it initially seems.
  - KronisLV 8mo ago
    > However usually code is easy to change, so defaulting to "just merge it" and creating followup tasks is often the cheaper approach than infinite review cycles. I wish this was the "default" mindset everywhere, especially in those cases where you have that one colleague that loves to nitpick everything and doesn't see an issue with holding up both releases and wasting your time over menial pedantic stuff. It would be so much easier to merge working code and let it work, and keep those TODOs in the backlog (e.g. trash). In a sane world, code review would be like: 1. Will this work and not break anything? We checked it, it's okay. There are no apparent critical or serious issues here. 2. Here's a list of stuff that you can change if you wish, here's why it might be an improvement. 3. Okay, we have some left-over nice to haves, let's keep track of those for later (or not) and merge. It gets infinitely worse if you're working on 3 projects in parallel and the person wants long winded calls or starts nitpicking about naming, or wants things done exactly their way as if it's the only way (doubly worse if their way is actually worse by most metrics and tastes).
roggenilsson 8mo ago
In my previous job we worked like this. We had one dev branch that everyone pushed directly to and the dev branch was eventually branched to an RC branch which in turn was merged to master once the release was complete. The team was small, around 6 people, and the codebase was maybe medium sized (~500k LOC). There was no formal review process, instead it was up to each team member to ensure the quality of their own and others code. In practice I would read through all commits that came in the previous day while having my morning coffee. If there was some egregious I would talk to whoever made to commit to make discuss if something should change, but this was fairly rare. Formal PR reviews were only ever really used for new members or for bigger/sketchy changes where someone wanted more eyes on it. Because I ended up reading most commits, I ended up knowing how pretty much the entire codebase worked. It takes a while for this to develop, but the more you do it the better you get at it, especially in the context of a single codebase.
maccard 8mo ago
I work in games, and we do commit directly to main. On a smaller team you can get away with pre submit review, post submit checks. On a bigger team you need pre submit checks but honestly the point where you need this is much much later than you think. One of my previous projects had 100+ people committing directly to main with no pre submit checks and jt broke once or twice a day. The builds took longer than that to go through so you just always sync to “last known good”

div3rs3 8mo ago

We're organized in small teams around specific products. That leads to mob programming being a good fit, so code reviews and knowledge sharing is organic. This gives us an opportunity to commit to main and do direct deploys to production. Treating every commit as deployable is key, but it drives good practices.

dxdm 8mo ago

As I understand it, trunk based development does not call for committing directly to main. It says to avoid long-lived branches for releases, whole features, etc. There's nothing wrong with small, short-lived branches that can be quickly reviewed and merged into main. That being said, I've been in a small team where the blessed style was to commit directly to main and do reviews "on demand". It quickly gets features deployed in a way that builds a lot of rot and debt into your project that people quickly lose a good understanding of. Then you just keep piling. There's probably a way to get this done properly with a tiny team of very experienced and well-aligned developers; but that's usually not what you have in an environment that pushes this kind of extreme no-review-interpretation of trunk-based development. Slow down, do reviews, avoid keeping branches open for more than a day.

jascha_eng 8mo ago
> well-aligned developers I think this is very key, if the development style and the direction of the project is clear, much less review and alignment is necessary. And also > avoid keeping branches open for more than a day Big +1 on that, fast reviews are extremely key. Most teams I have seen often took days or even weeks to merge branches though, often because you end up waiting too long for reviews in the first place. Or because of good old bike-shedding. But also because these code reviews often uncovered uncertainties that needed longer discussions. However usually code is easy to change, so defaulting to "just merge it" and creating followup tasks is often the cheaper approach than infinite review cycles.
- dxdm 8mo ago
  I think it's still worth-while to do reviews. A second pair of eyes does wonders, and it spreads knowledge of what things exist and how they work. If changes are small, reviews can be quick. It's possible to keep building on top of code being reviewed, and even easy when using modern VCS tooling like jujutsu. Once the code is merged, chances are it will not get changed Those follow-up tasks will be displaced by more pressing work that will keep piling onto a slightly unstable foundation, increasing the tilt over time. There is an excluded middle between "no reviews" and "infinite review cycles": proper, timely and efficient reviews. They are worth investing the time to get right. They will start paying dividends months down the line, and boy will they keep paying. This is not about trying to get things perfect from the get go, but to get them done right while you're there. "We'll fix it later" is not gonna happen, and is much more expensive than it initially seems.
- KronisLV 8mo ago
  > However usually code is easy to change, so defaulting to "just merge it" and creating followup tasks is often the cheaper approach than infinite review cycles. I wish this was the "default" mindset everywhere, especially in those cases where you have that one colleague that loves to nitpick everything and doesn't see an issue with holding up both releases and wasting your time over menial pedantic stuff. It would be so much easier to merge working code and let it work, and keep those TODOs in the backlog (e.g. trash). In a sane world, code review would be like: 1. Will this work and not break anything? We checked it, it's okay. There are no apparent critical or serious issues here. 2. Here's a list of stuff that you can change if you wish, here's why it might be an improvement. 3. Okay, we have some left-over nice to haves, let's keep track of those for later (or not) and merge. It gets infinitely worse if you're working on 3 projects in parallel and the person wants long winded calls or starts nitpicking about naming, or wants things done exactly their way as if it's the only way (doubly worse if their way is actually worse by most metrics and tastes).

roggenilsson 8mo ago

In my previous job we worked like this. We had one dev branch that everyone pushed directly to and the dev branch was eventually branched to an RC branch which in turn was merged to master once the release was complete. The team was small, around 6 people, and the codebase was maybe medium sized (~500k LOC). There was no formal review process, instead it was up to each team member to ensure the quality of their own and others code. In practice I would read through all commits that came in the previous day while having my morning coffee. If there was some egregious I would talk to whoever made to commit to make discuss if something should change, but this was fairly rare. Formal PR reviews were only ever really used for new members or for bigger/sketchy changes where someone wanted more eyes on it. Because I ended up reading most commits, I ended up knowing how pretty much the entire codebase worked. It takes a while for this to develop, but the more you do it the better you get at it, especially in the context of a single codebase.

maccard 8mo ago

I work in games, and we do commit directly to main. On a smaller team you can get away with pre submit review, post submit checks. On a bigger team you need pre submit checks but honestly the point where you need this is much much later than you think. One of my previous projects had 100+ people committing directly to main with no pre submit checks and jt broke once or twice a day. The builds took longer than that to go through so you just always sync to “last known good”