Merge pull request dependabot#7354 from dependabot/brrygrdn/extract-g…

…roup-creation

[Updater] Extract creation of new group Pull Requests into a discrete class

updater/lib/dependabot/dependency_snapshot.rb

@@ -53,14 +53,10 @@ def job_dependencies
       end
     end
 
-    def job_group_name
-      job.dependency_group_to_refresh
-    end
-
     # Returns just the group that is specifically requested to be updated by
     # the job definition
     def job_group
-      return nil unless job_group_name
+      return nil unless job.dependency_group_to_refresh
       return @job_group if defined?(@job_group)
 
       @job_group = groups.fetch(job.dependency_group_to_refresh.to_sym, nil)

updater/lib/dependabot/job.rb

@@ -91,8 +91,9 @@ def initialize(attributes) # rubocop:disable Metrics/AbcSize
       # TODO: Make this hash required
       #
       # We will need to do a pass updating the CLI and smoke tests before this is possible,
-      # so let's consider it optional for now.
-      @existing_group_pull_requests   = attributes.fetch(:existing_group_pull_requests, [])
+      # so let's consider it optional for now. If we get a nil value, let's force it to be
+      # an array.
+      @existing_group_pull_requests   = attributes.fetch(:existing_group_pull_requests, []) || []
       @experiments                    = attributes.fetch(:experiments, {})
       @ignore_conditions              = attributes.fetch(:ignore_conditions)
       @package_manager                = attributes.fetch(:package_manager)

updater/lib/dependabot/updater/operations/create_group_update_pull_request.rb

@@ -0,0 +1,71 @@
+# frozen_string_literal: true
+
+require "dependabot/updater/group_update_creation"
+
+# This class implements our strategy for creating a single Pull Request which
+# updates all outdated Dependencies within a specific project folder that match
+# a specificed Dependency Group.
+#
+# This will always post a new Pull Request to Dependabot API and does not check
+# to see if any exists for the group or any of the dependencies involved.
+#
+module Dependabot
+  class Updater
+    module Operations
+      class CreateGroupUpdatePullRequest
+        include GroupUpdateCreation
+
+        # We do not invoke this class directly for any jobs, so let's return false in the event this
+        # check is called.
+        def self.applies_to?(*)
+          false
+        end
+
+        def self.tag_name
+          :create_version_group_pr
+        end
+
+        # Since this class is not invoked generically based on the job definition, this class accepts a `group` argument
+        # which is expected to be a prepopulated DependencyGroup object.
+        def initialize(service:, job:, dependency_snapshot:, error_handler:, group:)
+          @service = service
+          @job = job
+          @dependency_snapshot = dependency_snapshot
+          @error_handler = error_handler
+          @group = group
+        end
+
+        def perform
+          Dependabot.logger.info("Starting update group for '#{group.name}'")
+
+          dependency_change = compile_all_dependency_changes_for(group)
+
+          if dependency_change.updated_dependencies.any?
+            Dependabot.logger.info("Creating a pull request for '#{group.name}'")
+            begin
+              service.create_pull_request(dependency_change, dependency_snapshot.base_commit_sha)
+            rescue StandardError => e
+              raise if ErrorHandler::RUN_HALTING_ERRORS.keys.any? { |err| e.is_a?(err) }
+
+              # FIXME: This will result in us reporting a the group name as a dependency name
+              #
+              # In future we should modify this method to accept both dependency and group
+              # so the downstream error handling can tag things appropriately.
+              error_handler.handle_dependabot_error(error: e, dependency: group)
+            end
+          else
+            Dependabot.logger.info("Nothing to update for Dependency Group: '#{group.name}'")
+          end
+        end
+
+        private
+
+        attr_reader :job,
+                    :service,
+                    :dependency_snapshot,
+                    :error_handler,
+                    :group
+      end
+    end
+  end
+end

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

@@ -1,24 +1,22 @@
 # frozen_string_literal: true
 
