Read npm min-release-age from .npmrc and apply as cooldown#15132
Read npm min-release-age from .npmrc and apply as cooldown#15132yeikel wants to merge 3 commits into
Conversation
e5daee2 to
b07b1cf
Compare
|
@yeikel This is great, thank you. I need to check with the team to make sure this is the direction we want to take, and I'll get back to you. |
|
@yeikel Thanks for driving these changes, and we agree with your approach. The floor logic is exactly right: One question: does this also work for pnpm and yarn, or is it npm-only right now? They have similar |
Awesome, I’m glad we’re aligned Just for the record, differences such as inclusion/exclusion are features that are expected to land in npm in a future feature release, so we should be able to reach feature parity over time. npm/cli#8994
Yes, this is what I know so far but I still need to do some additional research for the TODO items below:
Maybe we can create a meta issue using the table above if it helps to track this easier.
What do you think would work best? The reason I started with npm first was mainly for two reasons:
As for the follow-up work, as you know I enjoy contributing to this project, so I’d be happy to pick up the remaining work. The hardest part of contributing to core is usually getting consistent reviews, which I completely understand given the limited capacity. If you’re able to help review, I’d be happy to handle the implementation and get your help to move them forward Likewise, if you would prefer to develop some of it yourself, I’d also be happy to help test things out. Let me know what you think would work best 🙏 |
|
@yeikel This all makes sense to me, and I think your incremental approach is the right one. This all makes sense to me, and I think your incremental approach is the right one. Shipping this and #15139 first, then handling the other ecosystems as separate PRs, fits well with how we like to keep changes atomic and reviewable, and getting them out early for community feedback is a nice bonus. A meta issue tracking the table above would be really helpful, please go ahead and create one. That'll make it much easier to see the state of each ecosystem at a glance. Thanks also for the npm/cli#8994 pointer, good to know inclusion/exclusion parity should come over time. On splitting the work: I really appreciate all the work you put into driving Dependabot forward. I'd never want to assign work to you as a contributor or take your time for granted, and it's honestly not my place to hand out tasks. So I'm more than happy for you to drive the implementation yourself if that's what you'd like, but I'd never expect it of you. Whatever you pick up is genuinely appreciated. What I can do is make sure you get consistent reviews from our side to keep things moving, since that's usually the real bottleneck. We're putting more effort into reviewing community pull requests, it'll take time but we're making reviews more of a priority. So my suggestion: you keep implementing whatever you're happy to take on, and I'll review and help shepherd them through. If there's a part you'd rather not work on, just tag me in it and I'll assign the ticket to myself. Also tag me in any prs related and I will make sure the pr is reviews quickly as this is an issue affected a lot of users. I'll leave that decision to you. |
- `default_days` now uses `[existing.default_days, npmrc_days].max` so the npmrc value never lowers a higher value set in dependabot.yml - Conflict warning is conditional: only fires when at least one cooldown field is actually below the npmrc threshold; otherwise logs at debug - Removed `default_days` from the per-field override-warning loop; the top-level conflict message covers it; per-field messages are for the three `semver_*_days` fields only - `.npmrc` filename match tightened to `File.basename(f.name) == ".npmrc"` to avoid false matches like `foo.npmrc` - Extracted `log_npmrc_cooldown_conflicts` and `merge_cooldown_with_npmrc_floor` helpers (satisfies Rubocop Metrics cops) - TODO comment added for Yarn 4.10+ `npmMinimalAgeGate` follow-up Spec changes: - "when explicit update_cooldown already exceeds the npmrc floor": asserts default_days stays unchanged and no warning is logged - "when dependabot.yml default_days is below the npmrc floor": expects conflict warning once and per-field warnings for semver fields only - Replace `instance_variable_get(:@update_cooldown)` with `checker.update_cooldown`
b07b1cf to
90b1758
Compare
Sounds good to me. Please let me know if you have any feedback/what is pending to ship these two for now
I created #15157
That's fair and understandable; I know that it is not expected but I like helping out as I also benefit
Sounds good. As soon as this and the other are merged, I'll pick up the next. Based on refinement, that seems to be yarn but I'd be happy to pivot based on what makes more sense. I ignore how many users we have on each ecosystem, but I do know that we got a direct report from users for yarn specifically (but not pnpm yet) |
|
@yeikel I think we should prioritise PNPM is affecting the most users at the moment, I am going to start reviewing these prs and get them deployed |
Summary
min-release-agesetting in.npmrcprevents installing any package version younger than N days. Dependabot was unaware of this setting, so it could try to open PRs for versions that would violate user's configurationThis pull request reads
min-release-agefrom.npmrcand uses its value as a lower bound on everyupdate_cooldownfield. If noupdate_cooldownis configured independabot.yml, one is created from the npmrc value. If one is already configured, the npmrc value only raises individual fields that are below itFixes #14151
Merge semantics
update_cooldownindependabot.ymldefault_days=.npmrcvaluedependabot.ymldefault_days>= npmrc valuedependabot.ymldefault_days< npmrc valuedefault_daysraised to npmrc floor; warn loggedsemver_*_daysfield < npmrc valueinclude/excludepatternsmin-release-ageglobally with no per-package filteringThe npmrc value acts as a floor: it can only raise cooldown values, never lower them.
Anything you want to highlight for special attention from reviewers?
.npmrcanddependabot.ymlcooldown settings don't translate cleanly. This is, for the most part, because Dependabot support features that npm does not support yetnpm's
min-release-ageis a global, unconditional install-time constraint with no per-package filteringdependabot.ymlsupportsinclude/excludepatterns and per-semver thresholds that have no npm equivalent. When both are present, the choices are: honourdependabot.ymlas-is and risk Dependabot proposing versionsnpm installimmediately rejects; or usemin-release-ageas a floor and silently drop incompatibledependabot.ymlfeaturesThis PR takes the second path on the grounds that a broken install is worse than a dropped preference, with a log warning to surface the trade-off. A third option such as failing the pipeline on conflict seems more damaging.
How will you know you've accomplished your goal?
Reproducer: https://github.com/yeikel/dependabot-issue-14151
Without this change, dependabot happily suggests the latest version of my dependency at test(3.5.34) ignoring my
min-release-ageconfiguration :However, after this change, it suggests the version the configuration expects(3.4.27):
https://github.com/yeikel/dependabot-issue-14151/blob/bfc9181bbe5b21e8c9f2a8b0853506164946cdab/.npmrc#L1
Checklist