Skip to content

chore: fix dependabot grouping#3142

Closed
rwaskiewicz wants to merge 1 commit into
mainfrom
rw/hack-to-fix-db-grouping
Closed

chore: fix dependabot grouping#3142
rwaskiewicz wants to merge 1 commit into
mainfrom
rw/hack-to-fix-db-grouping

Conversation

@rwaskiewicz

Copy link
Copy Markdown
Contributor

Asana task: N/A

Description

This commit changes the ordering of Dependabot update grouping such that the predefined groups take precedence over the npm-minor group. Prior to this commit, major version bumps of groups (e.g. eslint) were being captured by the npm-minor group, causing major version bumps from the same family of packages to be split up (and likely unmergable due to peer dependencies)

  • Tests added?

This commit changes the ordering of Dependabot update grouping such that
the predefined groups take precedence over the `npm-minor` group. Prior
to this commit, major version bumps of groups (e.g. eslint) were being
captured by the `npm-minor` group, causing major version bumps from the
same family of packages to be split up (and likely unmergable due to
peer dependencies)
Comment thread .github/dependabot.yml
update-types:
- "patch"
- "minor"
exclude-patterns:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't like this in terms of maintainability. However, it's the only thing I've gotten to work in a separate (private) repo while noodling on it in my own time to try to relearn how Dependabot works.

My current thought is "if it's dumb and works, maybe it's not dumb" - at least until we find a better solution and can get through the babel upgrade that Dependabot brought our way this morning.

I'm open to closing this, and/or putting a task on the board to properly investigate a better solution. This feels so much like a hack and runs contrary to a good bit of the documentation

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is what it takes I would rather simply eliminate all of the groups other than the minor-updates ones — which I believe Brett suggested previously. I wanted to keep the existing groups since it would theoretically let us bundle together major updates of "related" packages that would otherwise be incompatible with each other, but given major updates don't happen that often and are more likely to require some manual code changes anyway, I don't think this would be too much of a downgrade in our experience — in fact, this is the experience we already have on most other repos.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can support that. I'm going to close this and cross reference it in a separate PR

@rwaskiewicz rwaskiewicz marked this pull request as ready for review June 25, 2026 13:03
@rwaskiewicz rwaskiewicz requested a review from a team as a code owner June 25, 2026 13:03
rwaskiewicz added a commit that referenced this pull request Jun 25, 2026
Remove package-specific dependency groupings from the Dependabot configuration. Prior to this commit, we were seeing cases where NPM major version updates were falling into the `npm-minor` group. This caused related packages that were peer-dependent on one another's new major version to be split into separate PRs, causing each created PR to fail. The outcome following this PR is the same,
and will require manual intervention from the dev team to fix cases of broken major version updates. This simply removes the groups to avoid future maintenance.

One approach in #3142 was to move `npm-minor` to the bottom of the group list, and explicitly exclude each package in any group from `npm-minor`. This was tested to fix the above issue in a separate repo, but adds maintenance burden. As a result, we choose to simply remove the groups as they exist today
@digitalcora

Copy link
Copy Markdown
Contributor

Prior to this commit, major version bumps of groups (e.g. eslint) were being captured by the npm-minor group, causing major version bumps from the same family of packages to be split up (and likely unmergable due to peer dependencies)

Can you point to a PR where this happened? I just went looking for one and couldn't actually find it.

@digitalcora digitalcora deleted the rw/hack-to-fix-db-grouping branch June 25, 2026 20:30
@rwaskiewicz

Copy link
Copy Markdown
Contributor Author

This recently occurred in a Dependabot action run - in the logs there's:

updater | 2026/06/25 14:07:07 INFO <job_1435213457> Skipping eslint for group 'eslint' - belongs to more specific group 'npm-minor'
...
updater | 2026/06/25 14:07:07 WARN <job_1435213457> Please check your configuration as there are groups where no dependencies match:

  • eslint
  • stylelint
  • babel
  • moment

@rwaskiewicz

Copy link
Copy Markdown
Contributor Author

eslint is probably somewhat of an outlier here - we're waiting on eslint-react-plugin to gain eslint 10 compatibility, so while Dependabot should have grouped it (AFAIK it doesn't do any dependency tree analysis at the stage in the logs I presented), it still would have failed due to peer dependency issues

@digitalcora

Copy link
Copy Markdown
Contributor

I guess (in retrospect) I would have liked to actually see a PR that has the problem you describe (a major version update being included in a PR for the npm-minor group). In the absence of that problem having concretely happened, was there really anything to "fix" here? From past experience, I wouldn't trust the logs alone to indicate that something's wrong.

@rwaskiewicz

rwaskiewicz commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

The problem was rather we had 2 PRs being created (e.g. #2999 and #3139) as a consequence of them each falling into the npm-minor group. So while they were considered to be group members as the Dependabot config describes, they were given their own PRs, rather than say eslint and eslint/js being in the same PR (which is was I understand to be (and personally) preferred).

EDIT: That's what this PR intended to fix, to combine these dependencies into a single PR

@digitalcora

Copy link
Copy Markdown
Contributor

So while they were considered to be group members as the Dependabot config describes, they were given their own PRs

Right... I think that was something we already knew was happening, and had just never bothered to actually delete all the named groups since it was a no-op (it wasn't doing what we wanted, and after deleting them, it still wouldn't do what we wanted, in exactly the same way).

EDIT: That's what this PR intended to fix, to combine these dependencies into a single PR

I see, so this PR specifically would have actually fixed everything to behave the way we wanted from the start, with both working -minor groups and working tool-specific groups just for major updates — but at an undesirable cost in duplicated configuration. I don't think I really understood that until now, at least assuming that is a correct understanding.

@rwaskiewicz

rwaskiewicz commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

Right... I think that was something we already knew was happening, and had just never bothered to actually delete all the named groups since it was a no-op (it wasn't doing what we wanted, and after deleting them, it still wouldn't do what we wanted, in exactly the same way).

I see, so I see, so this PR specifically would have actually fixed everything to behave the way we wanted from the start, with both working -minor groups and working tool-specific groups just for major updates — but at an undesirable cost in duplicated configuration. I don't think I really understood that until now, at least assuming that is a correct understanding. I don't think I really understood that until now, at least assuming that is a correct understanding.

(To both bolded above) Correct, I saw the babel bumps this morning, and knew that this was a way to fix the config, but with that cost. Apologies if that wasn't clear - ideally there'd be a way to show what the outcome would have been w/o merging it, and can see how the description could have been interpreted differently

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants