-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[Updater] Move filtering of parsed dependencies by allow rules or job dependencies #6991
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
Changes from all commits
85a8d29
02498cb
3b8cebe
fda8c0b
6cddb03
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -134,6 +134,14 @@ def reject_external_code? | |
| @reject_external_code | ||
| end | ||
|
|
||
| # TODO: Remove vulnerability checking | ||
| # | ||
| # This method does too much, let's make it focused on _just_ determining | ||
| # if the given dependency is within the configurations allowed_updates. | ||
| # | ||
| # The calling operation should be responsible for checking vulnerability | ||
| # separately, if required. | ||
|
Comment on lines
+139
to
+143
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
| # | ||
| # rubocop:disable Metrics/PerceivedComplexity | ||
| def allowed_update?(dependency) | ||
| allowed_updates.any? do |update| | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -69,6 +69,7 @@ def run | |
| error_handler: error_handler | ||
| ).perform | ||
| rescue *ErrorHandler::RUN_HALTING_ERRORS.keys => e | ||
| # TODO: Drop this into Security-specific operations | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤤 |
||
| if e.is_a?(Dependabot::AllVersionsIgnored) && !job.security_updates_only? | ||
| error = StandardError.new( | ||
| "Dependabot::AllVersionsIgnored was unexpectedly raised for a non-security update job" | ||
|
|
@@ -581,34 +582,19 @@ def existing_pull_request(updated_dependencies) | |
| end | ||
|
|
||
| def dependencies | ||
| all_deps = dependency_snapshot.dependencies | ||
|
|
||
| # Rebases and security updates have dependencies, version updates don't | ||
| if job.dependencies | ||
| # Gradle, Maven and Nuget dependency names can be case-insensitive and | ||
| # the dependency name in the security advisory often doesn't match what | ||
| # users have specified in their manifest. | ||
| # | ||
| # It's technically possibly to publish case-sensitive npm packages to a | ||
| # private registry but shouldn't cause problems here as job.dependencies | ||
| # is set either from an existing PR rebase/recreate or a security | ||
| # advisory. | ||
| job_dependencies = job.dependencies.map(&:downcase) | ||
| return all_deps.select do |dep| | ||
| job_dependencies.include?(dep.name.downcase) | ||
| end | ||
| return dependency_snapshot.job_dependencies if job.dependencies | ||
|
|
||
| if dependency_snapshot.dependencies.any? && dependency_snapshot.allowed_dependencies.none? | ||
| Dependabot.logger.info("Found no dependencies to update after filtering allowed updates") | ||
| return [] | ||
| end | ||
|
|
||
| allowed_deps = all_deps.select { |d| job.allowed_update?(d) } | ||
| allowed_deps = dependency_snapshot.allowed_dependencies | ||
|
Comment on lines
-602
to
+593
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm loving the ergonomics of having |
||
| # Return dependencies in a random order, with top-level dependencies | ||
| # considered first so that dependency runs which time out don't always hit | ||
| # the same dependencies | ||
| allowed_deps = allowed_deps.shuffle unless ENV["UPDATER_DETERMINISTIC"] | ||
|
|
||
| if all_deps.any? && allowed_deps.none? | ||
| Dependabot.logger.info("Found no dependencies to update after filtering allowed " \ | ||
| "updates") | ||
| end | ||
| allowed_deps = allowed_deps.shuffle unless Environment.deterministic_updates? | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I dig the naming. It adds helpful context to a variable name that I had to think about 🙂 |
||
|
|
||
| # Consider updating vulnerable deps first. Only consider the first 10, | ||
| # though, to ensure they don't take up the entire update run | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,18 +44,7 @@ def perform | |
| :created_pull_requests | ||
|
|
||
| def dependencies | ||
| # Gradle, Maven and Nuget dependency names can be case-insensitive and | ||
| # the dependency name in the security advisory often doesn't match what | ||
| # users have specified in their manifest. | ||
| # | ||
| # It's technically possibly to publish case-sensitive npm packages to a | ||
| # private registry but shouldn't cause problems here as job.dependencies | ||
| # is set either from an existing PR rebase/recreate or a security | ||
| # advisory. | ||
| job_dependencies = job.dependencies.map(&:downcase) | ||
| dependency_snapshot.dependencies.select do |dep| | ||
| job_dependencies.include?(dep.name.downcase) | ||
| end | ||
| dependency_snapshot.job_dependencies | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💥 |
||
| end | ||
|
|
||
| # rubocop:disable Metrics/AbcSize | ||
|
|
||
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.
💅 Not significant!
Are there cases where
job.dependencieswould change? It seems 🤏 surprising to have a guard clause that checks state before thereturn @ivar if defined? @ivarclause. I would typically expect that to be a part of the memoization.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.
Yeah, we could just remove that line and let it return an implicit empty set it memoizes, I kind of just wanted to call out that we make no attempt to derive the job dependencies if the source is empty - and it shouldn't ever change as all job data is effectively immutable ( something we could maybe enforce later? )