Skip to content
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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Feb 24, 2023
Merged

Fix updating GitHub Actions & Dockerfiles with mixed versions #6082

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
984a267
Fix updating GitHub Actions with mixed versions
deivid-rodriguez Nov 8, 2022
9b61bb9
Merge branch 'main' into deivid-rodriguez/fix-updating-github-actions…
jakecoffman Feb 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 19 additions & 12 deletions common/lib/dependabot/file_parsers/base/dependency_set.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,19 +117,10 @@ def <<(dep)

# Produces a new dependency by merging the attributes of `old_dep` with those of
# `new_dep`. Requirements and subdependency metadata will be combined and deduped.
# The version of the combined dependency is determined by the logic below.
# The version of the combined dependency is determined by the
# `#combined_version` method below.
def combined_dependency(old_dep, new_dep)
version = if old_dep.top_level? # Prefer a direct dependency over a transitive one
old_dep.version || new_dep.version
elsif !version_class.correct?(new_dep.version)
old_dep.version
elsif !version_class.correct?(old_dep.version)
new_dep.version
elsif version_class.new(new_dep.version) > version_class.new(old_dep.version)
old_dep.version
else
new_dep.version
end
version = combined_version(old_dep, new_dep)
requirements = (old_dep.requirements + new_dep.requirements).uniq
subdependency_metadata = (
(old_dep.subdependency_metadata || []) +
Expand All @@ -145,6 +136,22 @@ def combined_dependency(old_dep, new_dep)
)
end

def combined_version(old_dep, new_dep)
if old_dep.version.nil? ^ new_dep.version.nil?
[old_dep, new_dep].find(&:version).version
elsif old_dep.top_level? ^ new_dep.top_level? # Prefer a direct dependency over a transitive one
[old_dep, new_dep].find(&:top_level?).version
elsif !version_class.correct?(new_dep.version)
old_dep.version
elsif !version_class.correct?(old_dep.version)
new_dep.version
elsif version_class.new(new_dep.version) > version_class.new(old_dep.version)
old_dep.version
else
new_dep.version
end
end

def version_class
@version_class ||= Utils.version_class_for_package_manager(@combined.package_manager)
end
Expand Down
4 changes: 2 additions & 2 deletions common/spec/dependabot/file_parsers/base/dependency_set_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@
expect(dependency_set.dependency_for_name("foo")).to eq(
Dependabot::Dependency.new(
name: "foo",
version: "1.0",
version: "1.1",
Copy link
Contributor Author

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.

requirements: (
foo_v1.requirements +
foo_sha.requirements +
Expand All @@ -325,7 +325,7 @@
expect(combined_set.dependency_for_name("foo")).to eq(
Dependabot::Dependency.new(
name: "foo",
version: "1.0",
version: "1.1",
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez Feb 23, 2023 •
edited
Loading

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).

requirements: (
foo_v1.requirements +
foo_sha.requirements +
Expand Down
3 changes: 1 addition & 2 deletions github_actions/lib/dependabot/github_actions/update_checker.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,9 @@ def lowest_resolvable_security_fix_version
lowest_security_fix_version
end

def updated_requirements # rubocop:disable Metrics/PerceivedComplexity
def updated_requirements
previous = dependency_source_details
updated = updated_source
return dependency.requirements if updated == previous

# Maintain a short git hash only if it matches the latest
if previous[:type] == "git" &&
Expand Down
2 changes: 1 addition & 1 deletion maven/spec/dependabot/maven/file_parser_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Contributor Author

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.

expect(dependency.requirements).to eq(
[{
requirement: "3.0.0-M1",
Expand Down
2 changes: 1 addition & 1 deletion npm_and_yarn/spec/dependabot/npm_and_yarn/helpers_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@
),
Dependabot::Dependency.new(
name: "bar",
version: "0.2.3",
version: "0.2.1",
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez Feb 23, 2023 •
edited
Loading

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).

requirements: (bar_c.requirements + bar_b.requirements + bar_a.requirements).uniq,
package_manager: "npm_and_yarn",
metadata: { all_versions: [bar_c, bar_b, bar_a] }
Expand Down
2 changes: 1 addition & 1 deletion nuget/spec/dependabot/nuget/file_parser_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez Feb 23, 2023 •
edited
Loading

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).

expect(dependency.requirements).to eq(
[{
requirement: "1.1.1",
Expand Down