From 76ef4986962c410e39e2dc2e68f5587948d7b432 Mon Sep 17 00:00:00 2001
From: Barry Gordon <brrygrdn@github.com>
Date: Tue, 2 May 2023 17:36:23 +0100
Subject: [PATCH 1/5] Stop injecting the placeholder dependency group

---
 .../operations/group_update_all_versions.rb   | 52 ++++++++++++-------
 .../group_update_all_versions_spec.rb         | 20 +++++--
 .../dependabot/updater/operations_spec.rb     |  7 +++
 updater/spec/dependabot/updater_spec.rb       |  2 +-
 4 files changed, 55 insertions(+), 26 deletions(-)

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 e1927b33659..5eb972cf637 100644
--- a/updater/lib/dependabot/updater/operations/group_update_all_versions.rb
+++ b/updater/lib/dependabot/updater/operations/group_update_all_versions.rb
@@ -1,5 +1,6 @@
 # frozen_string_literal: true
 
+require "dependabot/updater/operations/update_all_versions"
 require "dependabot/updater/group_update_creation"
 
 # This class implements our strategy for creating Pull Requests for Dependency
@@ -23,7 +24,7 @@ def self.applies_to?(job:)
           return false if job.updating_a_pull_request?
           return false if job.dependencies&.any?
 
-          Dependabot::Experiments.enabled?(:grouped_updates_prototype)
+          job.dependency_groups&.any? && Dependabot::Experiments.enabled?(:grouped_updates_prototype)
         end
 
         def self.tag_name
@@ -39,10 +40,37 @@ def initialize(service:, job:, dependency_snapshot:, error_handler:)
 
         # rubocop:disable Metrics/AbcSize
         def perform
-          # FIXME: This preserves the default behavior of grouping all updates into a single PR
-          # but we should figure out if this is the default behavior we want.
-          register_all_dependencies_group unless job.dependency_groups&.any?
+          if dependency_snapshot.groups.any?
+            run_grouped_dependency_updates
+            run_ungrouped_dependency_updates if dependency_snapshot.ungrouped_dependencies.any?
+          else
+            # We shouldn't have selected this operation if no groups were defined
+            # due to the rules in `::applies_to?`, but if it happens it isn't
+            # enough reasons to fail the job.
+            Dependabot.logger.warn(
+              "No dependency groups defined!"
+            )
+
+            # We should warn our exception tracker...
+            service.capture_exception(
+              error: DependabotError.new("Attempted a grouped update with no groups defined."),
+              job: job
+            )
+
+            # ... and then just delegate everything to UpdateAllVersions
+            run_ungrouped_dependency_updates
+          end
+        end
+        # rubocop:enable Metrics/AbcSize
 
+        private
+
+        attr_reader :job,
+                    :service,
+                    :dependency_snapshot,
+                    :error_handler
+
+        def run_grouped_dependency_updates
           Dependabot.logger.info("Starting grouped update job for #{job.source.repo}")
 
           dependency_snapshot.groups.each do |_group_hash, group|
@@ -67,22 +95,6 @@ def perform
               Dependabot.logger.info("Nothing to update for Dependency Group: '#{group.name}'")
             end
           end
-
-          run_ungrouped_dependency_updates if dependency_snapshot.ungrouped_dependencies.any?
-        end
-        # rubocop:enable Metrics/AbcSize
-
-        private
-
-        attr_reader :job,
-                    :service,
-                    :dependency_snapshot,
-                    :error_handler
-
-        def register_all_dependencies_group
-          all_dependencies_group = { "name" => "all-dependencies", "rules" => { "patterns" => ["*"] } }
-          Dependabot::DependencyGroupEngine.register(all_dependencies_group["name"],
-                                                     all_dependencies_group["rules"]["patterns"])
         end
 
         def run_ungrouped_dependency_updates
diff --git a/updater/spec/dependabot/updater/operations/group_update_all_versions_spec.rb b/updater/spec/dependabot/updater/operations/group_update_all_versions_spec.rb
index 636180aec05..4b2984c12f4 100644
--- a/updater/spec/dependabot/updater/operations/group_update_all_versions_spec.rb
+++ b/updater/spec/dependabot/updater/operations/group_update_all_versions_spec.rb
@@ -70,12 +70,22 @@
       stub_rubygems_calls
     end
 
-    it "injects a placeholder group to update everything in a single request without errors" do
+    it "logs a warning, reports an error but defers everything to individual updates" do
       expect(mock_error_handler).not_to receive(:handle_dependabot_error)
-      expect(mock_service).to receive(:create_pull_request) do |dependency_change|
-        expect(dependency_change.dependency_group.name).to eql("all-dependencies")
-        expect(dependency_change.updated_dependency_files_hash).to eql(updated_bundler_files_hash)
-      end
+      expect(mock_service).not_to receive(:create_pull_request)
+
+      expect(Dependabot.logger).to receive(:warn).with("No dependency groups defined!")
+
+      expect(mock_service).to receive(:capture_exception).with(
+        error: an_instance_of(Dependabot::DependabotError),
+        job: job
+      )
+
+      expect(Dependabot::Updater::Operations::UpdateAllVersions).to receive(:new) do |args|
+        expect(args[:dependency_snapshot].dependencies.length).to eql(2)
+        expect(args[:dependency_snapshot].dependencies.first.name).to eql("dummy-pkg-a")
+        expect(args[:dependency_snapshot].dependencies.last.name).to eql("dummy-pkg-b")
+      end.and_return(instance_double(Dependabot::Updater::Operations::UpdateAllVersions, perform: nil))
 
       group_update_all.perform
     end
diff --git a/updater/spec/dependabot/updater/operations_spec.rb b/updater/spec/dependabot/updater/operations_spec.rb
index 418510eea77..4d33f7f9a1b 100644
--- a/updater/spec/dependabot/updater/operations_spec.rb
+++ b/updater/spec/dependabot/updater/operations_spec.rb
@@ -19,6 +19,7 @@
                             security_updates_only?: false,
                             updating_a_pull_request?: true,
                             dependencies: [],
+                            dependency_groups: [],
                             is_a?: true)
 
       expect(described_class.class_for(job: job)).to be_nil
@@ -29,6 +30,7 @@
                             security_updates_only?: false,
                             updating_a_pull_request?: false,
                             dependencies: [],
+                            dependency_groups: [],
                             is_a?: true)
 
       expect(described_class.class_for(job: job)).to be(Dependabot::Updater::Operations::UpdateAllVersions)
@@ -42,6 +44,7 @@
                               security_updates_only?: false,
                               updating_a_pull_request?: false,
                               dependencies: [],
+                              dependency_groups: [anything],
                               is_a?: true)
 
         expect(described_class.class_for(job: job)).to be(Dependabot::Updater::Operations::GroupUpdateAllVersions)
@@ -57,6 +60,7 @@
                               updating_a_pull_request?: true,
                               dependencies: [anything],
                               dependency_group_to_refresh: anything,
+                              dependency_groups: [anything],
                               is_a?: true)
 
         expect(described_class.class_for(job: job)).
@@ -72,6 +76,7 @@
                             updating_a_pull_request?: true,
                             dependencies: [anything],
                             dependency_group_to_refresh: nil,
+                            dependency_groups: [anything],
                             is_a?: true)
 
       expect(described_class.class_for(job: job)).
@@ -83,6 +88,7 @@
                             security_updates_only?: true,
                             updating_a_pull_request?: false,
                             dependencies: [anything],
+                            dependency_groups: [anything],
                             is_a?: true)
 
       expect(described_class.class_for(job: job)).
@@ -94,6 +100,7 @@
                             security_updates_only?: true,
                             updating_a_pull_request?: true,
                             dependencies: [anything],
+                            dependency_groups: [anything],
                             is_a?: true)
 
       expect(described_class.class_for(job: job)).
diff --git a/updater/spec/dependabot/updater_spec.rb b/updater/spec/dependabot/updater_spec.rb
index 65ff9d912d1..63c2aa21835 100644
--- a/updater/spec/dependabot/updater_spec.rb
+++ b/updater/spec/dependabot/updater_spec.rb
@@ -2275,7 +2275,7 @@ def expect_update_checker_with_ignored_versions(versions, dependency_matcher: an
       )
 
       allow(service).to receive(:create_pull_request)
-      expect(snapshot).to receive(:groups).and_call_original
+      expect(snapshot).to receive(:groups).at_least(:once).and_call_original
       expect(snapshot).to receive(:ungrouped_dependencies).at_least(:once).and_call_original
       expect(service).to receive(:increment_metric).
         with("updater.started", { tags: { operation: :grouped_updates_prototype } })

From b6f0c93c710cb4899ef3037fdd2457e743f81e45 Mon Sep 17 00:00:00 2001
From: Barry Gordon <brrygrdn@github.com>
Date: Tue, 2 May 2023 18:01:58 +0100
Subject: [PATCH 2/5] Migrate grouped and ungrouped test from the legacy spec

---
 .../group_update_all_versions_spec.rb         | 37 ++++++++++++++++++
 updater/spec/dependabot/updater_spec.rb       | 21 ----------
 .../group_update_all_with_ungrouped.yaml      | 38 +++++++++++++++++++
 3 files changed, 75 insertions(+), 21 deletions(-)
 create mode 100644 updater/spec/fixtures/job_definitions/bundler/version_updates/group_update_all_with_ungrouped.yaml

diff --git a/updater/spec/dependabot/updater/operations/group_update_all_versions_spec.rb b/updater/spec/dependabot/updater/operations/group_update_all_versions_spec.rb
index 4b2984c12f4..38d4f5b1b90 100644
--- a/updater/spec/dependabot/updater/operations/group_update_all_versions_spec.rb
+++ b/updater/spec/dependabot/updater/operations/group_update_all_versions_spec.rb
@@ -57,6 +57,43 @@
     Dependabot::DependencyGroupEngine.reset!
   end
 
+  context "when only some dependencies match the defined group" do
+    let(:job_definition) do
+      job_definition_fixture("bundler/version_updates/group_update_all_with_ungrouped")
+    end
+
+    let(:dependency_files) do
+      original_bundler_files
+    end
+
+    before do
+      stub_rubygems_calls
+    end
+
+    it "performs a grouped and ungrouped dependency update when both are present" do
+      expect(mock_error_handler).not_to receive(:handle_dependabot_error)
+
+      expect(mock_service).to receive(:create_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(Dependabot::Updater::Operations::UpdateAllVersions).to receive(:new) do |args|
+        expect(args[:dependency_snapshot].ungrouped_dependencies.length).to eql(1)
+        expect(args[:dependency_snapshot].ungrouped_dependencies.first.name).to eql("dummy-pkg-a")
+      end.and_return(instance_double(Dependabot::Updater::Operations::UpdateAllVersions, perform: nil))
+
+      group_update_all.perform
+    end
+  end
+
   context "when the snapshot has no groups configured" do
     let(:job_definition) do
       job_definition_fixture("bundler/version_updates/update_all_simple")
diff --git a/updater/spec/dependabot/updater_spec.rb b/updater/spec/dependabot/updater_spec.rb
index 63c2aa21835..b0bd48bb0c8 100644
--- a/updater/spec/dependabot/updater_spec.rb
+++ b/updater/spec/dependabot/updater_spec.rb
@@ -2262,27 +2262,6 @@ def expect_update_checker_with_ignored_versions(versions, dependency_matcher: an
       updater.run
     end
 
-    it "performs a grouped and ungrouped dependency update when both are present" do
-      job = build_job(experiments: { "grouped-updates-prototype" => true },
-                      dependency_groups: [{ "name" => "group-b", "rules" => { "patterns" => ["dummy-pkg-b"] } }])
-      stub_update_checker
-      service = build_service
-      snapshot = build_dependency_snapshot(job: job)
-      updater = build_updater(
-        service: service,
-        job: job,
-        dependency_snapshot: snapshot
-      )
-
-      allow(service).to receive(:create_pull_request)
-      expect(snapshot).to receive(:groups).at_least(:once).and_call_original
-      expect(snapshot).to receive(:ungrouped_dependencies).at_least(:once).and_call_original
-      expect(service).to receive(:increment_metric).
-        with("updater.started", { tags: { operation: :grouped_updates_prototype } })
-
-      updater.run
-    end
-
     it "does not include ignored dependencies in the group PR" do
       job = build_job(
         ignore_conditions: [
diff --git a/updater/spec/fixtures/job_definitions/bundler/version_updates/group_update_all_with_ungrouped.yaml b/updater/spec/fixtures/job_definitions/bundler/version_updates/group_update_all_with_ungrouped.yaml
new file mode 100644
index 00000000000..46c3b2d3557
--- /dev/null
+++ b/updater/spec/fixtures/job_definitions/bundler/version_updates/group_update_all_with_ungrouped.yaml
@@ -0,0 +1,38 @@
+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: []
+  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"

From 5fb6e688735c567cee647bfe6903cdd9e03ff91d Mon Sep 17 00:00:00 2001
From: Barry Gordon <brrygrdn@github.com>
Date: Tue, 2 May 2023 18:12:26 +0100
Subject: [PATCH 3/5] If no groups are defined, ungrouped_dependencies should
 not be empty

---
 updater/lib/dependabot/dependency_snapshot.rb          |  3 +++
 .../updater/operations/group_update_all_versions.rb    | 10 +++++-----
 .../updater/operations/update_all_versions.rb          | 10 ++--------
 .../operations/group_update_all_versions_spec.rb       |  6 +++---
 4 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/updater/lib/dependabot/dependency_snapshot.rb b/updater/lib/dependabot/dependency_snapshot.rb
index 495d1f02183..86a9284c688 100644
--- a/updater/lib/dependabot/dependency_snapshot.rb
+++ b/updater/lib/dependabot/dependency_snapshot.rb
@@ -75,6 +75,9 @@ def groups
     end
 
     def ungrouped_dependencies
+      # If no groups are defined, all dependencies are ungrouped by default.
+      return allowed_dependencies unless groups.any?
+
       Dependabot::DependencyGroupEngine.ungrouped_dependencies(allowed_dependencies)
     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 5eb972cf637..5affd61fcab 100644
--- a/updater/lib/dependabot/updater/operations/group_update_all_versions.rb
+++ b/updater/lib/dependabot/updater/operations/group_update_all_versions.rb
@@ -42,7 +42,6 @@ def initialize(service:, job:, dependency_snapshot:, error_handler:)
         def perform
           if dependency_snapshot.groups.any?
             run_grouped_dependency_updates
-            run_ungrouped_dependency_updates if dependency_snapshot.ungrouped_dependencies.any?
           else
             # We shouldn't have selected this operation if no groups were defined
             # due to the rules in `::applies_to?`, but if it happens it isn't
@@ -51,15 +50,16 @@ def perform
               "No dependency groups defined!"
             )
 
-            # We should warn our exception tracker...
+            # We should warn our exception tracker in case this represents an
+            # unexpected problem hydrating groups we have swallowed and then
+            # delegate everything to run_ungrouped_dependency_updates.
             service.capture_exception(
               error: DependabotError.new("Attempted a grouped update with no groups defined."),
               job: job
             )
-
-            # ... and then just delegate everything to UpdateAllVersions
-            run_ungrouped_dependency_updates
           end
+
+          run_ungrouped_dependency_updates if dependency_snapshot.ungrouped_dependencies.any?
         end
         # rubocop:enable Metrics/AbcSize
 
diff --git a/updater/lib/dependabot/updater/operations/update_all_versions.rb b/updater/lib/dependabot/updater/operations/update_all_versions.rb
index fbb91b17bc8..e41f66f22f3 100644
--- a/updater/lib/dependabot/updater/operations/update_all_versions.rb
+++ b/updater/lib/dependabot/updater/operations/update_all_versions.rb
@@ -48,16 +48,10 @@ def dependencies
             return []
           end
 
-          dependencies = if dependency_snapshot.ungrouped_dependencies.any?
-                           dependency_snapshot.ungrouped_dependencies
-                         else
-                           dependency_snapshot.allowed_dependencies
-                         end
-
           if Environment.deterministic_updates?
-            dependencies
+            dependency_snapshot.ungrouped_dependencies
           else
-            dependencies.shuffle
+            dependency_snapshot.ungrouped_dependencies.shuffle
           end
         end
 
diff --git a/updater/spec/dependabot/updater/operations/group_update_all_versions_spec.rb b/updater/spec/dependabot/updater/operations/group_update_all_versions_spec.rb
index 38d4f5b1b90..c336ea85c35 100644
--- a/updater/spec/dependabot/updater/operations/group_update_all_versions_spec.rb
+++ b/updater/spec/dependabot/updater/operations/group_update_all_versions_spec.rb
@@ -119,9 +119,9 @@
       )
 
       expect(Dependabot::Updater::Operations::UpdateAllVersions).to receive(:new) do |args|
-        expect(args[:dependency_snapshot].dependencies.length).to eql(2)
-        expect(args[:dependency_snapshot].dependencies.first.name).to eql("dummy-pkg-a")
-        expect(args[:dependency_snapshot].dependencies.last.name).to eql("dummy-pkg-b")
+        expect(args[:dependency_snapshot].ungrouped_dependencies.length).to eql(2)
+        expect(args[:dependency_snapshot].ungrouped_dependencies.first.name).to eql("dummy-pkg-a")
+        expect(args[:dependency_snapshot].ungrouped_dependencies.last.name).to eql("dummy-pkg-b")
       end.and_return(instance_double(Dependabot::Updater::Operations::UpdateAllVersions, perform: nil))
 
       group_update_all.perform

From b22d6da1016eda615a6c39156ee25b0869769d10 Mon Sep 17 00:00:00 2001
From: Barry Gordon <brrygrdn@github.com>
Date: Tue, 2 May 2023 18:21:26 +0100
Subject: [PATCH 4/5] Remove redundant warning

---
 .../dependabot/updater/operations/group_update_all_versions.rb  | 2 --
 1 file changed, 2 deletions(-)

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 5affd61fcab..e07264059d1 100644
--- a/updater/lib/dependabot/updater/operations/group_update_all_versions.rb
+++ b/updater/lib/dependabot/updater/operations/group_update_all_versions.rb
@@ -38,7 +38,6 @@ def initialize(service:, job:, dependency_snapshot:, error_handler:)
           @error_handler = error_handler
         end
 
-        # rubocop:disable Metrics/AbcSize
         def perform
           if dependency_snapshot.groups.any?
             run_grouped_dependency_updates
@@ -61,7 +60,6 @@ def perform
 
           run_ungrouped_dependency_updates if dependency_snapshot.ungrouped_dependencies.any?
         end
-        # rubocop:enable Metrics/AbcSize
 
         private
 

From 057d1eea1c3e881c3219b0719967f88a2810dacc Mon Sep 17 00:00:00 2001
From: Barry Gordon <brrygrdn@github.com>
Date: Tue, 2 May 2023 18:30:50 +0100
Subject: [PATCH 5/5] Switch smoke test over

---
 .github/workflows/smoke.yml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/.github/workflows/smoke.yml b/.github/workflows/smoke.yml
index 8659b6af24c..6d054fff503 100644
--- a/.github/workflows/smoke.yml
+++ b/.github/workflows/smoke.yml
@@ -19,7 +19,7 @@ jobs:
       matrix:
         suite:
           - { path: bundler, name: bundler, ecosystem: bundler }
-          - { path: bundler, name: bundler-grouped, ecosystem: bundler }
+          - { path: bundler, name: bundler-group-rules, ecosystem: bundler }
           - { path: cargo, name: cargo, ecosystem: cargo }
           - { path: composer, name: composer, ecosystem: composer }
           - { path: docker, name: docker, ecosystem: docker }
@@ -64,7 +64,7 @@ jobs:
             - 'common/**'
             - 'updater/**'
             - 'bundler/**'
-          bundler-grouped:
+          bundler-group-rules:
             - .github/workflows/smoke.yml
             - Dockerfile.updater-core
             - 'common/**'