[npm] Flag indirect transitive updates to be ignored by the FileUpdater #5873
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mctofu
force-pushed
the
mctofu/optimize-transitive-update
branch
from
b8ddf55
to
3beae68
Compare
deivid-rodriguez
approved these changes
Oct 12, 2022
updater/lib/dependabot/updater.rb
Outdated
| # Ignore dependencies that are tagged as information_only. These will be | ||
| # updated indirectly as a result of a parent dependency update and are | ||
| # only included here to be included in the PR info. | ||
| deps_to_update = updated_dependencies.reject { |d| d.metadata[:information_only] } |
There was a problem hiding this comment.
I had the thought of whether it makes sense to move this to a dependency method like #informational_only? or something, but I'm not sure if other ecosystems will need something like this so probably premature. I just thought that deps_to_update = updated_dependencies.reject(&:informational_only?) would read a bit better.
There was a problem hiding this comment.
Thanks! I was on the fence about adding that but you've persuaded me!
| @@ -1426,14 +1443,15 @@ | |||
| let(:registry_listing_url) { "https://registry.npmjs.org/transitive-dependency-locked-by-intermediate" } | |||
|
|
|||
| it "correctly updates the transitive dependency" do | |||
| expect(checker.send(:updated_dependencies_after_full_unlock)).to eq([ | |||
| expect(checker.send(:updated_dependencies_after_full_unlock)).to eq_with_metadata([ | |||
There was a problem hiding this comment.
Nice, I would maybe have chosen eq_including_metadata but I'm not great at English 😛.
There was a problem hiding this comment.
I like that wording better as well so updated to use it!
mctofu
force-pushed
the
mctofu/optimize-transitive-update
branch
from
dbac3c2
to
9930d5f
Compare
Co-authored-by: David Rodriguez <deivid-rodriguez@riseup.net>
Co-authored-by: David Rodriguez <deivid-rodriguez@riseup.net>
mctofu
force-pushed
the
mctofu/optimize-transitive-update
branch
from
9930d5f
to
c92d679
Compare
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When we need to update a parent dependency to fix an alert on a transitive child dependency we currently return both the parent & child as dependencies to update. Only the parent actually needs to be directly updated as its child dependencies will be automatically updated as part of that process. Still, we include the child dependency in the updated dependency list so it will be included in the PR title/description for context.
Since introducing #5822 that means we'll attempt to do a sub-dependency update of the child which has no effect.
This adds a flag to indicate dependencies returned by the updater that don't need to be directly updated and then filters them out before sending them to the FileUpdater. This also works for removed dependencies so we no longer need to filter them explicitly.
I'm making use of the new dependency metadata field that was added in #5801. It's currently meant for temporary data that doesn't need to be persisted or used in equals checks so I ended up working on some custom matchers to help validate the metadata output in tests. Let me know if theres a better way I could have done that!