Merge pull request #7216 from dependabot/brrygrdn/updater-does-not-in…

…ject-groups

[Updater] The Updater always expects job.dependency_groups to be defined

.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/**'

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
 

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
@@ -37,12 +38,37 @@ def initialize(service:, job:, dependency_snapshot:, error_handler:)
           @error_handler = error_handler
         end
 
-        # 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
+          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 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
+            )
+          end
+
+          run_ungrouped_dependency_updates if dependency_snapshot.ungrouped_dependencies.any?
+        end
+
+        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 +93,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

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
 

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

@@ -57,9 +57,9 @@
     Dependabot::DependencyGroupEngine.reset!
   end
 
-  context "when the snapshot has no groups configured" do
+  context "when only some dependencies match the defined group" do
     let(:job_definition) do
-      job_definition_fixture("bundler/version_updates/update_all_simple")
+      job_definition_fixture("bundler/version_updates/group_update_all_with_ungrouped")
     end
 
     let(:dependency_files) do
@@ -70,13 +70,60 @@
       stub_rubygems_calls
     end
 
-    it "injects a placeholder group to update everything in a single request without errors" do
+    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("all-dependencies")
+        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")
+    end
+
+    let(:dependency_files) do
+      original_bundler_files
+    end
+
+    before do
+      stub_rubygems_calls
+    end
+
+    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).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].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
     end
   end

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)).

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).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: [

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"