mozilla / bedrock

Making mozilla.org awesome, one pebble at a time
https://www.mozilla.org
Mozilla Public License 2.0
1.18k stars 913 forks source link

Dependabot PR grouping #14809

Open janbrasna opened 2 months ago

janbrasna commented 2 months ago

Description

Motivated by psycopg* that can't be updated by individual PRs now, would it be helpful if scheduled updates are grouped? Either for just psycopg* or maybe whole pip * for minor bumps (not security, they still trigger individually regardless of schedule).

Dependabot grouping makes sense for minor+patch. Major shouldn't be intertwined with these.

(I see the only issue in Selenium minor version updates being avoided constantly for some compatibility reasons. Once Selenium gets phased out, this won't be a problem anymore.)

Having regular updates in one PR/checkout also makes it easier to test, instead of individual packages' PRs (that can often fail to resolve one by one too, as with wagtail being picky about willow etc.), so current dependabot scheduled bumps are merely a reminder to redo it manually on the side and supersede it by own PR.


Success Criteria

alexgibson commented 2 months ago

Less noise, and dep update PRs ready to test and merge instead of updating manually just to close out the individual dependabot PRs by superseding them with own PRs.

This is something I'd very much like to do yeah. If we can group dependabot PRs in a more manageable way, then I'd also love to enable them for NPM dependencies too.

janbrasna commented 1 month ago

@alexgibson I've iterated over the various settings and grouping configs to get a bit meaningful results, you can check the current yml I'm somewhat comfortable with that produces these updates — there are still a few letdowns I'm jotting down. RIght now I'm not happy with updates to python lockfiles so I'll elaborate on that if I find a good setting…

Screenshot 2024-07-29 at 12 23 33

(EDIT: I should have made it clear that the linked repo used for experimenting is a copy of dependency definitions from here, so the PRs, how they're grouped, what should be done only manually etc. tries to reflect the reality here, so feel free to peek inside the grouped bumps, how does it look to you etc.…)

TL;DR if the settings (and results) for npm and docker look similar to something you'd like to achieve, I can PR those first, and look at the pip requirements with @stevejalim separately…

alexgibson commented 1 month ago

@janbrasna thanks for taking a look at this! I think this may be something we want / need to test out iteratively to see what works best.

janbrasna commented 1 month ago

I will write up the path I took, why I did some tuning of the configs etc. so you can then swipe the bits that look applicable (or you can just follow my changes through the few commits and see the resulting PRs there). TL;DR the frontend groups look alright to me if you feel like browsing around, it's the backend groups that are somewhat underwhelming in how the versions are pinned:/

alexgibson commented 1 month ago

Thanks, this will be super useful!

janbrasna commented 1 month ago

Well, to see what the PRs would look like (as the docs still leave most of the resulting effects to consumers' imagination) I went ahead and made a test repo with the exact same dependency requirements (incl. GHA workflows and Dockerfile+envfiles) to observe the effects of dependabot.yml changes trying to tune it to (my, personal) liking. I have to admit I expected perhaps a slightly higher fidelity or the ability to customize the bits that it doesn't do well, but… I'm filing feedback with them instead;)

So I've ended up including these ecosystems:

:octocat: Actions

This one is easy, it's a wildcard grouping for any major update across all the workflow files, should happen once in a few years so the only effect is all the actions will get updated in one PR to cut down the noise about it. #L60

🐳 Docker

The docs clearly state that any updates have to start with exporting labels and OCI namespaces and whatnot in the Dockerfile; that is not used here. To my surprise dependabot happily picked up the base images proposing to update them. So I excluded the python builder as that's done manually, and defined only minor+patch strategy for node image now that we're pinning an exact minor version for build stage reproducibility (and also completely excluding major version PRs, I mean, we all know there's v21 and v22, right) as again any major updates ought to be done manually (and also impacting CI, docs etc.) — so I set that manually to "docker(build):" scope and tagged that only as a frontend dependency. #L48

📦 npm

This is the first ecosystem I split into more groups, based on dependency vs. devDependency #L24 as I thought the former may need a little more scrutiny than just the linters+test, but thinking of it now the dev-deps are nonetheless being used in build pipelines so it may need another grouping after all (e.g. manifest in / vs. in /test, TBD), but this split makes the output PRs quite reasonable to manage @alexgibson. I hated how the grouping made the most prominent thing in PR title be the directory i.e. "Bump the dev-dependencies group across 1 directory with 6 updates" but couldn't really get anything more useful from that, so at least added the ecosystem prefix and dev/dep scope by the runner to have it more glanceable as "npm(dev-deps): bump the frontend group across…" where the scope in parens feels more like some angular-flavored nomenclature — so this might be better off set manually for the groups, as e.g. "npm(prod):", "npm(dev):" or "npm(test):" eventually. But besides that, the bumps are reasonable, the only thing I miss is the compatibility badge not present for packages in the grouped PR.

🐍 pip

I originally wanted to group updates for backend deps in the first place, as a) they need to be updated together, b) they are a manual work now and the PRs are merely a reminder. Turns out dependabot only supports paths, not globs/patterns so pointing it at /requirements dir won't offer any reasonable way to split prod, dev and docs updates it seems, and seeing the diffs it even doesn't update the hashes in all the places (doesn't understand referencing requirements across files?). So this one will need some more investigating with @stevejalim into how useful that even is. Worst case, we can group these to keep serving as just a reminder like today, still having to update manually, but at least without all the individual PR noise.

So here the current state is: #L4 Excluded all the deps that should stay pinned where they are, excluded those with Monterey/ARM issues until somehow resolved (funnily those are the ones that I wanted the grouped updates for in the first place;D), tried various strategies to write the hashes (widen vs. increase auto, force increase etc. with no change), but still have somewhat half-assed results and a lot of noise changing paths around etc. that I don't really like. TL;DR good to go to keep serving as a reminder, but the patch doesn't look reliable to me.

EDIT: Seems like this ADR-006 impact is still valid for the current bespoke makefile logic that's opaque to dependabot…

(Also got confirmed that 1) the dep scopes no matter what ecosystem are only those "angular"-esque values hardcoded in as two strings, so they'd be better set manually to something more relevant; and 2) that pip itself indeed can't support dev/prod scopes — they only have that for pipenv/poetry that got included under the same ecosystem label…)

janbrasna commented 1 month ago

Also one bit to point out when grouping, if there are PRs open immediately for security bumps, when time comes for the scheduled group these would also get included in the group, and the original individual security updates get superseded and closed. That kinda makes it less visible that it was an actual security advisory-triggered bump in the first place.