[plasma]

My workflow personally and in teams:

1) Consider main branch deployable any time, so don’t push changes that can’t be deployed

2) You can commit to main/master if it’s a reasonably small change not needing review

3) PRs uses for more complex changes, easier to review

4) Deploys off main branch

5) Tests run on all branches

Works well 99% of the time, can fall down when a large chain is queued for deploy (just merged) and someone wants to push a minor change but now it’s got to be everything (unless you revert temporarily).

[seattle_spring]

> 2) You can commit to main/master if it’s a reasonably small change not needing review

I earnestly do not believe there exists any change small enough to not need a review.

[lkschubert8]

In my experience the "this is so small it doesn't need a code review" changes are more often the ones that blow things up.

[m12k]

Yep, if it's a one-liner that's important enough that it is worth shipping immediately on its own, it's probably got enough gunpowder behind it to blow off your foot.

[lamontcg]

I pushed a one-liner with no code review a few minutes ago.

Prior massive change of mine went through pre-merge CI tests just fine, but the post-merge build process blew up because a very large list of possible arches was missing aarch64. For various reasons the pre-merge CI tests (which take minutes) are necessarily more limited than the post-merge (which take hours).

A massive list of labels gets one more label so there's no style cleanup concerns, and our CI is already red so its not getting any redder.

The prior real PR was reviewed and everyone missed the label that wasn't there.

Turns out the fairly massive CI test that I wrote caught the bug that everyone's human eyeballs entirely failed to catch (and I doubt anyone who reviewed it actually went through the code in the CI test in the same level of detail that I went through in order to create it).

[maximilianroos]

One advantage of writing tests is that you can immortalize your review as "do the tests pass". It's liberating.

[seattle_spring]

"does the code work functionally" is a fraction of the overall purpose of a code review. If you rely solely on "does the code work", you end up with the average Rails app after 1 year: 10k line controllers, deeply entangled code, half uncommented code and half deeply over-commented code. Hundreds of comments inevitably end up out of date. Whole blocks of code might be commented out. jQuery is instantiated alongside React and 2 different versions of Angular.

I'll repeat that there's not a single code change I can think of, including comments and documentation, that can safely be merged without a review from a second set of eyes. This is exponentially true the more engineers work on the codebase. I can maybe understand why someone on a team of 3 or less doesn't see the benefit in having the overhead of mandatory code reviews.

[KallDrexx]

If you push into master and forget to run tests (which happens, people are human and make mistakes, especially with small changes), then your broken tests are now breaking everyone's attempts to run the build though. You don't get the feedback that your tests are broken until after it's pushed into master.

[sneak]

You need the two-person rule on changes to master simply to avoid compromised developer credentials being equal to a full compromise of production systems and databases.

A minimum of two sets of eyeballs on every change. CI cannot detect intentional backdoors being introduced.

[josephg]

CI also cannot detect the downstream effects of some small changes.

I've seen plenty of subtle bugs get introduced by someone who has an overly simplistic view of some part of a system. And they expose a simple method to share their simplified view of some part to the world. "I believe you when you say that in all of your tests this array has a length of 1. This is a failure of your test cases. Don't add a getter method which returns arr[0]. Come with me and lets chat in front of a whiteboard."

The log4j bug might have been caught with more eyeballs. "Here's a small patch which adds JNDI support in log messages" -> "Whoa hold on - what are the implications of that? JNDI is complex". But of course, most opensource code can't afford to spend developer time on code review by multiple people.

[couchand]

CI should be sufficient review for most code changes. Manually review architecture and design, but automatically verify implementation.

[nyanpasu64]

But if you push a bad commit to main without making a branch or PR first, then CI tells you about your mistake... after main is already broken and not "deployable any time"!

[methyl]

Just thinking out loud: why in that case we couldn't have automatic revert of the push?

[GauntletWizard]

The review should be perfunctory, but a second set of eyes is always needed

[maximilianroos]

That seems inherently contradictory? Particularly if you put some cost on a) not being able to merge and build on the feature in another branch, b) disturbing someone to do something perfunctory

[GauntletWizard]

It is an inherent contraction. Having good testing and fast flow is a contradiction. The advantage of a second set of eyes is simply a sanity check... from a second pair of eyes. It's all a balancing act, but it does prevent a large set of failure classes that are basically "This dude went crazy"

[astonex]

Of course there are. For example a one line change to make the text-size slightly bigger.

[jameshart]

All you can ever say when you push up a git commit to master is ‘I’m pretty sure the sequence of commands I ran locally will lead to pushing up a single one line commit that has no significant risk’

If you’ve never found yourself in the position where you slipped up and accidentally included one of the following in a commit that you didn’t notice until after you pushed, I envy your attention to detail:

- a line of code commented out to bypass it when running locally

- a non passing test

- a disabled test

- a config change to point to local host instead of a real server

- a developer credential

- a change to a package lock file

- some build output that should have been gitignored

Yes, you should diligently check what you will push before you push; but since git won’t stop you pushing any of these things, and when you are making a ‘quick one line change’ is precisely when your guard is down because you don’t think you could possibly be accidentally about to ship one of these things, these things will get pushed to master if you allow pushes to master.

[seattle_spring]

* There might be a typo

* A bigger text size might break the layout or the styleguide conventions

* Small style changes might break a11y

* Your branch might be messed up, causing you to push a bunch of unrelated changes.

Just a few reasons i can think of, I'm sure there are more.

[FalconSensei]

but thats the commit that accidentally will include something else that blows up everything. It's not only about checking the desired change, but it's also about checking that it was the only thing committed