-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Fix updating GitHub Actions & Dockerfiles with mixed versions #6082
Fix updating GitHub Actions & Dockerfiles with mixed versions #6082
Conversation
25a46ff
to
97d964d
Compare
Interesting. If I understand correctly, when the same dependency is defined in multiple places, the intention is to choose the lowest version so that nothing is misinterpreted as up to date. And that's what I think it's the culprit for #5633 and what I'm trying to fix here. However, there are broken specs on several ecosystems, but I'm not really convinced they signal an issue with my fix. If someone else can have a look, I appreciate it! |
97d964d
to
bac23a3
Compare
I rebased this PR after noticing yet another issue it would fix. In #6336, Dependabot confusingly seems to be trying to upgrade the base Docker image to I'm going to have another look at the failing tests to confirm that the issue is with the tests asserting the wrong thing and not with the implementation proposed here. |
This PR would also partially fix #6700. |
0da1f84
to
5a707c8
Compare
5a707c8
to
65d474b
Compare
I worked through the test failures and they all looked as "bad expectations" to me, so I went ahead and updated the tests. I will add inline comments on the updated tests to elaborate. |
@@ -301,7 +301,7 @@ | |||
expect(dependency_set.dependency_for_name("foo")).to eq( | |||
Dependabot::Dependency.new( | |||
name: "foo", | |||
version: "1.0", | |||
version: "1.1", |
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.
If one dependency in the set is a top level dependency (the one at 1.1 in this case), and another one is not (the one at 1.0), then the version of the top level dependency is chosen.
The previous logic would only do this sometimes, in a way that was dependent on the order dependencies are parsed. If a top level dependency would be parsed first, and then an indirect dependency merged into the Dependency Set, then the version of the top level dependency would be respected. But the version of the indirect dependency would win if lower and if the indirect dependency parsed first.
The new logic has the same spirit but it's order independent.
@@ -628,7 +628,7 @@ | |||
expect(dependency).to be_a(Dependabot::Dependency) | |||
expect(dependency.name). | |||
to eq("org.apache.maven.plugins:maven-javadoc-plugin") | |||
expect(dependency.version).to eq("3.0.0-M1") | |||
expect(dependency.version).to eq("2.10.4") |
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.
The new logic prefers lower versions if the dependencies are of the same type. This makes sense because it prevents misdirecting things as out of date, and was already the "spirit" of the code. But, due to this bug, sometimes the higher version would win depending on parsing order.
@@ -325,7 +325,7 @@ | |||
expect(combined_set.dependency_for_name("foo")).to eq( | |||
Dependabot::Dependency.new( | |||
name: "foo", | |||
version: "1.0", | |||
version: "1.1", |
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.
Same explanation as in common/spec/dependabot/file_parsers/base/dependency_set_spec.rb (version of top-level dependency wins).
@@ -143,7 +143,7 @@ | |||
), | |||
Dependabot::Dependency.new( | |||
name: "bar", | |||
version: "0.2.3", | |||
version: "0.2.1", |
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.
Same explanation as in maven/spec/dependabot/maven/file_parser_spec.rb (lowest version wins).
@@ -80,7 +80,7 @@ | |||
it "has the right details" do | |||
expect(dependency).to be_a(Dependabot::Dependency) | |||
expect(dependency.name).to eq("Microsoft.Extensions.DependencyModel") | |||
expect(dependency.version).to eq("1.1.1") | |||
expect(dependency.version).to eq("1.0.1") |
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.
Same explanation as in the maven/spec/dependabot/maven/file_parser_spec.rb (lowest version wins).
65d474b
to
360707b
Compare
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.
Makes sense! We weren't expecting a top level dep to have multiple versions but it can happen for ecosystems where we process top level deps from multiple files at once.
360707b
to
984a267
Compare
Thanks for the review @mctofu! |
…-with-mixed-versions
Fixes #3750.
Fixes #5633.