From 9578df85b00bd68d958e084b1dd0f306e3ba1f79 Mon Sep 17 00:00:00 2001 From: Landon Grindheim Date: Fri, 14 Jun 2024 17:46:31 +0000 Subject: [PATCH 1/3] Consider `directory` when doing grouped updates When considering whether a pull request matches an existing pull request, we should consider whether the directories match. --- updater/lib/dependabot/dependency_change.rb | 16 +++ .../spec/dependabot/dependency_change_spec.rb | 114 ++++++++++++++++++ updater/spec/dependabot/service_spec.rb | 4 +- .../version_updates/group_update_refresh.yaml | 1 + 4 files changed, 133 insertions(+), 2 deletions(-) diff --git a/updater/lib/dependabot/dependency_change.rb b/updater/lib/dependabot/dependency_change.rb index 1030a8ea230..d7ffa785a6f 100644 --- a/updater/lib/dependabot/dependency_change.rb +++ b/updater/lib/dependabot/dependency_change.rb @@ -59,6 +59,7 @@ def initialize(job:, updated_dependencies:, updated_dependency_files:, dependenc @dependency_group = dependency_group @pr_message = T.let(nil, T.nilable(Dependabot::PullRequestCreator::Message)) + ensure_dependencies_have_directories end sig { returns(Dependabot::PullRequestCreator::Message) } @@ -180,10 +181,25 @@ def updated_dependencies_set { "dependency-name" => dep.name, "dependency-version" => dep.version, + "directory" => grouped_update? ? dep.directory : nil, "dependency-removed" => dep.removed? ? true : nil }.compact end ) end + + sig { returns(T::Array[Dependabot::Dependency]) } + def ensure_dependencies_have_directories + updated_dependencies.each do |dep| + dep.directory = directory + end + end + + sig { returns(String) } + def directory + return "" if updated_dependency_files.empty? + + updated_dependency_files.first.directory + end end end diff --git a/updater/spec/dependabot/dependency_change_spec.rb b/updater/spec/dependabot/dependency_change_spec.rb index 025a8beb079..349c8088bbe 100644 --- a/updater/spec/dependabot/dependency_change_spec.rb +++ b/updater/spec/dependabot/dependency_change_spec.rb @@ -267,4 +267,118 @@ end end end + + describe "#matches_existing_pr?" do + context "when no existing pull requests are found" do + let(:job) do + instance_double(Dependabot::Job, + dependencies: updated_dependencies.map(&:name), + existing_pull_requests: []) + end + let(:dependency_change) do + described_class.new( + job: job, + updated_dependencies: updated_dependencies, + updated_dependency_files: updated_dependency_files + ) + end + + it "returns false" do + expect(dependency_change.matches_existing_pr?).to be false + end + end + + context "when updating a pull request with the same dependencies" do + let(:job) do + instance_double(Dependabot::Job, + dependencies: updated_dependencies.map(&:name), + existing_pull_requests: existing_pull_requests) + end + let(:existing_pull_requests) do + [ + updated_dependencies.map do |dep| + { "dependency-name" => dep.name, + "dependency-version" => dep.version } + end + ] + end + let(:dependency_change) do + described_class.new( + job: job, + updated_dependencies: updated_dependencies, + updated_dependency_files: updated_dependency_files + ) + end + + it "returns true" do + expect(dependency_change.matches_existing_pr?).to be true + end + end + + context "when updating a grouped pull request with the same dependencies" do + let(:job) do + instance_double(Dependabot::Job, + dependencies: updated_dependencies.map(&:name), + existing_group_pull_requests: existing_group_pull_requests) + end + let(:existing_group_pull_requests) do + [ + { "dependency-group-name" => "foo", + "dependencies" => [ + updated_dependencies.map do |dep| + { "dependency-name" => dep.name.to_s, + "dependency-version" => dep.version.to_s, + "directory" => dep.directory.to_s } + end + ] } + ] + end + + let(:dependency_change) do + described_class.new( + job: job, + updated_dependencies: updated_dependencies, + updated_dependency_files: updated_dependency_files, + dependency_group: Dependabot::DependencyGroup.new(name: "foo", rules: { patterns: ["*"] }) + ) + end + + it "returns true" do + expect(dependency_change.matches_existing_pr?).to be false + end + end + + context "when updating a grouped pull request with the same dependencies, but in different directory" do + let(:job) do + instance_double(Dependabot::Job, + dependencies: updated_dependencies.map(&:name), + existing_group_pull_requests: existing_group_pull_requests) + end + let(:existing_group_pull_requests) do + [ + { "dependency-group-name" => "foo", + "dependencies" => [ + updated_dependencies.map do |dep| + { "dependency-name" => dep.name.to_s, + "dependency-version" => dep.version.to_s, + "directory" => "/foo" } + end + ] } + ] + end + + let(:dependency_change) do + described_class.new( + job: job, + updated_dependencies: updated_dependencies, + updated_dependency_files: updated_dependency_files, + dependency_group: Dependabot::DependencyGroup.new(name: "foo", rules: { patterns: ["*"] }) + ) + end + + it "returns false" do + expect(dependency_change.matches_existing_pr?).to be false + end + end + end end diff --git a/updater/spec/dependabot/service_spec.rb b/updater/spec/dependabot/service_spec.rb index 86393869092..2352dd82123 100644 --- a/updater/spec/dependabot/service_spec.rb +++ b/updater/spec/dependabot/service_spec.rb @@ -82,7 +82,7 @@ let(:dependency_files) do [ - { name: "Gemfile", content: "some gems" } + Dependabot::DependencyFile.new(name: "Gemfile", content: "some gems") ] end @@ -138,7 +138,7 @@ let(:dependency_files) do [ - { name: "Gemfile", content: "some gems" } + Dependabot::DependencyFile.new(name: "Gemfile", content: "some gems") ] end diff --git a/updater/spec/fixtures/job_definitions/bundler/version_updates/group_update_refresh.yaml b/updater/spec/fixtures/job_definitions/bundler/version_updates/group_update_refresh.yaml index 3d3e5742321..d33aac07379 100644 --- a/updater/spec/fixtures/job_definitions/bundler/version_updates/group_update_refresh.yaml +++ b/updater/spec/fixtures/job_definitions/bundler/version_updates/group_update_refresh.yaml @@ -15,6 +15,7 @@ job: dependencies: - dependency-name: dummy-pkg-b dependency-version: 1.2.0 + directory: "/" updating-a-pull-request: true lockfile-only: false update-subdependencies: false From 992f41c3d975fef691072222761a0c632754f756 Mon Sep 17 00:00:00 2001 From: Landon Grindheim Date: Fri, 14 Jun 2024 17:59:57 +0000 Subject: [PATCH 2/3] Feature flag directory comparison --- updater/lib/dependabot/dependency_change.rb | 6 +++++- updater/spec/dependabot/dependency_change_spec.rb | 8 ++++++++ .../operations/refresh_group_update_pull_request_spec.rb | 5 +++++ 3 files changed, 18 insertions(+), 1 deletion(-) diff --git a/updater/lib/dependabot/dependency_change.rb b/updater/lib/dependabot/dependency_change.rb index d7ffa785a6f..43736fecff8 100644 --- a/updater/lib/dependabot/dependency_change.rb +++ b/updater/lib/dependabot/dependency_change.rb @@ -181,7 +181,7 @@ def updated_dependencies_set { "dependency-name" => dep.name, "dependency-version" => dep.version, - "directory" => grouped_update? ? dep.directory : nil, + "directory" => should_consider_directory? ? dep.directory : nil, "dependency-removed" => dep.removed? ? true : nil }.compact end @@ -201,5 +201,9 @@ def directory updated_dependency_files.first.directory end + + def should_consider_directory? + grouped_update? && Dependabot::Experiments.enabled?("dependency_has_directory") + end end end diff --git a/updater/spec/dependabot/dependency_change_spec.rb b/updater/spec/dependabot/dependency_change_spec.rb index 349c8088bbe..b467876a7b2 100644 --- a/updater/spec/dependabot/dependency_change_spec.rb +++ b/updater/spec/dependabot/dependency_change_spec.rb @@ -269,6 +269,14 @@ end describe "#matches_existing_pr?" do + before do + Dependabot::Experiments.register("dependency_has_directory", true) + end + + after do + Dependabot::Experiments.reset! + end + context "when no existing pull requests are found" do let(:job) do instance_double(Dependabot::Job, diff --git a/updater/spec/dependabot/updater/operations/refresh_group_update_pull_request_spec.rb b/updater/spec/dependabot/updater/operations/refresh_group_update_pull_request_spec.rb index 3825770be6d..ceb22fe739b 100644 --- a/updater/spec/dependabot/updater/operations/refresh_group_update_pull_request_spec.rb +++ b/updater/spec/dependabot/updater/operations/refresh_group_update_pull_request_spec.rb @@ -70,6 +70,11 @@ before do stub_rubygems_calls + Dependabot::Experiments.register("dependency_has_directory", true) + end + + after do + Dependabot::Experiments.reset! end it "updates the existing pull request without errors" do From 224573fbd7fd86bde96dfc0456a5eb23dadbd074 Mon Sep 17 00:00:00 2001 From: Landon Grindheim Date: Fri, 14 Jun 2024 18:16:30 +0000 Subject: [PATCH 3/3] Add missing types --- updater/lib/dependabot/dependency_change.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/updater/lib/dependabot/dependency_change.rb b/updater/lib/dependabot/dependency_change.rb index 43736fecff8..361b69f5a5a 100644 --- a/updater/lib/dependabot/dependency_change.rb +++ b/updater/lib/dependabot/dependency_change.rb @@ -199,9 +199,10 @@ def ensure_dependencies_have_directories def directory return "" if updated_dependency_files.empty? - updated_dependency_files.first.directory + T.must(updated_dependency_files.first).directory end + sig { returns(T::Boolean) } def should_consider_directory? grouped_update? && Dependabot::Experiments.enabled?("dependency_has_directory") end