From ff5444c226a5bff8af7e1e9ec783dde8154e40e9 Mon Sep 17 00:00:00 2001 From: Jake Coffman Date: Fri, 19 Jul 2024 14:23:48 -0500 Subject: [PATCH] removing more of dependency_has_directory feature flag (#10252) --- updater/lib/dependabot/dependency_snapshot.rb | 25 +++---------- .../updater/group_update_creation.rb | 29 +++------------ .../operations/group_update_all_versions.rb | 36 +++++-------------- .../dependabot/dependency_snapshot_spec.rb | 25 ++++++------- 4 files changed, 27 insertions(+), 88 deletions(-) diff --git a/updater/lib/dependabot/dependency_snapshot.rb b/updater/lib/dependabot/dependency_snapshot.rb index d347a2c20fd..108a3fd4e88 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 == "" @@ -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 @@ -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 @@ -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..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) @@ -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..06a365bf78e 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 @@ -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) diff --git a/updater/spec/dependabot/dependency_snapshot_spec.rb b/updater/spec/dependabot/dependency_snapshot_spec.rb index c7a42f3717e..876337f48e1 100644 --- a/updater/spec/dependabot/dependency_snapshot_spec.rb +++ b/updater/spec/dependabot/dependency_snapshot_spec.rb @@ -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, @@ -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 @@ -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 @@ -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!