[Updater] Absorb Security Advisory and Ignore Conditions into Job#6989
[Updater] Absorb Security Advisory and Ignore Conditions into Job#6989
Conversation
brrygrdn
changed the base branch from
main
to
brrygrdn/dependency-snapshot-can-filter
brrygrdn
force-pushed
the
brrygrdn/dry-out-filtering-and-config
branch
from
acd6322 to
0212772
Compare
brrygrdn
changed the base branch from
brrygrdn/dependency-snapshot-can-filter
to
main
brrygrdn
force-pushed
the
brrygrdn/dry-out-filtering-and-config
branch
from
0212772 to
ee46de9
Compare
| # This is a workaround for our existing logging using the 'raw' | ||
| # ignore conditions passed into the job definition rather than | ||
| # the objects returned by `ignore_conditions_for`. | ||
| # | ||
| # The blocker on adopting Dependabot::Config::IgnoreCondition is | ||
| # that it does not have a 'source' attribute which we currently | ||
| # use to distinguish rules from the config file from those that | ||
| # were created via "@dependabot ignore version" commands |
There was a problem hiding this comment.
Thanks for the context here 😄
| job.name_normaliser.call(name1), | ||
| job.name_normaliser.call(name2) | ||
| ) | ||
| job.log_ignore_conditions_for(dependency) |
There was a problem hiding this comment.
I see this is a refactor, where behavior shouldn't change at all. I'm still going to take the opportunity to ask about the code that's being updated 😇
It feels 🤏 strange to me to have an operations class tell the job to log something. I wonder if in the longer term it would make sense for the job to return the ignore conditions in a format that can be logged. I have a feeling this is related to your comment in the job class, which I commented on.
There was a problem hiding this comment.
Yeah, this is definitely clunky, if it wasn't for the size of the method required to parse the 'raw' ignore_conditions I wouldn't mind it being duplicated.
I did also think about having a mixin that collects up all the common logging methods which might be an adequate middle ground but I punted on it as part of this as there's actually fewer shared logging methods that it seems.
My first preference would be that we effectively only have one notation of ignore conditions, as objects, available on the Job and then have a single log-presentation method but that's annoying non-trivial.
brrygrdn
force-pushed
the
brrygrdn/dry-out-filtering-and-config
branch
from
ee46de9 to
d97e14c
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.
Changes to Dependabot::Job
This PR follows up the observation that the
Dependabot::Jobalready implements asecurity_advisories_forprivate method which is duplicated and used by the Updater and now the Operation classes. By making this method public, we can just call it from the Job, removing the need to duplicate the method as well as thename_match?helper.Another source of duplicate is filtering the
job.ignore_conditionsby a given Dependency name and then hydrating them into aDependabot::Config::UpdateConfigso they can be passed into core classes asDependabot::Config::IgnoreConditionobject. By preparing theDependabot::Config::UpdateConfigwithin theDependabot::Jobonce, we can use it to provide this functionality as required.Finally, there's a duplicated log helper which needs knowledge internal to the Job that isn't otherwise required by the Updater anymore. I've moved it into the job for simplicity, but this isn't an ideal home for it. I've added a note on making this better in future but it requires a deeper change.