6 ms·

Seems to assume that people are actively/idly waiting for CI to pass and then start the review? Or they've done the review, wait for CI to pass then hit merge?

Seems to assume that people are actively/idly waiting for CI to pass and then start the review? Or they've done the review, wait for CI to pass then hit merge? Most projects I've worked on with multiple contributors on have had a merge queue where you simply tell it to merge if/when the CI passes, then you can move on with your day.

zimpenfish 3y ago
Anecdata of one, obviously, but I've never worked anywhere using Github that has had auto-merge turned on. You just have to wait for the "CI complete" email (or keep refreshing the page) and hit the merge button (or, in some places, ask the appropriate person to do that.)
- veidr 3y ago
  me either, so now anecdata of 2; but also, sounds insane of all the things a human should be in the loop for, merging to main seems like one of them
  - veidr 3y ago
    (also nuking from orbit)
  - capableweb 3y ago
    > of all the things a human should be in the loop for, merging to main seems like one of them That's what the review is for. And I'd argue that the review should be decoupled from CI, because you're not reviewing if you think the tests is passing before/after merge, you're reviewing the code and docs themselves. And once the review is done, it should be fine to merge at any point, today or tomorrow, barring any merge conflicts of course. Merging to main should be the most mundane task ever, and even a robot should be able to do it with confidence.
    - 000ooo000 3y ago
      >once the review is done, it should be fine to merge at any point, today or tomorrow Sometimes there are considerations like QA/infrastructure resourcing that dictate when something can go into master. Ideally that's rare but I have worked in a place where it's a consideration for every single merge. To expand on this a little: we had a simple branching model. To/from master for work, branch from master to cut a release. Being highly regulated, as a matter of (unchangeable) policy, every change needed testing by QA team. QA team == QA guy. Among feature work without hard release dates, there was also bug fixes with some urgency, and regulatory work with hard release dates. Given we traded flexibility in our branching model for simplicity, we instead had to consider if something could be merged to master without impacting QA load of anything already merged but not released. It worked fine for us but were were a team of <5. My remark about trading flexibility is really the crux of it all though. There's compromises that need to made and this one made sense for us at the time.
  - staindk 3y ago
    If main/master isn't your production branch then allowing auto merge makes a lot of sense. And if your changes have been QA'd in staging and/or they aren't that hectic (and are backwards compatible) then I think auto merge into prod can be fine too.
    - veidr 3y ago
      Yeah; I was assuming main is the production branch. I basically do not think "auto merge into prod" is ever OK, but then again, I let GitHub Copilot write 40-60% of my doc comments, so...
      - capableweb 3y ago
        > I basically do not think "auto merge into prod" is ever OK, but then again, I let GitHub Copilot write 40-60% of my doc comments, so... Funny how different people can be :) I'll always try to make the main branch so clean that it can be deployed at any moment, and production will always use the latest commit as soon as possible. Basically, I'm doing CI + CD, but I understand it's not for everyone.
veidr 3y ago
> Or they've done the review, wait for CI to pass then hit merge? yes (in my experience)
bluGill 3y ago
Mostly I won't even look at a review until CI passes. I don't want to look at your code if some static analysis in CI would find issues. Once in a while the code is obvious and I won't need that (the one line change "the is not spelled teh" which I've seen multiple times in different areas of code - anyone know of an IDE/tool that can spell check UI strings without tripping up on variable names), but we have a lot of static analysis that we run as part of CI, some of it standard some of it custom for people who violate our code style in some way. Note that my CI system runs multiple builds in parallel (as yours should too!). Any individual build might only take 10 minutes, but it is 2 hours if I try to run them on my local machine. Odds are if my code builds for one linux variant on x86 it will build on all the other ones and arm so I don't expect someone to run those before starting a review.
- capableweb 3y ago
  > Mostly I won't even look at a review until CI passes. I don't want to look at your code if some static analysis in CI would find issues. > but we have a lot of static analysis that we run as part of CI, some of it standard some of it custom for people who violate our code style in some way. Yeah, but why would issues that static analysis find stop you from doing the review? I never review things that automated tools can find, I care more about reviewing things only a human could review. "Does this make sense here?", "Is this decoupled enough/too much?", "Does this work well with the overall architecture?", "Does this test test the right thing?" and so on. I wouldn't spend my review time on spellchecking people or pointing out issues like that, that's for the tooling to do. And even if the CI fails because of some analysis, PR author fixes it, every review comment should still be applicable, otherwise you're just doing robot work.
  - bluGill 3y ago
    Sometimes static analisys finds something that needs significant code changes to fix. Other times it will tell you use an algorithm to do this instead of a hand written loop and so the code is a lot easier to understand after. Most of the time it doesn't matter, but I don't want to waste my time in the few cases where it does.
  - 3y ago
