typescript-eslint / typescript-eslint

:sparkles: Monorepo for all the tooling which enables ESLint to support TypeScript
https://typescript-eslint.io
Other
14.91k stars 2.68k forks source link

Repo: Consider having Renovate only update major and minor versions, not patch versions #9372

Closed JoshuaKGoldberg closed 1 week ago

JoshuaKGoldberg commented 3 weeks ago

Suggestion

I'm sick and tired of getting so many Dependabot/Renovate PRs. In this package alone it's over a dozen a week [example dependencies search: first week of June 2024]. And across a few dozen repositories... I had to make https://github.com/JoshuaKGoldberg/prune-github-notifications just to have a usable GitHub inbox.

One potential way to trim down on these updates could be to no longer take in patch updates automatically. The default could be to take in only major and minor versions.

Is this ... a good idea? Bad idea? Input needed!

⁉️

bradzacher commented 3 weeks ago

We could always set them to automerge. I don't love the idea of bypassing protections but it's an option to automate things at least.

rubiesonthesky commented 2 weeks ago

Adding weekly schedule is also an option. And some dependencies could be updated even less frequently, like just dev tools like knip etc. - easiest way is to add preset 'schedule:weekly' to enable schedule for all.

Also I think currently the config does not use all possible groupings that renovate offers out of box. But that is probably not the biggest problem.

I would also consider, should automatic nx updates be disabled since they always need human intervention. So the question for that: Is there benefit for creating the branch and PR automatically?

I think, reducing the churn for maintainers but also for CI is good thing. I have not checked the config for a while so my knowledge of it may not be up to date :)

mcmxcdev commented 2 weeks ago

Something that worked really well for freelance client projects I worked on: one monthly non-major (patch, minor) deps upgrade PR and one PR per major dep upgrade delayed by a month to ensure its more stable. I understand that a consumer lib like this might have different requirements though.

bradzacher commented 2 weeks ago

That generally doesn't work well because things change and break even in non majors. Behaviours change in subtle ways. Lint rules add new errors. Prettier tweaks its formatting.

It can be really hard to remediate things when you upgrade 15+ packages and a bunch of tests and CI jobs start to fail.

There are a few PRs we've got open right now from renovate with errors that we need to investigate and fix from just one package change. It would be horrid if we had those errors buried along with many other upgrades. We'd probably end up blocking all dep upgrades until someone has a bandwidth to manually pull the branch to investigate.

mcmxcdev commented 2 weeks ago

It's subjective, you have to make sure everything still works for the minor/patch upgrades of course, but at the same time you ensure that 3rd party deps are compatible with major dep upgrades that you do afterwards. Can't be so bad, since it's even an official renovate preset: https://docs.renovatebot.com/presets-group/#groupallnonmajor

Probably most time efficient, instead of doing minor/patch one by one where one dep could just be incompatible with another one until you upgrade that one too.

rubiesonthesky commented 2 weeks ago

Since many deps are defined as version range, one idea could be to have weekly “lock file maintenance” task. Then other deps could be set to monthly schedule or similar. That way there would be probably only 1 PR usually and it should not be failing or then the accepted ranges would be incorrectly set. I think this would mean that you get patch updates for most deps in one go, but minor and major deps would get their own PRs less frequently. If this weekly PR would be failing that would also mean, that the accepted ranges should be fixed? Also, I feel that it should be safe for any maintainer to just look at that PR quickly to see that everything is still passing and merge it.

Also, I think it could be beneficial to recognise which kind of dependencies need more frequent updates than others. It’s a work tho.

If Joshua's suggestion feels easiest to do right now, I don’t see any reasons not to try it. It’s not permanent change and if it does not work for this repo, then you can revert it and try something different :)

bradzacher commented 2 weeks ago

You'd be surprised how often minor bumps can fail CI when done en masse.

Here's an example renovate config that does a grouping of updates on a schedule though https://github.com/camunda/infraex-common-config/blob/main/default.json5

The thing I'm worried about is that if a PR fails CI it'll likely sit there in perpetuity waiting for someone to investigate and fix it.

But maybe I'm worrying about nothing and It would be worth us exploring this.

rubiesonthesky commented 1 week ago

The current renovate config is working differently than I would expect it to. Major updates should wait for dependency dashboard approval before they are opened, but then this PR was opened (#9407) - either someone approved it or it's because of the grouping. The config should be correct according to the documentation: https://docs.renovatebot.com/key-concepts/dashboard/#require-approval-for-major-updates

Current config:

  major: {
    // most majors will require some manual effort to upgrade to, so we don't want to create
    // PRs automatically or else we'll just spam ourselves.
    dependencyDashboardApproval: true,
  },

There is also part for automerging, but the bot does not have right to automerge. If the config is to be touched, this section could be removed I think:

    // automerge everything but major updates
    {
      matchUpdateTypes: ['minor', 'patch', 'pin', 'digest'],
      automerge: true,
      automergeStrategy: 'squash',
    },

Things I would suggest right now:


Other findings ### Deprecated It seems that these deps are deprecated and could be removed - @types/eslint-visitor-keys - @types/marked ### Unnecessary deps - `stylelint-config-standard` includes / extends `[stylelint-config-recommended](https://github.com/stylelint/stylelint-config-recommended)` - so `stylelint-config-recommended` could be removed from stylelint config and it could be uninstalled ### Deps that could be updated? - this PR could be re-tried now again? #7427
JoshuaKGoldberg commented 1 week ago
Screenshot of many various patch-level dependency update PRs

I'm going to mark this as team assigned. 🫠