search

mapbox / node-pre-gyp

Node.js tool for easy binary deployment of C++ addons
BSD 3-Clause "New" or "Revised" License
1.1k stars 259 forks source link

Dependabot should only open PRs for meaningful updates #834

Open benmccann opened 2 days ago

benmccann commented 2 days ago

https://docs.github.com/en/code-security/dependabot/dependabot-version-updates/configuration-options-for-the-dependabot.yml-file#allow:

By default all dependencies that are explicitly defined in a manifest are kept up to date by Dependabot version updates. In addition, Dependabot security updates also update vulnerable dependencies that are defined in lock files.

This would be much better than our current config. Every single dependabot PR that's passing the CI currently looks to be just noise as none of them affect users and only affect our own lockfile. This makes it difficult to tell which PRs are actually meaningful as they're hidden amongst the ones with no impact.

cclauss commented 2 days ago

Agreed. I did this on purpose temporarily to see where we had problem dependencies and where we did not.

Once we have a v2 production release and then our dependencies upgrades have settled down, I would prefer that JS dependencies are grouped like our GitHub Actions are currently configured.

striezel commented 2 days ago

Since I am the one who opened the pull request that introduced the Dependabot configuration (https://github.com/mapbox/node-pre-gyp/pull/688), I may at least in part be to blame for the current situation. However, in my pull request the limit was set so that Dependabot only opens at most ten pull requests at a time. In my experience that limit is a good way to make sure maintainers do not get overwhelmed by the sheer amount of incoming pull requests from Dependabot. Then somebody else increased the limit to 100, and as a result there are currently over 70 open Dependabot pull requests.

That is a huge number, but something like that can unfortunately be expected. Until a week ago the latest commit on the master branch was from 14 July 2023, so there is almost a full year of version updates to catch up with. In a fast-moving ecosystem like npm / JavaScript that can be a lot. Basically, there was a backlog of updates building up for almost a year. It's just that Dependabot now makes that visible to us. But as soon as those updates have been dealt with, the number of new incoming Dependabot updates should be relatively low, even with the current configuration.

One thing that helps in dealing with so much version updates is a comprehensive and automated test suite. So the question is: Do you trust your test suite to catch possible problems that may arise when upgrading dependencies to newer versions? If the answer to that question is yes, then you can basically merge any Dependabot pull request that passes all tests. Seeing that most of the Dependabot pull requests passed, one could probably merge 60 or more of those 76 pull requests now, assuming that the test suite is good.

benmccann commented 2 days ago

The bigger problem is that none of the updates are meaningful regardless of how gradually you deal with them. In fact doing it gradually makes the problem far worse because you would have had to have done an update for every intermediate update. Test suite isn't relevant for the open PRs as none of them will affect users.

cclauss commented 2 days ago

somebody else increased the limit to 100

Guilty.

Do you trust your test suite to catch possible problems

I have less confidence than others.

One could probably merge 60 or more of those 76 pull requests now

Let's do that just after #739 is completely resolved and we are on a new production release. After that megamerge, we can group the JS updates just like #740.

None of [the open Dependabot PR] will affect users.

How do we prove that?

Thanks for your help on this.

benmccann commented 2 days ago

How do we prove that?

Because they don't edit package.json, which is what gets shipped to users. They only update package-lock.json, which isn't part of the distributable and is only used for our own testing