Skip to content

Commit

Permalink
fix edge cases during semver grouping creating single PRs erroneously (
Browse files Browse the repository at this point in the history
  • Loading branch information
jakecoffman authored Aug 23, 2023
1 parent 81737dc commit 1e17026
Show file tree
Hide file tree
Showing 6 changed files with 51 additions and 15 deletions.
11 changes: 10 additions & 1 deletion common/lib/dependabot/dependency_group.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,21 @@

module Dependabot
class DependencyGroup
attr_reader :name, :rules, :dependencies
attr_reader :name, :rules, :dependencies, :handled_dependencies

def initialize(name:, rules:)
@name = name
@rules = rules
@dependencies = []
@handled_dependencies = Set.new
end

def add_to_handled(*dependencies)
@handled_dependencies += dependencies.map(&:name)
end

def add_all_to_handled
@handled_dependencies += dependencies.map(&:name)
end

def contains?(dependency)
Expand Down
9 changes: 3 additions & 6 deletions updater/lib/dependabot/dependency_snapshot.rb
Original file line number Diff line number Diff line change
Expand Up @@ -69,16 +69,13 @@ def groups
@dependency_group_engine.dependency_groups
end

def calculate_ungrouped_dependencies(dependencies_handled)
@ungrouped_dependencies = allowed_dependencies.reject { |dep| dependencies_handled.include?(dep.name) }
end

def ungrouped_dependencies
# If no groups are defined, all dependencies are ungrouped by default.
return allowed_dependencies unless groups.any?
return @ungrouped_dependencies if defined?(@ungrouped_dependencies)

@dependency_group_engine.ungrouped_dependencies
# Otherwise return dependencies that haven't been handled during the group update portion.
all_handled_dependencies = Set.new(groups.map { |g| g.handled_dependencies.to_a }.flatten)
allowed_dependencies.reject { |dep| all_handled_dependencies.include?(dep.name) }
end

private
Expand Down
10 changes: 9 additions & 1 deletion updater/lib/dependabot/updater/group_update_creation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ def create_change_for(lead_dependency, updated_dependencies, dependency_files, d
#
# This method **must** must return an Array when it errors
#
def compile_updates_for(dependency, dependency_files, group)
def compile_updates_for(dependency, dependency_files, group) # rubocop:disable Metrics/MethodLength
checker = update_checker_for(
dependency,
dependency_files,
Expand All @@ -126,6 +126,10 @@ def compile_updates_for(dependency, dependency_files, group)
return [] if all_versions_ignored?(dependency, checker)
return [] unless semver_rules_allow_grouping?(group, dependency, checker)

# Consider the dependency handled so no individual PR is raised since it is in this group.
# Even if update is not possible, etc.
group.add_to_handled(dependency)

if checker.up_to_date?
log_up_to_date(dependency)
return []
Expand All @@ -145,6 +149,7 @@ def compile_updates_for(dependency, dependency_files, group)
requirements_to_unlock: requirements_to_unlock
)
rescue Dependabot::InconsistentRegistryResponse => e
group.add_to_handled(dependency)
error_handler.log_dependency_error(
dependency: dependency,
error: e,
Expand All @@ -153,6 +158,9 @@ def compile_updates_for(dependency, dependency_files, group)
)
[] # return an empty set
rescue StandardError => e
# If there was an error we might not be able to determine if the dependency is in this
# group due to semver grouping, so we consider it handled to avoid raising an individual PR.
group.add_to_handled(dependency)
error_handler.handle_dependency_error(error: e, dependency: dependency, dependency_group: group)
[] # return an empty set
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,23 +74,21 @@ def run_grouped_dependency_updates
dependency_snapshot.groups.each do |group|
# If this group does not use update-types, then consider all dependencies as grouped.
# This will prevent any failures from creating individual PRs erroneously.
@dependencies_handled += group.dependencies.map(&:name) unless group.rules&.key?("update-types")
group.add_all_to_handled unless group.rules&.key?("update-types")

if pr_exists_for_dependency_group?(group)
Dependabot.logger.info("Detected existing pull request for '#{group.name}'.")
Dependabot.logger.info(
"Deferring creation of a new pull request. The existing pull request will update in a separate job."
)
# add the dependencies in the group so individual updates don't try to update them
@dependencies_handled += group.dependencies.map(&:name)
group.add_all_to_handled
next
end

result = run_update_for(group)
@dependencies_handled += result.updated_dependencies.map(&:name) if result
group.add_to_handled(*result.updated_dependencies) if result
end

@dependencies_handled
end

def pr_exists_for_dependency_group?(group)
Expand All @@ -108,7 +106,6 @@ def run_update_for(group)
end

def run_ungrouped_dependency_updates
dependency_snapshot.calculate_ungrouped_dependencies(@dependencies_handled)
return if dependency_snapshot.ungrouped_dependencies.empty?

Dependabot::Updater::Operations::UpdateAllVersions.new(
Expand Down
4 changes: 3 additions & 1 deletion updater/spec/dependabot/dependency_snapshot_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,9 @@
expect(group.dependencies.length).to eql(1)
expect(group.dependencies.first.name).to eql("dummy-pkg-a")

expect(snapshot.ungrouped_dependencies.length).to eql(1)
expect(snapshot.ungrouped_dependencies.length).to eql(2)

group.add_to_handled(group.dependencies.find { |d| d.name == "dummy-pkg-a" })
expect(snapshot.ungrouped_dependencies.first.name).to eql("dummy-pkg-b")

Dependabot::Experiments.reset!
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,29 @@
end
end

context "when there are semver rules but an error occurs gathering versions" do
before do
allow_any_instance_of(Dependabot::Bundler::UpdateChecker).
to receive(:latest_version).
and_raise(StandardError.new("Test error while getting latest version"))
end

let(:job_definition) do
job_definition_fixture("bundler/version_updates/group_update_all_semver_grouping")
end

let(:dependency_files) do
original_bundler_files(fixture: "bundler_grouped_by_types")
end

it "does not create individual PRs" do
expect(mock_service).not_to receive(:create_pull_request)
expect(mock_error_handler).to receive(:handle_dependency_error).exactly(3).times

group_update_all.perform
end
end

context "when the snapshot is only grouping patch-level changes and major changes are ignored", :vcr do
let(:job_definition) do
job_definition_fixture("bundler/version_updates/group_update_all_semver_grouping_with_global_ignores")
Expand Down

0 comments on commit 1e17026

Please sign in to comment.