这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@trmartin4
Copy link
Member

@trmartin4 trmartin4 commented Jan 29, 2025

🎟️ 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 to strict along with minimumReleaseAge to 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 setting prCreation to be not-pending along with minimum release age configuration, in order to hold the PR from being created prematurely.

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 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

@trmartin4 trmartin4 changed the title Add configuration for minimum release age [PM-15985] Add configuration for minimum release age Jan 29, 2025
@trmartin4 trmartin4 marked this pull request as ready for review January 29, 2025 23:56
@trmartin4
Copy link
Member Author

❓ Open to suggestions of how to test this change.

Copy link
Contributor

@withinfocus withinfocus left a 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",
Copy link
Contributor

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",
Copy link
Contributor

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?

@withinfocus
Copy link
Contributor

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.

@trmartin4
Copy link
Member Author

@withinfocus Thank you for the suggestions! I've commented on each of them below rather than in separate threads.

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?

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 minimumReleaseAge. The question of slowing down fast-releasing dependencies would need to be addressed elsewhere, via a schedule change, or ignoring patch dependencies for those particular packages, for example. Am I mis-interpreting that guidance?

❓ Per examples like docs.renovatebot.com/configuration-options#prevent-holding-broken-npm-packages what if we considered this per manager?

A per-manager approach would be fine with me; would you want that still in the renovate-config repo or in each individual repo to decide? And would there be a reason to deviate from 7 days for any of them, if the goal is security of our dependencies?

❓ 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.

Yes! I agree 100%. I think status-success would stop everything today, but when we remove the check-run from the Renovate PRs, we could adjust to status-success, I think? What cases are you thinking that this would help with? In other words, what checks would status-success help with that not-pending would not?

non-pinned.json Outdated
"prHourlyLimit": 0,
"branchConcurrentLimit": 0,
"minimumReleaseAge": "7 days",
"internalChecksFilter": "strict",
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

@trmartin4 trmartin4 Feb 2, 2025

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).

Copy link
Contributor

@withinfocus withinfocus left a 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!

@trmartin4
Copy link
Member Author

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.

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 status-success doesn't work, I'd be happy to put in an issue for Renovate to clarify.

Also, I'm not sure not-pending is getting us much when we run Renovate at quiet times, but it doesn't hurt.

The only reason I added not-pending is based on this discussion on a fairly recent issue, in which PRs were still getting created even with the release age set. I interpreted this as a gap in their documentation and that this is actually required.

@trmartin4 trmartin4 merged commit 93eb8bd into main Feb 4, 2025
2 checks passed
@trmartin4 trmartin4 deleted the platform/add-delay-for-pr-creation branch February 4, 2025 02:45
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.

4 participants