epage 3y ago
I've seen too many cases where people see CI is a first-pass reviewer and wait for it. I can understand if people throw complete garbage at CI that will need to be reworked to the point it isn't worth reviewing. I've rarely seen it be that case though. I instead view CI as a peer taking care of the nitty gritty while I focus on the big picture and the non-automatable. I've also worked on too many projects without a merge queue.

zimpenfish 3y ago

Anecdata of one, obviously, but I've never worked anywhere using Github that has had auto-merge turned on. You just have to wait for the "CI complete" email (or keep refreshing the page) and hit the merge button (or, in some places, ask the appropriate person to do that.)

veidr 3y ago
me either, so now anecdata of 2; but also, sounds insane of all the things a human should be in the loop for, merging to main seems like one of them
- veidr 3y ago
  (also nuking from orbit)
- capableweb 3y ago
  > of all the things a human should be in the loop for, merging to main seems like one of them That's what the review is for. And I'd argue that the review should be decoupled from CI, because you're not reviewing if you think the tests is passing before/after merge, you're reviewing the code and docs themselves. And once the review is done, it should be fine to merge at any point, today or tomorrow, barring any merge conflicts of course. Merging to main should be the most mundane task ever, and even a robot should be able to do it with confidence.
  - 000ooo000 3y ago
    >once the review is done, it should be fine to merge at any point, today or tomorrow Sometimes there are considerations like QA/infrastructure resourcing that dictate when something can go into master. Ideally that's rare but I have worked in a place where it's a consideration for every single merge. To expand on this a little: we had a simple branching model. To/from master for work, branch from master to cut a release. Being highly regulated, as a matter of (unchangeable) policy, every change needed testing by QA team. QA team == QA guy. Among feature work without hard release dates, there was also bug fixes with some urgency, and regulatory work with hard release dates. Given we traded flexibility in our branching model for simplicity, we instead had to consider if something could be merged to master without impacting QA load of anything already merged but not released. It worked fine for us but were were a team of <5. My remark about trading flexibility is really the crux of it all though. There's compromises that need to made and this one made sense for us at the time.
- staindk 3y ago
  If main/master isn't your production branch then allowing auto merge makes a lot of sense. And if your changes have been QA'd in staging and/or they aren't that hectic (and are backwards compatible) then I think auto merge into prod can be fine too.
  - veidr 3y ago
    Yeah; I was assuming main is the production branch. I basically do not think "auto merge into prod" is ever OK, but then again, I let GitHub Copilot write 40-60% of my doc comments, so...
    - capableweb 3y ago
      > I basically do not think "auto merge into prod" is ever OK, but then again, I let GitHub Copilot write 40-60% of my doc comments, so... Funny how different people can be :) I'll always try to make the main branch so clean that it can be deployed at any moment, and production will always use the latest commit as soon as possible. Basically, I'm doing CI + CD, but I understand it's not for everyone.

veidr 3y ago

> Or they've done the review, wait for CI to pass then hit merge? yes (in my experience)

bluGill 3y ago

Mostly I won't even look at a review until CI passes. I don't want to look at your code if some static analysis in CI would find issues. Once in a while the code is obvious and I won't need that (the one line change "the is not spelled teh" which I've seen multiple times in different areas of code - anyone know of an IDE/tool that can spell check UI strings without tripping up on variable names), but we have a lot of static analysis that we run as part of CI, some of it standard some of it custom for people who violate our code style in some way. Note that my CI system runs multiple builds in parallel (as yours should too!). Any individual build might only take 10 minutes, but it is 2 hours if I try to run them on my local machine. Odds are if my code builds for one linux variant on x86 it will build on all the other ones and arm so I don't expect someone to run those before starting a review.

capableweb 3y ago
> Mostly I won't even look at a review until CI passes. I don't want to look at your code if some static analysis in CI would find issues. > but we have a lot of static analysis that we run as part of CI, some of it standard some of it custom for people who violate our code style in some way. Yeah, but why would issues that static analysis find stop you from doing the review? I never review things that automated tools can find, I care more about reviewing things only a human could review. "Does this make sense here?", "Is this decoupled enough/too much?", "Does this work well with the overall architecture?", "Does this test test the right thing?" and so on. I wouldn't spend my review time on spellchecking people or pointing out issues like that, that's for the tooling to do. And even if the CI fails because of some analysis, PR author fixes it, every review comment should still be applicable, otherwise you're just doing robot work.
- bluGill 3y ago
  Sometimes static analisys finds something that needs significant code changes to fix. Other times it will tell you use an algorithm to do this instead of a hand written loop and so the code is a lot easier to understand after. Most of the time it doesn't matter, but I don't want to waste my time in the few cases where it does.
- 3y ago

epage 3y ago

I've seen too many cases where people see CI is a first-pass reviewer and wait for it. I can understand if people throw complete garbage at CI that will need to be reworked to the point it isn't worth reviewing. I've rarely seen it be that case though. I instead view CI as a peer taking care of the nitty gritty while I focus on the big picture and the non-automatable. I've also worked on too many projects without a merge queue.