+require "dependabot/updater/operations/create_group_update_pull_request"
 require "dependabot/updater/operations/update_all_versions"
-require "dependabot/updater/group_update_creation"
 
-# This class implements our strategy for creating Pull Requests for Dependency
-# Groups defined for a given folder before handling any un-grouped Dependencies
-# via Dependabot::Updater::Operations::UpdateAllVersions.
+# This class is responsible for coordinating the creation and upkeep of Pull Requests for
+# a given folder's defined DependencyGroups.
 #
-# **Note:** This is currently an experimental feature which is not supported
-#           in the service or as an integration point.
+# - If there is no Pull Request already open for a DependencyGroup, it will be delegated
+#   to Dependabot::Updater::Operations::CreateGroupUpdatePullRequest.
+# - If there is an open Pull Request for a DependencyGroup, it will skip over that group
+#   as the service is responsible for refreshing it in a separate job.
+# - Any ungrouped Dependencies will be handled individually by delegation to
+#   Dependabot::Updater::Operations::UpdateAllVersions.
 #
-# Some limitations of the current implementation:
-# - It has no superseding logic for groups - every time this strategy runs for a
-#  repo it will create a new Pull Request regardless of any existing, open PR
 module Dependabot
   class Updater
     module Operations
       class GroupUpdateAllVersions
-        include GroupUpdateCreation
-
         def self.applies_to?(job:)
           return false if job.security_updates_only?
           return false if job.updating_a_pull_request?
@@ -28,7 +26,7 @@ def self.applies_to?(job:)
         end
 
         def self.tag_name
-          :grouped_updates_prototype
+          :group_update_all_versions
         end
 
         def initialize(service:, job:, dependency_snapshot:, error_handler:)
@@ -68,7 +66,7 @@ def perform
                     :dependency_snapshot,
                     :error_handler
 
-        def run_grouped_dependency_updates # rubocop:disable Metrics/AbcSize
+        def run_grouped_dependency_updates
           Dependabot.logger.info("Starting grouped update job for #{job.source.repo}")
           Dependabot.logger.info("Found #{dependency_snapshot.groups.count} group(s).")
 
@@ -81,33 +79,24 @@ def run_grouped_dependency_updates # rubocop:disable Metrics/AbcSize
               next
             end
 
-            Dependabot.logger.info("Starting update group for '#{group.name}'")
-
-            dependency_change = compile_all_dependency_changes_for(group)
-
-            if dependency_change.updated_dependencies.any?
-              Dependabot.logger.info("Creating a pull request for '#{group.name}'")
-              begin
-                service.create_pull_request(dependency_change, dependency_snapshot.base_commit_sha)
-              rescue StandardError => e
-                raise if ErrorHandler::RUN_HALTING_ERRORS.keys.any? { |err| e.is_a?(err) }
-
-                # FIXME: This will result in us reporting a the group name as a dependency name
-                #
-                # In future we should modify this method to accept both dependency and group
-                # so the downstream error handling can tag things appropriately.
-                error_handler.handle_dependabot_error(error: e, dependency: group)
-              end
-            else
-              Dependabot.logger.info("Nothing to update for Dependency Group: '#{group.name}'")
-            end
+            run_update_for(group)
           end
         end
 
         def pr_exists_for_dependency_group?(group)
           job.existing_group_pull_requests&.any? { |pr| pr["dependency-group-name"] == group.name }
         end
 
+        def run_update_for(group)
+          Dependabot::Updater::Operations::CreateGroupUpdatePullRequest.new(
+            service: service,
+            job: job,
+            dependency_snapshot: dependency_snapshot,
+            error_handler: error_handler,
+            group: group
+          ).perform
+        end
+
         def run_ungrouped_dependency_updates
           Dependabot::Updater::Operations::UpdateAllVersions.new(
             service: service,

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

@@ -51,7 +51,7 @@ def perform
           # were out of sync.
           unless dependency_snapshot.job_group
             Dependabot.logger.warn(
-              "The '#{dependency_snapshot.job_group_name || 'unknown'}' group has been removed from the update config."
+              "The '#{job.dependency_group_to_refresh || 'unknown'}' group has been removed from the update config."
             )
 
             service.capture_exception(

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

@@ -6,6 +6,7 @@
 
 require "dependabot/dependency_change"
 require "dependabot/dependency_snapshot"
+require "dependabot/environment"
 require "dependabot/service"
 require "dependabot/updater/error_handler"
 require "dependabot/updater/operations/group_update_all_versions"
@@ -162,6 +163,32 @@
     end
   end
 
+  context "when a pull request already exists for a group" do
+    let(:job_definition) do
+      job_definition_fixture("bundler/version_updates/group_update_all_with_existing_pr")
+    end
+
+    let(:dependency_files) do
+      original_bundler_files
+    end
+
+    before do
+      stub_rubygems_calls
+    end
+
+    it "does not create a new pull request for a group if one already exists" do
+      expect(mock_error_handler).not_to receive(:handle_dependabot_error)
+      expect(mock_service).not_to receive(:create_pull_request)
+
+      allow(Dependabot.logger).to receive(:info)
+      expect(Dependabot.logger).to receive(:info).with(
+        "Detected existing pull request for 'group-b'."
+      )
+
+      group_update_all.perform
+    end
+  end
+
   context "when nothing in the group needs to be updated" do
     let(:job_definition) do
       job_definition_fixture("bundler/version_updates/group_update_all")

updater/spec/dependabot/updater_spec.rb

@@ -2301,26 +2301,6 @@ def expect_update_checker_with_ignored_versions(versions, dependency_matcher: an
       expect(service).not_to receive(:create_pull_request)
       updater.run
     end
-
-    it "does not create a new pull request for a group if one already exists" do
-      job = build_job(
-        existing_group_pull_requests: [
-          {
-            "dependency-group-name" => "group-b",
-            "dependencies" => [
-              { "dependency-name" => "dummy-pkg-b", "dependency-version" => "1.2.0" }
-            ]
-          }
-        ],
-        dependency_groups: [{ "name" => "group-b", "rules" => { "patterns" => ["dummy-pkg-b"] } }],
-        experiments: { "grouped-updates-prototype" => true }
-      )
-      service = build_service
-      updater = build_updater(service: service, job: job)
-
-      expect(service).not_to receive(:create_pull_request)
-      updater.run
-    end
   end
 
   def build_updater(service: build_service, job: build_job, dependency_files: default_dependency_files,

updater/spec/fixtures/job_definitions/bundler/version_updates/group_update_all_with_existing_pr.yaml

@@ -0,0 +1,43 @@
+job:
+  package-manager: bundler
+  source:
+    provider: github
+    repo: dependabot/smoke-tests
+    directory: "/bundler"
+    branch:
+    api-endpoint: https://api.github.com/
+    hostname: github.com
+  dependencies:
+  existing-pull-requests: []
+  existing-group-pull-requests:
+    - dependency-group-name: "group-b"
+      dependencies:
+        - dependency-name: "dummy-pkg-b"
+          dependency-version: "1.2.0"
+  updating-a-pull-request: false
+  lockfile-only: false
+  update-subdependencies: false
+  ignore-conditions: []
+  requirements-update-strategy:
+  allowed-updates:
+  - dependency-type: direct
+    update-type: all
+  credentials-metadata:
+  - type: git_source
+    host: github.com
+  security-advisories: []
+  max-updater-run-time: 2700
+  vendor-dependencies: false
+  experiments:
+    grouped-updates-prototype: true
+  reject-external-code: false
+  commit-message-options:
+    prefix:
+    prefix-development:
+    include-scope:
+  security-updates-only: false
+  dependency-groups:
+  - name: group-b
+    rules:
+      patterns:
+        - "dummy-pkg-b"