From e9aaefacca6f6c439f20e6cae06f53931a599549 Mon Sep 17 00:00:00 2001 From: Jake Coffman Date: Fri, 19 Jul 2024 09:02:46 -0500 Subject: [PATCH 1/3] revert add_handled_group_dependencies --- updater/lib/dependabot/dependency_snapshot.rb | 19 +----- .../updater/group_update_creation.rb | 27 +------- .../operations/group_update_all_versions.rb | 34 +++-------- .../dependabot/dependency_snapshot_spec.rb | 61 +------------------ 4 files changed, 13 insertions(+), 128 deletions(-) diff --git a/updater/lib/dependabot/dependency_snapshot.rb b/updater/lib/dependabot/dependency_snapshot.rb index d347a2c20fd..b0bb7445d65 100644 --- a/updater/lib/dependabot/dependency_snapshot.rb +++ b/updater/lib/dependabot/dependency_snapshot.rb @@ -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 == "" @@ -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), diff --git a/updater/lib/dependabot/updater/group_update_creation.rb b/updater/lib/dependabot/updater/group_update_creation.rb index c90c1fcfa15..5b7785cecbe 100644 --- a/updater/lib/dependabot/updater/group_update_creation.rb +++ b/updater/lib/dependabot/updater/group_update_creation.rb @@ -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, @@ -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) @@ -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, @@ -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) @@ -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 diff --git a/updater/lib/dependabot/updater/operations/group_update_all_versions.rb b/updater/lib/dependabot/updater/operations/group_update_all_versions.rb index 14b7561220b..c98b90494d3 100644 --- a/updater/lib/dependabot/updater/operations/group_update_all_versions.rb +++ b/updater/lib/dependabot/updater/operations/group_update_all_versions.rb @@ -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}") @@ -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 diff --git a/updater/spec/dependabot/dependency_snapshot_spec.rb b/updater/spec/dependabot/dependency_snapshot_spec.rb index c7a42f3717e..822ad645da1 100644 --- a/updater/spec/dependabot/dependency_snapshot_spec.rb +++ b/updater/spec/dependabot/dependency_snapshot_spec.rb @@ -140,63 +140,6 @@ end end - describe "::add_handled_group_dependencies" do - subject(:create_dependency_snapshot) do - described_class.create_from_job_definition( - job: job, - job_definition: job_definition - ) - end - - let(:job_definition) do - { - "base_commit_sha" => base_commit_sha, - "base64_dependency_files" => encode_dependency_files(dependency_files) - } - end - - 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))) - end - - context "when there are multiple directories" do - let(:directory) { nil } - let(:directories) { %w(/foo /bar) } - let(:dependency_files) do - [ - Dependabot::DependencyFile.new( - name: "Gemfile", - content: fixture("bundler/original/Gemfile"), - directory: "/foo" - ), - Dependabot::DependencyFile.new( - name: "Gemfile", - content: fixture("bundler/original/Gemfile"), - directory: "/bar" - ) - ] - end - - 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" } - ]) - - expect(snapshot.handled_group_dependencies).to eq(Set.new(%w(a b))) - snapshot.current_directory = "/bar" - expect(snapshot.handled_group_dependencies).to eq(Set.new(%w(a b))) - end - end - end - describe "::create_from_job_definition" do subject(:create_dependency_snapshot) do described_class.create_from_job_definition( @@ -251,9 +194,7 @@ 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.add_handled_dependencies(group.dependencies.find { |d| d.name == "dummy-pkg-a" }.name) expect(snapshot.ungrouped_dependencies.first.name).to eql("dummy-pkg-b") Dependabot::Experiments.reset! From a2810dace5de7ada5542ff90f53787edee00ba9e Mon Sep 17 00:00:00 2001 From: Jake Coffman Date: Fri, 19 Jul 2024 09:13:49 -0500 Subject: [PATCH 2/3] lint: remove enable which is not needed anymore --- .../dependabot/updater/operations/group_update_all_versions.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/updater/lib/dependabot/updater/operations/group_update_all_versions.rb b/updater/lib/dependabot/updater/operations/group_update_all_versions.rb index c98b90494d3..06a365bf78e 100644 --- a/updater/lib/dependabot/updater/operations/group_update_all_versions.rb +++ b/updater/lib/dependabot/updater/operations/group_update_all_versions.rb @@ -138,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) From 6de4df9a9cc8eff8b6693e16fa1ba42beafd05e9 Mon Sep 17 00:00:00 2001 From: Jake Coffman Date: Fri, 19 Jul 2024 10:04:13 -0500 Subject: [PATCH 3/3] rename to be more clear what it's doing --- updater/lib/dependabot/dependency_snapshot.rb | 6 +- .../updater/group_update_creation.rb | 2 +- .../dependabot/dependency_snapshot_spec.rb | 56 ++++++++++++++++++- 3 files changed, 59 insertions(+), 5 deletions(-) diff --git a/updater/lib/dependabot/dependency_snapshot.rb b/updater/lib/dependabot/dependency_snapshot.rb index b0bb7445d65..108a3fd4e88 100644 --- a/updater/lib/dependabot/dependency_snapshot.rb +++ b/updater/lib/dependabot/dependency_snapshot.rb @@ -121,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 @@ -143,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 diff --git a/updater/lib/dependabot/updater/group_update_creation.rb b/updater/lib/dependabot/updater/group_update_creation.rb index 5b7785cecbe..ba975dc5c0d 100644 --- a/updater/lib/dependabot/updater/group_update_creation.rb +++ b/updater/lib/dependabot/updater/group_update_creation.rb @@ -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) diff --git a/updater/spec/dependabot/dependency_snapshot_spec.rb b/updater/spec/dependabot/dependency_snapshot_spec.rb index 822ad645da1..876337f48e1 100644 --- a/updater/spec/dependabot/dependency_snapshot_spec.rb +++ b/updater/spec/dependabot/dependency_snapshot_spec.rb @@ -140,6 +140,59 @@ end end + describe "::handled_dependencies_all_directories" do + subject(:create_dependency_snapshot) do + described_class.create_from_job_definition( + job: job, + job_definition: job_definition + ) + end + + let(:job_definition) do + { + "base_commit_sha" => base_commit_sha, + "base64_dependency_files" => encode_dependency_files(dependency_files) + } + end + + it "handles dependencies" do + snapshot = create_dependency_snapshot + 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 + let(:directory) { nil } + let(:directories) { %w(/foo /bar) } + let(:dependency_files) do + [ + Dependabot::DependencyFile.new( + name: "Gemfile", + content: fixture("bundler/original/Gemfile"), + directory: "/foo" + ), + Dependabot::DependencyFile.new( + name: "Gemfile", + content: fixture("bundler/original/Gemfile"), + directory: "/bar" + ) + ] + end + + it "is agnostic of the current directory" do + snapshot = create_dependency_snapshot + snapshot.current_directory = "/foo" + snapshot.add_handled_dependencies("a") + snapshot.current_directory = "/bar" + snapshot.add_handled_dependencies("b") + + expect(snapshot.handled_dependencies_all_directories).to eq(Set.new(%w(a b))) + snapshot.current_directory = "/bar" + expect(snapshot.handled_dependencies_all_directories).to eq(Set.new(%w(a b))) + end + end + end + describe "::create_from_job_definition" do subject(:create_dependency_snapshot) do described_class.create_from_job_definition( @@ -194,7 +247,8 @@ expect(snapshot.ungrouped_dependencies.length).to be(2) - snapshot.add_handled_dependencies(group.dependencies.find { |d| d.name == "dummy-pkg-a" }.name) + 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!