search

fleetdm / fleet

Open-source platform for IT, security, and infrastructure teams. (Linux, macOS, Chrome, Windows, cloud, data center)
https://fleetdm.com
Other
3.12k stars 431 forks source link

make "Test DB Changes" a required check for merging a PR #20999

Open roperzh opened 3 months ago

roperzh commented 3 months ago

abusing the eng-initiated story flow, as this is not really a story but a request.

this workflow prevents merging migrations in the wrong order (among other DB checks). It would be good to try for a while to make it required.

lucasmrod commented 3 months ago

It also needs to re-run after updates to the target branch, right?

lukeheath commented 3 months ago

@roperzh Thanks for filing this! Agreed we need to put something in place to avoid the migration order issue yesterday. To Lucas' point, even if it was required it would have allowed merge, because it ran before the other offending migration was merged to main. So, we'd need to make it required and require all PRs to be up-to-date in order to merge them. But our CI takes so long, that you could get up-to-date, and someone else has merged into main, and now you have to re-run CI, etc., etc.

We can't afford that kind of slow down, but we also can't afford to merge migrations that crash instances on deploy. I'm trying to think of a way to avoid slowing down and still enforce migration order and welcome any ideas!

roperzh commented 3 months ago

@lucasmrod @lukeheath I think in this case we might be fine, because if your PR includes migrations you'll inevitably get conflicts in the server/datastore/mysql/schema.sql file, and you'll be forced to merge main.

Dante's PR did fail, so having the check required would have prevented it

image

My argument is that even if we don't cover 100% of the cases, this might cover the vast majority and having something is better than nothing.

The other thing to consider: because Go tests have been broken in main for a couple of days, we're kind of ignoring CI. I think that's why the PR was merged anyways.

lukeheath commented 3 months ago

@roperzh Thanks for the additional context, I didn't realize that. I'll make it required now.

@georgekarrv @sharon-fdm Any ideas on how we can effectively fix Go tests when they break? Otherwise CI tends to get ignored, which can cause things like this to happen.

lukeheath commented 3 months ago

@roperzh Test DB changes is now required on main, minor, and patch branches. Leaving this ticket open for discussion on avoiding Go test failures that hang around.

lukeheath commented 3 months ago

I maybe jumped the gun. We need to fix the migration errors on main before I can do this. I've removed it as required for now, and will add it back when main stabilizes.

sharon-fdm commented 3 months ago

My two cents: