Merge pull request #7432 from dependabot/brrygrdn/scheduled-updates-d…

…o-not-rebase

[Updater] When a Dependency Group is refreshed as part of a scheduled GroupUpdateAllVersions, it defers rebasing

updater/lib/dependabot/updater/operations/group_update_all_versions.rb

@@ -110,6 +110,8 @@ def run_update_for(group)
           end
         end
 
+        # If a PR exists for a group, we defer to this logic to determine if the existing changes
+        # still apply or if we need to replace the existing PR.
         def run_refresh_for(group, pull_request)
           Dependabot::Updater::Operations::RefreshGroupUpdatePullRequest.new(
             service: service,
@@ -122,6 +124,9 @@ def run_refresh_for(group, pull_request)
             operation.previous_dependency_names = pull_request["dependencies"].map do |dependency|
               dependency["dependency-name"]
             end
+            # Unlike a standalone refresh job, we shouldn't rebase the PR - we only care about
+            # replacing it if the target dependencies or versions have changed.
+            operation.defer_rebasing_existing_prs = true
           end.perform
         end
 

updater/lib/dependabot/updater/operations/refresh_group_update_pull_request.rb

@@ -41,7 +41,10 @@ def self.tag_name
         # We permit these attributes to be set to override obtaining these values from
         # the job definition, which enables GroupUpdateAllVersions to defer to this
         # class with information from an existing pull request overriding the job.
-        attr_writer :job_group, :job_group_name, :previous_dependency_names
+        attr_writer :defer_rebasing_existing_prs,
+                    :job_group,
+                    :job_group_name,
+                    :previous_dependency_names
 
         def initialize(service:, job:, dependency_snapshot:, error_handler:)
           @service = service
@@ -93,6 +96,10 @@ def previous_dependency_names
           @previous_dependency_names || job.dependencies
         end
 
+        def defer_rebasing_existing_prs
+          @defer_rebasing_existing_prs || false
+        end
+
         def upsert_pull_request_with_error_handling(dependency_change)
           if dependency_change.updated_dependencies.any?
             upsert_pull_request(dependency_change)
@@ -116,19 +123,11 @@ def upsert_pull_request_with_error_handling(dependency_change)
         # - Supersede the existing PR if the dependencies are the same but the target verisons have changed
         def upsert_pull_request(dependency_change)
           if should_replace_existing_pr?(dependency_change)
-            Dependabot.logger.info("Dependencies have changed, closing existing Pull Request")
-            close_pull_request(reason: :dependencies_changed)
-            Dependabot.logger.info("Creating a new pull request for '#{job_group.name}'")
-            service.create_pull_request(dependency_change, dependency_snapshot.base_commit_sha)
+            replace_existing_pr(dependency_change)
           elsif matches_existing_pr?(dependency_change)
-            Dependabot.logger.info("Updating pull request for '#{job_group.name}'")
-            service.update_pull_request(dependency_change, dependency_snapshot.base_commit_sha)
+            rebase_existing_pr(dependency_change)
           else
-            # If the changes do not match an existing PR, then we should open a new pull request and leave it to
-            # the backend to close the existing pull request with a comment that it has been superseded.
-            Dependabot.logger.info("Target versions have changed, existing Pull Request should be superseded")
-            Dependabot.logger.info("Creating a new pull request for '#{job_group.name}'")
-            service.create_pull_request(dependency_change, dependency_snapshot.base_commit_sha)
+            supersede_existing_pr(dependency_change)
           end
         end
 
@@ -137,6 +136,13 @@ def should_replace_existing_pr?(dependency_change)
             previous_dependency_names.map(&:downcase)
         end
 
+        def replace_existing_pr(dependency_change)
+          Dependabot.logger.info("Dependencies have changed, closing existing Pull Request")
+          close_pull_request(reason: :dependencies_changed)
+          Dependabot.logger.info("Creating a new pull request for '#{job_group.name}'")
+          service.create_pull_request(dependency_change, dependency_snapshot.base_commit_sha)
+        end
+
         def matches_existing_pr?(dependency_change)
           updated_dependencies_set = Set.new(
             dependency_change.updated_dependencies.map do |dep|
@@ -154,6 +160,24 @@ def matches_existing_pr?(dependency_change)
           end
         end
 
+        def rebase_existing_pr(dependency_change)
+          if defer_rebasing_existing_prs
+            Dependabot.logger.info("No new changes required for '#{job_group.name}'")
+            return
+          end
+
+          Dependabot.logger.info("Updating pull request for '#{job_group.name}'")
+          service.update_pull_request(dependency_change, dependency_snapshot.base_commit_sha)
+        end
+
+        def supersede_existing_pr(dependency_change)
+          # If the changes do not match an existing PR, then we should open a new pull request and leave it to
+          # the backend to close the existing pull request with a comment that it has been superseded.
+          Dependabot.logger.info("Target versions have changed, existing Pull Request should be superseded")
+          Dependabot.logger.info("Creating a new pull request for '#{job_group.name}'")
+          service.create_pull_request(dependency_change, dependency_snapshot.base_commit_sha)
+        end
+
         def close_pull_request(reason:)
           reason_string = reason.to_s.tr("_", " ")
           Dependabot.logger.info(

updater/spec/dependabot/updater/operations/group_update_all_versions_spec.rb

@@ -176,7 +176,7 @@
       stub_rubygems_calls
     end
 
-    it "delegates the group to RefreshGroupUpdatePullRequest which updates the pull request" do
+    it "delegates the group to RefreshGroupUpdatePullRequest which defers rebasing the PR" do
       expect(mock_error_handler).not_to receive(:handle_dependabot_error)
 
       allow(Dependabot.logger).to receive(:info)
@@ -186,20 +186,13 @@
 
       expect(Dependabot::Updater::Operations::RefreshGroupUpdatePullRequest).to receive(:new).and_call_original
 
-      expect(mock_service).to receive(:update_pull_request) do |dependency_change|
-        expect(dependency_change.dependency_group.name).to eql("group-b")
-
-        # We updated the right depednencies
-        expect(dependency_change.updated_dependencies.length).to eql(1)
-        expect(dependency_change.updated_dependencies.map(&:name)).to eql(%w(dummy-pkg-b))
-
-        # We updated the right files correctly.
-        expect(dependency_change.updated_dependency_files_hash.length).to eql(2)
-        expect(dependency_change.updated_dependency_files_hash).to eql(updated_bundler_files_hash)
-      end
-
+      expect(mock_service).not_to receive(:update_pull_request)
       expect(mock_service).not_to receive(:create_pull_request)
 
+      expect(Dependabot.logger).to receive(:info).with(
+        "No new changes required for 'group-b'"
+      )
+
       group_update_all.perform
     end
   end