-
Notifications
You must be signed in to change notification settings - Fork 4
[PM-15985] Add configuration for minimum release age #18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
❓ Open to suggestions of how to test this change. |
withinfocus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also see https://docs.renovatebot.com/noise-reduction/#selective-scheduling -- the docs are pretty explicit about not using what you have here to slow down fast-releasing projects. Should we consider alternatives? Is per-manager release age better, or a schedule, or this?
| "branchConcurrentLimit": 0, | ||
| "minimumReleaseAge": "7 days", | ||
| "internalChecksFilter": "strict", | ||
| "prCreation": "not-pending", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ This is pretty hot and I didn't know it existed. I looked into this and if we used status-success and we let Renovate through check runs it will be really powerful, but ... if that merged today it would stop all PR creations in our check run-ed repos right? This is fine as-is, but maybe it's a callout for a later task to move to my suggestion.
| "prConcurrentLimit": 0, | ||
| "prHourlyLimit": 0, | ||
| "branchConcurrentLimit": 0, | ||
| "minimumReleaseAge": "7 days", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Per examples like https://docs.renovatebot.com/configuration-options/#prevent-holding-broken-npm-packages what if we considered this per manager?
|
It's slow and boring but we have some private repos of certain repo clones and you can just override the Renovate settings there to see how it acts. |
|
@withinfocus Thank you for the suggestions! I've commented on each of them below rather than in separate threads.
I did see that documentation, but my understanding is that we're not using this to slow down fast-releasing projects. Rather, we're trying to let a given release "sit" for a period of 7 days so that we can ensure that a malicious dependency has time to be addressed. I felt like this purpose was the intended use of
A per-manager approach would be fine with me; would you want that still in the
Yes! I agree 100%. I think |
non-pinned.json
Outdated
| "prHourlyLimit": 0, | ||
| "branchConcurrentLimit": 0, | ||
| "minimumReleaseAge": "7 days", | ||
| "internalChecksFilter": "strict", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the default value https://docs.renovatebot.com/configuration-options/#internalchecksfilter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for catching that. It appears all of their docs are not caught up with that change. I've adjusted with 7b14dff (#18).
withinfocus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a better answer on the scheduling element -- this works, and it's really a matter of perspective. I agree with what you stated on how it's a security practice.
I think it will be here where we do per-manager release configurations, applied broadly. It would be a lot for us to expect specific repos to configure that, and it seems like a centralized security initiative. I don't have a recommendation there, but my gut feels like we'd expect longer ages for NPM specifically; we're fine as-is and this is conjecture.
We can experiment with status-success in the future and once things like the PR I just linked are in, everywhere. I am not sure why the Renovate docs explicitly say "tests" there vs. "checks" -- is that just an oversight? I would really just want it so that it would cease proposing broken changes but maybe we want those actually, especially for larger upgrades, for awareness.
Also, I'm not sure not-pending is getting us much when we run Renovate at quiet times, but it doesn't hurt.
Let's do it!
I am not sure about "tests" vs. "checks" there. I don't see anywhere in their docs where it's referred to differently that might clarify. If
The only reason I added |
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-15985
📔 Objective
Suppresses the creation of PRs for 7 days after the dependency release is stable, so that we can avoid introducing dependencies that may not have had time for security issues to be addressed.
The relevant config that was changed is:
minimumReleaseAge- Documented here.internalChecksFilter- Documented here. This is recommended to be set tostrictalong withminimumReleaseAgeto ensure that we don't bounce between release versions. This forces Renovate to strictly apply all internal checks before allowing an update.prCreation- Documented here. Several discussions on Renovate boards indicate that this is now best practice to include settingprCreationto benot-pendingalong with minimum release age configuration, in order to hold the PR from being created prematurely.⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes