I do think we can add a PR to the merge queue while bypassing branch potections (like we do for the Merge button today), but I'm not 100% sure. I like the idea of running unit tests, though I don't think we have data on how long just the unit tests run on Jenkins (since we run the "test" target which includes all tests). I'm also not sure how flaky the unit test suite is alone.
Since we already bypass the PR checks when merging, it seems that adding a required compile/check step before landing on trunk is strictly an improvement. What about this as a short term plan: 1) Add the merge queue, only run compile/check 2) Split our CI "test" job into unit and integration so we can start collecting data on those suites 3) Add "unitTest" to merge queue job once we're satisfied it won't cause disruption On Fri, Feb 9, 2024 at 11:43 AM Josep Prat <josep.p...@aiven.io.invalid> wrote: > Hi David, > I like the idea, it will solve the problem we've seen a couple of times in > the last 2 weeks where compilation for some Scala version failed, it was > probably overlooked during the PR build because of the flakiness of tests > and the compilation failure was buried among the amount of failed tests. > > Regarding the type of check, I'm not sure what's best, have a real quick > check or a longer one including unit tests. A full test suite will run per > each commit in each PR (these we have definitely more than 8 per day) and > this should be used to ensure changes are safe and sound. I'm not sure if > having unit tests run as well before the merge itself would cause too much > of an extra load on the CI machines. > We can go with `gradlew unitTest` and see if this takes too long or causes > too many delays with the normal pipeline. > > Best, > > On Fri, Feb 9, 2024 at 4:16 PM Ismael Juma <m...@ismaeljuma.com> wrote: > > > Hi David, > > > > I think this is a helpful thing (and something I hoped we would use when > I > > learned about it), but it does require the validation checks to be > reliable > > (or else the PR won't be merged). Sounds like you are suggesting to skip > > the tests for the merge queue validation. Could we perhaps include the > unit > > tests as well? That would incentivize us to ensure the unit tests are > fast > > and reliable. Getting the integration tests to the same state will be a > > longer journey. > > > > Ismael > > > > On Fri, Feb 9, 2024 at 7:04 AM David Arthur <mum...@gmail.com> wrote: > > > > > Hey folks, > > > > > > I recently learned about Github's Merge Queue feature, and I think it > > could > > > help us out. > > > > > > Essentially, when you hit the Merge button on a PR, it will add the PR > > to a > > > queue and let you run a CI job before merging. Just something simple > like > > > compile + static analysis would probably save us from a lot of > headaches > > on > > > trunk. > > > > > > I can think of two situations this would help us avoid: > > > * Two valid PRs are merged near one another, but they create a code > > > breakage (rare) > > > * A quick little "fixup" commit on a PR actually breaks something (less > > > rare) > > > > > > Looking at our Github stats, we are averaging under 40 commits per > week. > > > Assuming those primarily come in on weekdays, that's 8 commits per day. > > If > > > we just run "gradlew check -x tests" for the merge queue job, I don't > > think > > > we'd get backlogged. > > > > > > Thoughts? > > > David > > > > > > > > > > > > > > > -- > > > David Arthur > > > > > > > > -- > [image: Aiven] <https://www.aiven.io> > > *Josep Prat* > Open Source Engineering Director, *Aiven* > josep.p...@aiven.io | +491715557497 > aiven.io <https://www.aiven.io> | <https://www.facebook.com/aivencloud > > > <https://www.linkedin.com/company/aiven/> < > https://twitter.com/aiven_io> > *Aiven Deutschland GmbH* > Alexanderufer 3-7, 10117 Berlin > Geschäftsführer: Oskari Saarenmaa & Hannu Valtonen > Amtsgericht Charlottenburg, HRB 209739 B > -- David Arthur
