[HN Gopher] Ask HN: How are pull requests integrated in a repo w...
___________________________________________________________________
Ask HN: How are pull requests integrated in a repo with high commit
frequency?
So let's assume we open a pull request, merged current main into
our feature branch and are running tests. The full acceptance test
suite takes 5 minutes to complete. Meanwhile, in those 5 minutes,
10 other commits have been made. If we merge now, we can't be sure
that we won't break stuff right? So after merging the pull
request, are you supposed to perform another set of regression
tests? Hard to find info about that
Author : dist1ll
Score : 108 points
Date : 2022-04-21 13:04 UTC (9 hours ago)
| jakub_g wrote:
| This is a real problem in my current place (huge monorepo, 100+
| devs, 100+ merges per day).
|
| Like others said, "merge queue" is the solution. GitHub's one has
| been in beta for many months now.
|
| There are also dedicated companies doing this like
| https://mergify.com/
|
| But there's several tricky aspects like "what if I want some
| commit to jump in front of queue?", compliance etc.
|
| Due to the multitude of requirements and off-the-shelf solutions
| being somehow limited, infra folks in my place are considering
| building a custom solution :)
| jd__ wrote:
| I'd be curious to know what you find limited in off-the-shelf
| solutions?
|
| (disclaimer: I'm from the Mergify team )
| lhorie wrote:
| One question and one comment.
|
| Question: You list Uber (my employer) as a user in your
| homepage, but searching internally, the only search result
| for mergify is a mergify.yml in some random debian module for
| lua, so it doesn't seem like we actually use it for anything
| (and we built an in-house solution for the repos w/ serious
| commit traffic). I'm curious how you determine who your
| corporate users are?
|
| Comment: we ended up building a merge queue in house because
| frankly build/test pipelines can look very different,(e.g. at
| one point we had jenkins pipelines and buildkite pipelines,
| some using bazel, some not, talking to phabricator instead of
| github, etc). At the end of day plugging in a merge queue
| technology requires a bunch of work integrating with a myriad
| of things anyways. Since the engineering effort is relatively
| high regardless, doing it in house lets us experiment with
| more aggressive optimization heuristics than just waiting on
| a 3rd party.
| jd__ wrote:
| We know Uber has some great internal tooling (there's even
| a paper published on their merge queue:
| https://eng.uber.com/research/keeping-master-green-at-
| scale/). We had various Uber engineers using Mergify on
| open source repositories those last years. I didn't check
| recently if that was still the case, I'll make sure our lis
| is not too much outdated.
|
| I definitely agree with your view on how pipelines can be
| different. I think none of our customers has something that
| is exactly the same. However, many of them don't have the
| workforce to build in house or even to optimize as far as
| you would do in a (very) large company.
| lhorie wrote:
| Ah, good point, our internal search tool only indexes
| internal code, so that makes sense, thanks!
| sandspit wrote:
| There are two great saas solutions for this - mergify.io and
| mergequeue.com.
| lhorie wrote:
| The technology you're looking for goes by many names, "submit
| queue", "merge queue", "merge train", etc.
|
| In a nutshell, the idea is to keep track of currently running
| jobs, then any time a new commit enters the queue, you merge all
| the running commits into that and test that as a bundle. If
| merging fails, bail out. If that bundle job fails, bail out. If
| one of the previously running commits fail, you bail out of the
| bundle job and run another speculative bundle job without the
| failed commit. When a bundle succeeds, land all of its commits
| and abort any remaining redundant jobs.
|
| Such systems will sometimes have heuristics to bundle commits
| together in some smarter way than a naive queue (e.g. preferring
| to bundle of commits without overlapping changes, or not bundling
| small commits with huge ones to prevent infecting speculative
| builds with the slowness of the big commit, using AI to come up
| with heuristics to detect "likely to fail" commits and
| preemptively starting speculative builds without them, etc)
| dist1ll wrote:
| That's amazing, I haven't heard of these terms before. I looked
| through some of the solutions posted here (GitLab Merge Trains,
| mergify, ZUUL Gate) and they are awesome.
|
| I just felt that merging into main without having 100%
| confidence that acceptance tests would pass means that true
| continuous delivery isn't achieved. But with these tools, we
| can make sure that our software is always in a releasable state
| (according to our test suite). I learned a lot today.
| scottmsul wrote:
| Wait, is it 10 other commits, or 10 other pull requests? For
| medium-sized projects (5-10 devs) it's generally good practice
| for devs to make a separate branch per feature, make lots of
| commits on the side branch, then merge the side branch into
| master in a single PR once the feature is ready.
| jeremy_k wrote:
| It is probably a huge leap to think about using Bazel, but I
| found Uber's blog post about how they manage this issue after
| setting up a monorepo a really good read -
| https://eng.uber.com/go-monorepo-bazel/
| frays wrote:
| Great thread, thanks.
| mountainriver wrote:
| Kubernetes does this with a tool called Prow, but it's roughly
| what others have mentioned around merge trains
| Revell wrote:
| Sounds like you want to implement something like merge trains?[0]
|
| [0]: https://about.gitlab.com/blog/2020/01/30/all-aboard-merge-
| tr...
| chrisshroba wrote:
| This is a great solution and while I was at Google I noticed
| several high commit frequency teams using this strategy. Of
| course Google had built a bunch of custom tooling and
| infrastructure around it, so I can't vouch for how easy it
| would be to integrate into a different company's dev workflow,
| but if there are enough developers to make it worthwhile, then
| tasking a few developers with setting this up should be a
| useful allocation of engineer time.
| heavenlyblue wrote:
| GitLab has this feature.
| geraldcombs wrote:
| Not for fast forward merges.
| vasco wrote:
| Merge trains are the way. They do optimistic testing of the
| prior branches in the queue so the happy path goes fast.
| carlmr wrote:
| Last time I checked they don't exist in GitHub, but it seems
| like a really good way to counteract this problem.
| tdumitrescu wrote:
| Almost there: https://github.blog/changelog/2021-10-27-pull-
| request-merge-...
| jeremy_k wrote:
| To note, we signed up for the beta and got accepted in last
| week. Haven't turned it on yet though.
| [deleted]
| carlmr wrote:
| Another question I got on this. What do you do with merge
| conflicts between the branches? Do you then provide the system
| with patches for the merge conflicts somehow, so that it can
| resolve them as needed?
|
| E.g. when merging A, B and C. C might have no conflict with
| master or A but it has a conflict with B. Now for the queuing
| to work A -> B -> C it will have to know somehow how to patch C
| to fit on top of B.
|
| Maybe B breaks if it is not working after merged into A, and
| then the queue becomes A -> C -> B (modified), now B probably
| needs a patch to merge cleanly on C?
| Communitivity wrote:
| The developer whose task branch had merge conflicts was
| responsible for resolving the merge conflicts, and doing
| sidebars as needed where a discussion of the approach was
| warranted. They would resolve those conflicts in their branch
| before the PR was submitted. If the conflicts happened
| between submission and approval then the conflicts would
| still be resolved on the task branch before merge.
| carlmr wrote:
| >The developer whose task branch had merge conflicts was
| responsible for resolving the merge conflicts
|
| But that's exactly my point. The first PR to merge cleanly
| will determine whether the next PR causes a merge conflict
| or not. At the time of merging, none of these PRs had a
| merge conflict.
|
| We often have 10 conflicts between PR admission and
| approval.
| avisser wrote:
| > We often have 10 conflicts between PR admission and
| approval.
|
| I don't think a devops solution can remove those
| conflicts. At least not all of them.
|
| If you have devs in the same hot-spots of the code,
| you're going to get conflicts. Maybe refactoring can
| address some of this core issue.
| deckard1 wrote:
| The key is to reduce the number of developers touching a
| specific section of code, and line up all tickets and
| hand them over to a single developer (or two).
|
| And yeah, management won't do it. Fred Brooks wrote an
| entire book about this in 1975 that no one reads today
| and everyone would certainly ignore if they did read it.
| Because it tells you the unvarnished truth about the
| nature of communication and information flow within an
| organization. Such is the state of things in our
| industry. Sweet little lies.
| sz4kerto wrote:
| Our full acceptance test run in 3 hrs (on build servers with 64
| cores and 3xx GB memory) and we have PRs coming up every hour or
| so.
|
| We run the acceptance tests for all PRs and also the main branch
| after each change. So a given PR needs to be 'green', and if we
| merge the PR then we run the tests on the integration branch too.
| If the integration branchs gets broken somehow, then all merges
| stop -- we don't merge any further PRs into a 'broken' branch.
|
| We have tried the option of always rebasing PRs on the most
| recent integration branch and re-running the tests, but that
| results in exponential number of builds.
|
| Of course you need a meaningful build farm -- we have around 1000
| CPU cores for ~30 developers that are fully utilised during work
| hours.
| Jyaif wrote:
| 1000 CPU for 30 developers sounds enormous. What kind of
| software are you working on?
| jFriedensreich wrote:
| we rebase all branches on top of the target branch and only allow
| fast forward merges, which prevents any untested merge commits to
| cause issues and is really good for reading history. if there are
| changes to the target branch since the last rebase, there needs
| to be another rebase triggering automatic CI/CD. if there is a
| known bottleneck where this would happen a lot to a number of PRs
| and all the prs are green we rebase the prs on each other in the
| desired merge order just have to wait for the youngest PR to run
| through ci/cd and then can fast forward merge all at once.
| [deleted]
| Cerium wrote:
| I work on a project with a 3 hour CI process that handles about
| 15 to 40 PRs a day. The problem you describe here is a real
| challenge. Our PRs don't use a merge button. Once CI passes (your
| changes built on latest develop), then you can click a box that
| says this PR should merge. An automated process scoops up all
| those PRs each night and builds them together. The system test
| team tests that build in the morning. If there is a problem, that
| is debugged (we build build some bifurcation builds ahead of time
| to make this faster). Then the batch is merged to develop. This
| process has worked very well for us. Develop keeps a good pace
| but is only very rarely broken.
| dmurray wrote:
| You run the tests on the same snapshot of the code you are about
| to deploy. That might be 10 minutes or 2 hours or a month behind
| your trunk branch that got the 10 new commits.
| apetresc wrote:
| Check out something like Bors, which implements a paradigm that
| fixes this class of issue: https://github.com/bors-ng/bors-ng
|
| (Homepage: https://bors.tech/)
| mijoharas wrote:
| Does bors stand for something? what does the ng stand for? (I
| wasn't able to find out while googling.)
| nicoburns wrote:
| Historical context: https://github.com/graydon/bors
| pxeger1 wrote:
| -ng is a suffix sometimes used for "reboots" of abandoned or
| poorly maintained software; I believe it stands for "next
| generation".
| quinncom wrote:
| Thanks, I assumed that's what it meant. It's not terribly
| common, but there are a few packages using this naming
| convention: aircrack-ng, bwm-ng, crosstool-ng, libcap-ng,
| lincity-ng, minizip-ng, proxychains-ng, raxml-ng, recon-ng,
| stress-ng, zlib-ng, macfusion-ng, shadowsocksx-ng
| rrss wrote:
| does bors limit the commit frequency / PR merge frequency to
| one merge every test runtime (5 minutes in OP), or does it do
| something more complex and speculate / run some stuff in
| parallel?
|
| OP's numbers are 120 commits per hour and 5 minute test run,
| I'm wondering if adopting bors would necessarily reduce that to
| 12 commits per hour.
| henryfjordan wrote:
| Bors batches PRs, tests the batch, then merges the whole
| batch at once. It's made to handle high-throughput repos.
| krab wrote:
| It tries to build all queued changes together at first. So if
| you don't have any conflicts, there is no throughput limit.
| TheCoelacanth wrote:
| What about if something in the middle causes a failure?
| Does it have a git-bisect-like workflow to figure out the
| bad change?
| dathinab wrote:
| I can recommend it.
|
| Even if you don't have a supper high commit frequency it is
| still nice to enable certain rebase/stacked PR workflows. Which
| can remove a lot of friction even for teams which "just" have
| idk. 6 PRs merged per day (but other PRs which are based on top
| of this PRs in review and progress maybe stacked multiple
| layers deep).
| anelson wrote:
| We use bors as well. Our repo isn't particularly active, a few
| dozen committers at any one time, but it lets us have pretty
| aggressive checks on master without having to run them
| separately for each PR before it lands.
|
| Definitely recommended. We run ours on a Heroku instance it's
| cheap and almost entirely maintenance free.
| msoad wrote:
| Github is working on this for a while now. It's still in beta :(
|
| https://github.blog/changelog/2021-10-27-pull-request-merge-...
| readme wrote:
| i would perform the tests _before_ merging - rebase the pull
| request branch on top of master and run the tests, then merge
| using the --no-ff flag to ensure a merge commit is still entered
| pavel_lishin wrote:
| With a sufficient number of merges, by the time your rebased
| branch's tests have finished running, your branch is no longer
| up to date with the main branch - so you get to rebase again,
| wait for tests again, etc.
| regularfry wrote:
| It sounds like what that's missing is a while loop and a
| coffee break.
| yakubin wrote:
| At dayjob we have a "commit queue" server. When a patch is
| accepted on Gerrit, there is a button to send it to the queue.
| The queue cherry-picks the commit onto the current master branch,
| makes several builds (different configs) and runs tests. If
| everything is ok, it pushes the commit to master and marks the
| patch on Gerrit as merged. If any of the builds or tests fail, it
| doesn't push the change and instead makes a comment on Gerrit
| with a link to the broken build/test.
|
| So all commits are made sequentially, but most of the time
| developers don't need to rebase them themselves before pushing.
| The time distribution of commits sent to the queue is non-
| uniform, so you may have a long queue during the day, but by late
| evening it's pretty much empty. It has the greatest number of
| commits on Fridays.
| drewcoo wrote:
| What are your "regression tests" and why are you only worried
| about those?
| __alexs wrote:
| Such a code base is likely to be large and the changes
| distributed somewhat uniformly across it. This means that
| conflicts are actually not super likely and also that your
| assumptions are not highly likely to get invalidated by the
| commits that happen in between.
|
| If your change does break after merging, it can usually either be
| quickly fixed with a minor code change or simply reverted
| immediately.
| Aeolun wrote:
| We don't try to deal with that issue. If something breaks because
| of the merges, it'll be the next persons problem (when they make
| a new feature branch off of master and have a few failing tests).
|
| Our test suite takes about 30m to finish.
| frozenport wrote:
| https://github.com/smarkets/marge-bot
| lamontcg wrote:
| Do you really need to solve this problem?
|
| You should definitely make sure that every PR is being tested
| against a merge commit to current main and not just tested
| against the code in the PR that may have forked off weeks ago. If
| the PR has been sitting you may need to recycle the tests to test
| against current main before merging.
|
| This should take care of most problems, but doesn't guarantee
| 100% that main is never broken.
|
| I'd argue that isn't likely to be possible, and you need gates in
| between main and whatever production is and that code needs to be
| retested before deployment actions happen.
|
| When you do this you should then address how often main actually
| breaks and what the root cause (or what the accident chain is for
| people who deeply hate that term). If it is really breaking a lot
| due to races in different orthogonal commits touching the same
| sensitive locations (somehow without merge conflicts), I'd argue
| the correct course of action might be to refactor the code rather
| than the pipeline, and worrying about the pipeline is the last
| thing.
|
| And your pipeline probably should break and hold up the tests
| from time to time, that probably costs less than designing a
| complex solution which tries to be perfect for the sake of being
| perfect.
|
| Of course for the FAANG-scale readers it is probably worth it,
| but most devs aren't actually FAANG-scale, and there'll be some
| fuzzy line in the middle where it really starts to matter.
|
| But if something breaks once in a blue moon that doesn't
| necessarily mean you should always fix it, as long as you can
| always contain the damage. So how often is this really happening
| that you think you need to fix it?
| slaymaker1907 wrote:
| What we do at my job is just merge it anyway, but when failures
| happen, we work backwards to find faulting commit(s). This is
| also necessary because certain tests take too long to block PRs
| for. For people creating new branches, we also keep track of the
| last known good commit since this almost always lags compared to
| the latest commit.
| efxhoy wrote:
| We run tests on the master branch before we deploy. If tests fail
| there we find the offending PR and revert it.
| Communitivity wrote:
| I've worked on two projects in the last six years that used PRs
| and had a high commit frequency.
|
| Two things had to work or we would have lost all our sanity
| points (reference to Call of Chthulhu): automated checks of
| style, building, testing; and small PRs subscribing to the 'do
| one thing and do it well' approach.
|
| As for the second, it's more doable than you might think - if you
| decomposed your work well. We decomposed our work at these levels
| on one project: product, demo capability, epic, task; the other
| project used feature, epic, task. Typically PRs were at the task
| level, though sometimes at PR level.
|
| For us this had the added benefit that the developer velocity on
| these two projects exceeded the velocity of any of the other
| projects I've been on.
|
| Some other tricks we used were to make sure we rebased our task
| branches from develop every morning (and if needed after lunch);
| each task branch had 1 person only working on it; where
| complexity warranted we created an Epic-### branch for that epic
| and treated it as a mini-develop for tasks on that epic.
| Someone wrote:
| If you want to be sure, you do run a regression test, yes.
|
| > in those 5 minutes, 10 other commits have been made
|
| If running tests take 5 minutes, that either means you have ten
| different developers working on your code, or people committing
| to the main branch without running tests.
|
| Seems like you have too many developers working on the code for
| the size of the project and/or velocity of your approval process,
| or a spaghetti project (edit: or your tasks are too small)
|
| Removing developers from the project may decrease the time spent
| on merge conflicts so much that it makes you move faster.
| hnrodey wrote:
| Run the tests before allowing the pull request to complete?
| andersco wrote:
| Is it possible that the solution is to look at _why_ there is
| such a high frequency of commits by different devs into the same
| main branch?
|
| Rather than look for a technical solution I would explore process
| and culture changes, such as having the various devs making all
| the different commits start pairing.
| silon42 wrote:
| Or split the project into multiple modules/services?
| brianwawok wrote:
| Sounds like a good way to cut productivity in half!
| regularfry wrote:
| High frequency of commits into main (assuming they're also
| release candidates) is what we would term a good problem to
| have.
| epage wrote:
| While tools like Github check for patch conflicts, they don't
| check for semantic conflicts (e.g. one PR adds a new use of a
| function another PR removes).
|
| This is where merge queues come into play which have a variety of
| names (merge queue, submit queue, merge train, etc)
|
| I have a write up on merge queues [0] that goes into other
| benefits (e.g. speeding up PR builds), design considerations and
| their trade offs, and a comparison of various implementations.
|
| [0] https://epage.github.io/dev/submit-queue/
| msk20 wrote:
| There is ZUUL Gating[0] CI it is actually the perfect solution
| for this. It works with Github or Git based repository system.
|
| It automatically tests the changes with a simulated merge on
| master together. So it orders PR1 -> PR2 -> PR3 -> .... -> PR-100
| by order of approval. If PR1 -> PR2 (Fails) -> PR3 -> .... ->
| PR-100
|
| It restarts -> PR3 -> .... -> PR-100 and Up after removing PR2.
| This behavior is even customizable.
|
| Video of it in action: https://zuul-ci.org/media/simulation.webm
|
| Links: [0]:https://zuul-ci.org/
___________________________________________________________________
(page generated 2022-04-21 23:02 UTC)