Conversation
brrygrdn
force-pushed
the
brrygrdn/dependency-snapshot-can-filter
branch
from
120ea42 to
43f450f
Compare
| # 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. |
| return [] unless job.dependencies&.any? | ||
| return @job_dependencies if defined? @job_dependencies |
There was a problem hiding this comment.
💅 Not significant!
Are there cases where job.dependencies would change? It seems 🤏 surprising to have a guard clause that checks state before the return @ivar if defined? @ivar clause. I would typically expect that to be a part of the memoization.
There was a problem hiding this comment.
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? )
| error_handler: error_handler | ||
| ).perform | ||
| rescue *ErrorHandler::RUN_HALTING_ERRORS.keys => e | ||
| # TODO: Drop this into Security-specific operations |
| allowed_deps = all_deps.select { |d| job.allowed_update?(d) } | ||
| allowed_deps = dependency_snapshot.allowed_dependencies |
There was a problem hiding this comment.
I'm loving the ergonomics of having dependency_snapshot! 👏
| Dependabot.logger.info("Found no dependencies to update after filtering allowed " \ | ||
| "updates") | ||
| end | ||
| allowed_deps = allowed_deps.shuffle unless Environment.deterministic_updates? |
There was a problem hiding this comment.
I dig the naming. It adds helpful context to a variable name that I had to think about 🙂
| dependency_snapshot.dependencies.select do |dep| | ||
| job_dependencies.include?(dep.name.downcase) | ||
| end | ||
| dependency_snapshot.job_dependencies |
brrygrdn
force-pushed
the
brrygrdn/dependency-snapshot-can-filter
branch
from
d453cc7 to
6cddb03
Compare
2 participants
Before adding two more "Operation" classes for Security Updates, we should ensure that any code that is starting to be carried into every Operation class so far is de-duplicated as much as possible.
Rather than repeat logic to filter the parsed dependencies by either those that are permitted by allow_conditions or those which intersect with the job.dependencies within the Updater, I've just moved this logic unto
Dependabot::Snapshotand slimmed down the dependencies method(s) in the various classes.