Skip to content

Commit

Permalink
removing more of dependency_has_directory feature flag (#10252)
Browse files Browse the repository at this point in the history
  • Loading branch information
@jakecoffman
jakecoffman authored Jul 19, 2024
1 parent 7dcd973 commit ff5444c
Show file tree
Hide file tree
Showing 4 changed files with 27 additions and 88 deletions.
25 changes: 4 additions & 21 deletions updater/lib/dependabot/dependency_snapshot.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,17 +112,6 @@ def add_handled_dependencies(dependency_names)
@handled_dependencies[@current_directory] = set
end

sig { params(dependencies: T::Array[{ name: T.nilable(String), directory: T.nilable(String) }]).void }
def add_handled_group_dependencies(dependencies)
raise "Current directory not set" if @current_directory == ""

dependencies.group_by { |d| d[:directory] }.each do |dir, dependency_hash|
set = @handled_dependencies[dir] || Set.new
set.merge(dependency_hash.map { |d| d[:name] })
@handled_dependencies[dir] = set
end
end

sig { returns(T::Set[String]) }
def handled_dependencies
raise "Current directory not set" if @current_directory == ""
Expand All @@ -132,7 +121,7 @@ def handled_dependencies

# rubocop:disable Performance/Sum
sig { returns(T::Set[String]) }
def handled_group_dependencies
def handled_dependencies_all_directories
T.must(@handled_dependencies.values.reduce(&:+))
end
# rubocop:enable Performance/Sum
Expand All @@ -154,11 +143,11 @@ def ungrouped_dependencies
return allowed_dependencies unless groups.any?

if Dependabot::Experiments.enabled?(:dependency_has_directory)
return allowed_dependencies.reject { |dep| handled_group_dependencies.include?(dep.name) }
return allowed_dependencies.reject { |dep| handled_dependencies_all_directories.include?(dep.name) }
end

# Otherwise return dependencies that haven't been handled during the group update portion.
allowed_dependencies.reject { |dep| T.must(@handled_dependencies[@current_directory]).include?(dep.name) }
allowed_dependencies.reject { |dep| handled_dependencies.include?(dep.name) }
end

private
Expand All @@ -178,13 +167,7 @@ def initialize(job:, base_commit_sha:, dependency_files:) # rubocop:disable Metr
@dependencies = T.let({}, T::Hash[String, T::Array[Dependabot::Dependency]])
directories.each do |dir|
@current_directory = dir
if Dependabot::Experiments.enabled?(:dependency_has_directory)
dependencies = parse_files!
dependencies_with_dir = dependencies.each { |dep| dep.directory = dir }
@dependencies[dir] = dependencies_with_dir
else
@dependencies[dir] = parse_files!
end
@dependencies[dir] = parse_files!
end

@dependency_group_engine = T.let(DependencyGroupEngine.from_job_config(job: job),
Expand Down
29 changes: 4 additions & 25 deletions updater/lib/dependabot/updater/group_update_creation.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ def compile_all_dependency_changes_for(group)

Dependabot.logger.info("Updating the #{job.source.directory} directory.")
group.dependencies.each do |dependency|
# We check dependency_snapshot.handled_dependencies instead of handled_group_dependencies here
# We check dependency_snapshot.handled_dependencies instead of handled_dependencies_all_directories here
# because we still want to update a dependency if it's been updated in another manifest files,
# but we should skip it if it's been updated in _the same_ manifest file
if dependency_snapshot.handled_dependencies.include?(dependency.name)
Expand Down Expand Up @@ -189,7 +189,6 @@ def create_change_for(lead_dependency, updated_dependencies, dependency_files, d
#
# This method **must** must return an Array when it errors
#
# rubocop:disable Metrics/AbcSize, Metrics/PerceivedComplexity
sig do
params(
dependency: Dependabot::Dependency,
Expand All @@ -213,12 +212,7 @@ def compile_updates_for(dependency, dependency_files, group) # rubocop:disable M

# Consider the dependency handled so no individual PR is raised since it is in this group.
# Even if update is not possible, etc.
if Dependabot::Experiments.enabled?(:dependency_has_directory)
dependency_snapshot.add_handled_group_dependencies([{ name: dependency.name,
directory: job.source.directory }])
else
dependency_snapshot.add_handled_dependencies(dependency.name)
end
dependency_snapshot.add_handled_dependencies(dependency.name)

if checker.up_to_date?
log_up_to_date(dependency)
Expand All @@ -239,12 +233,7 @@ def compile_updates_for(dependency, dependency_files, group) # rubocop:disable M
requirements_to_unlock: requirements_to_unlock
)
rescue Dependabot::InconsistentRegistryResponse => e
if Dependabot::Experiments.enabled?(:dependency_has_directory)
dependency_snapshot.add_handled_group_dependencies([{ name: dependency.name,
directory: job.source.directory }])
else
dependency_snapshot.add_handled_dependencies(dependency.name)
end
dependency_snapshot.add_handled_dependencies(dependency.name)
error_handler.log_dependency_error(
dependency: dependency,
error: e,
Expand All @@ -255,16 +244,10 @@ def compile_updates_for(dependency, dependency_files, group) # rubocop:disable M
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.
if Dependabot::Experiments.enabled?(:dependency_has_directory)
dependency_snapshot.add_handled_group_dependencies([{ name: dependency.name,
directory: job.source.directory }])
else
dependency_snapshot.add_handled_dependencies(dependency.name)
end
dependency_snapshot.add_handled_dependencies(dependency.name)
error_handler.handle_dependency_error(error: e, dependency: dependency, dependency_group: group)
[] # return an empty set
end
# rubocop:enable Metrics/AbcSize, Metrics/PerceivedComplexity

sig { params(dependency: Dependabot::Dependency).void }
def log_up_to_date(dependency)
Expand Down Expand Up @@ -476,10 +459,6 @@ def deduce_updated_dependency(dependency, original_dependency)
package_manager: dependency.package_manager
}

if Dependabot::Experiments.enabled?(:dependency_has_directory)
dependency_params[:directory] = dependency.directory
end

Dependabot::Dependency.new(**dependency_params)
end
end
Expand Down
36 changes: 9 additions & 27 deletions updater/lib/dependabot/updater/operations/group_update_all_versions.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ def perform
sig { returns(Dependabot::Updater::ErrorHandler) }
attr_reader :error_handler

# rubocop:disable Metrics/AbcSize, Metrics/PerceivedComplexity
# rubocop:disable Metrics/AbcSize
sig { returns(T::Array[Dependabot::DependencyGroup]) }
def run_grouped_dependency_updates
Dependabot.logger.info("Starting grouped update job for #{job.source.repo}")
Expand All @@ -114,31 +114,13 @@ def run_grouped_dependency_updates
"Deferring creation of a new pull request. The existing pull request will update in a separate job."
)

if Dependabot::Experiments.enabled?(:dependency_has_directory)

# A grouped version update gets its directories from user-defined update configs.
# A multi-directory grouped update will iterate each group over every directory.
# Therefore, we can skip a grouped dependency if it's been updated in *any* directory
# add the dependencies in the group so individual updates don't try to update them
dependency_snapshot.add_handled_group_dependencies(
dependencies_in_existing_pr_for_group(group)
.map { |d| { name: d["dependency-name"], directory: d["directory"] } }
)
# also add dependencies that might be in the group, as a rebase would add them;
# this avoids individual PR creation that immediately is superseded by a group PR supersede
dependency_snapshot.add_handled_group_dependencies(
group.dependencies.map { |d| { name: d.name, directory: d.directory } }
)
else
# add the dependencies in the group so individual updates don't try to update them
dependency_snapshot.add_handled_dependencies(
dependencies_in_existing_pr_for_group(group).filter_map { |d| d["dependency-name"] }
)
# also add dependencies that might be in the group, as a rebase would add them;
# this avoids individual PR creation that immediately is superseded by a group PR supersede
dependency_snapshot.add_handled_dependencies(group.dependencies.map(&:name))
end

# add the dependencies in the group so individual updates don't try to update them
dependency_snapshot.add_handled_dependencies(
dependencies_in_existing_pr_for_group(group).filter_map { |d| d["dependency-name"] }
)
# also add dependencies that might be in the group, as a rebase would add them;
# this avoids individual PR creation that immediately is superseded by a group PR supersede
dependency_snapshot.add_handled_dependencies(group.dependencies.map(&:name))
next
end

Expand All @@ -156,7 +138,7 @@ def run_grouped_dependency_updates
end
end
end
# rubocop:enable Metrics/AbcSize, Metrics/PerceivedComplexity
# rubocop:enable Metrics/AbcSize

sig { params(group: Dependabot::DependencyGroup).returns(T.nilable(Dependabot::DependencyChange)) }
def run_grouped_update_for(group)
Expand Down
25 changes: 10 additions & 15 deletions updater/spec/dependabot/dependency_snapshot_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@
end
end

describe "::add_handled_group_dependencies" do
describe "::handled_dependencies_all_directories" do
subject(:create_dependency_snapshot) do
described_class.create_from_job_definition(
job: job,
Expand All @@ -157,11 +157,8 @@

it "handles dependencies" do
snapshot = create_dependency_snapshot
snapshot.add_handled_group_dependencies([
{ name: "a", directory: job.source.directory },
{ name: "b", directory: job.source.directory }
])
expect(snapshot.handled_group_dependencies).to eq(Set.new(%w(a b)))
snapshot.add_handled_dependencies(%w(a b))
expect(snapshot.handled_dependencies_all_directories).to eq(Set.new(%w(a b)))
end

context "when there are multiple directories" do
Expand All @@ -185,14 +182,13 @@
it "is agnostic of the current directory" do
snapshot = create_dependency_snapshot
snapshot.current_directory = "/foo"
snapshot.add_handled_group_dependencies([
{ name: "a", directory: "/foo" },
{ name: "b", directory: "/bar" }
])
snapshot.add_handled_dependencies("a")
snapshot.current_directory = "/bar"
snapshot.add_handled_dependencies("b")

expect(snapshot.handled_group_dependencies).to eq(Set.new(%w(a b)))
expect(snapshot.handled_dependencies_all_directories).to eq(Set.new(%w(a b)))
snapshot.current_directory = "/bar"
expect(snapshot.handled_group_dependencies).to eq(Set.new(%w(a b)))
expect(snapshot.handled_dependencies_all_directories).to eq(Set.new(%w(a b)))
end
end
end
Expand Down Expand Up @@ -251,9 +247,8 @@

expect(snapshot.ungrouped_dependencies.length).to be(2)

snapshot.add_handled_group_dependencies([
{ name: group.dependencies.find { |d| d.name == "dummy-pkg-a" }.name, directory: job.source.directory }
])
snapshot.current_directory = directory
snapshot.add_handled_dependencies("dummy-pkg-a")
expect(snapshot.ungrouped_dependencies.first.name).to eql("dummy-pkg-b")

Dependabot::Experiments.reset!
Expand Down

0 comments on commit ff5444c

Please sign in to comment.