4 ms·

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/maste

by plasma 5y ago

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 5y ago
> 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.
- couchand 5y ago
  CI should be sufficient review for most code changes. Manually review architecture and design, but automatically verify implementation.
  - GauntletWizard 5y ago
    The review should be perfunctory, but a second set of eyes is always needed
    - maximilianroos 5y ago
      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 5y ago
        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"
  - nyanpasu64 5y ago
    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 5y ago
      Just thinking out loud: why in that case we couldn't have automatic revert of the push?
- maximilianroos 5y ago
  One advantage of writing tests is that you can immortalize your review as "do the tests pass". It's liberating.
  - seattle_spring 5y ago
    "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 5y ago
    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 5y ago
  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 5y ago
    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.
- lkschubert8 5y ago
  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 5y ago
    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 5y ago
      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).
- astonex 5y ago
  Of course there are. For example a one line change to make the text-size slightly bigger.
  - jameshart 5y ago
    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.
  - FalconSensei 5y ago
    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
  - seattle_spring 5y ago
    * 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.

by plasma 5y ago

seattle_spring 5y ago

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

couchand 5y ago
CI should be sufficient review for most code changes. Manually review architecture and design, but automatically verify implementation.
- GauntletWizard 5y ago
  The review should be perfunctory, but a second set of eyes is always needed
  - maximilianroos 5y ago
    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 5y ago
      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"
- nyanpasu64 5y ago
  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 5y ago
    Just thinking out loud: why in that case we couldn't have automatic revert of the push?
maximilianroos 5y ago
One advantage of writing tests is that you can immortalize your review as "do the tests pass". It's liberating.
- seattle_spring 5y ago
  "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 5y ago
  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 5y ago
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 5y ago
  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.
lkschubert8 5y ago
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 5y ago
  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 5y ago
    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).
astonex 5y ago
Of course there are. For example a one line change to make the text-size slightly bigger.
- jameshart 5y ago
  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.
- FalconSensei 5y ago
  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
- seattle_spring 5y ago
  * 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.