diff --git a/common/lib/dependabot/dependency_group.rb b/common/lib/dependabot/dependency_group.rb index abf5db47c72..ff753eb5672 100644 --- a/common/lib/dependabot/dependency_group.rb +++ b/common/lib/dependabot/dependency_group.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require "wildcard_matcher" +require "yaml" module Dependabot class DependencyGroup @@ -13,7 +14,8 @@ def initialize(name:, rules:) end def contains?(dependency) - @dependencies.include?(dependency) if @dependencies.any? + return true if @dependencies.include?(dependency) + positive_match = rules["patterns"].any? { |rule| WildcardMatcher.match?(rule, dependency.name) } negative_match = rules["exclude-patterns"]&.any? { |rule| WildcardMatcher.match?(rule, dependency.name) } @@ -23,5 +25,12 @@ def contains?(dependency) def to_h { "name" => name } end + + # Provides a debug utility to view the group as it appears in the config file. + def to_config_yaml + { + "groups" => { name => rules } + }.to_yaml.delete_prefix("---\n") + end end end diff --git a/common/spec/dependabot/dependency_group_spec.rb b/common/spec/dependabot/dependency_group_spec.rb index fcc449ad954..a4c7139a134 100644 --- a/common/spec/dependabot/dependency_group_spec.rb +++ b/common/spec/dependabot/dependency_group_spec.rb @@ -27,6 +27,22 @@ end let(:test_dependency2) do + Dependabot::Dependency.new( + name: "test-dependency-2", + package_manager: "bundler", + version: "1.1.0", + requirements: [ + { + file: "Gemfile", + requirement: "~> 1.1.0", + groups: [], + source: nil + } + ] + ) + end + + let(:another_test_dependency) do Dependabot::Dependency.new( name: "another-test-dependency", package_manager: "bundler", @@ -74,32 +90,72 @@ end describe "#contains?" do - context "before dependencies are assigned to the group" do - it "returns true if the dependency matches a rule" do - expect(dependency_group.dependencies).to eq([]) - expect(dependency_group.contains?(test_dependency1)).to be_truthy + context "when the rules include patterns" do + let(:rules) do + { + "patterns" => ["test-*", "nothing-matches-this"], + "exclude-patterns" => ["*-2"] + } end - it "returns false if the dependency does not match a rule" do - expect(dependency_group.dependencies).to eq([]) - expect(dependency_group.contains?(test_dependency2)).to be_falsey - end - end + context "before dependencies are assigned to the group" do + it "returns true if the dependency matches a pattern" do + expect(dependency_group.dependencies).to eq([]) + expect(dependency_group.contains?(test_dependency1)).to be_truthy + end - context "after dependencies are assigned to the group" do - before do - dependency_group.dependencies << test_dependency1 - end + it "returns false if the dependency is specifically excluded" do + expect(dependency_group.dependencies).to eq([]) + expect(dependency_group.contains?(test_dependency2)).to be_falsey + end - it "returns true if the dependency is in the dependency list" do - expect(dependency_group.dependencies).to include(test_dependency1) - expect(dependency_group.contains?(test_dependency1)).to be_truthy + it "returns false if the dependency does not match any patterns" do + expect(dependency_group.dependencies).to eq([]) + expect(dependency_group.contains?(another_test_dependency)).to be_falsey + end end - it "returns false if the dependency is not in the dependency list and does not match a rule" do - expect(dependency_group.dependencies).to include(test_dependency1) - expect(dependency_group.contains?(test_dependency2)).to be_falsey + context "after dependencies are assigned to the group" do + before do + dependency_group.dependencies << test_dependency1 + end + + it "returns true if the dependency is in the dependency list" do + expect(dependency_group.dependencies).to include(test_dependency1) + expect(dependency_group.contains?(test_dependency1)).to be_truthy + end + + it "returns false if the dependency is specifically excluded" do + expect(dependency_group.dependencies).to include(test_dependency1) + expect(dependency_group.contains?(test_dependency2)).to be_falsey + end + + it "returns false if the dependency is not in the dependency list and does not match a pattern" do + expect(dependency_group.dependencies).to include(test_dependency1) + expect(dependency_group.contains?(another_test_dependency)).to be_falsey + end end end end + + describe "#to_config_yaml" do + let(:rules) do + { + "patterns" => ["test-*", "nothing-matches-this"], + "exclude-patterns" => ["*-2"] + } + end + + it "renders the group to match our configuration file" do + expect(dependency_group.to_config_yaml).to eql(<<~YAML) + groups: + test_group: + patterns: + - test-* + - nothing-matches-this + exclude-patterns: + - "*-2" + YAML + end + end end diff --git a/updater/lib/dependabot/dependency_group_engine.rb b/updater/lib/dependabot/dependency_group_engine.rb index cdd4f4db96c..232587c6b99 100644 --- a/updater/lib/dependabot/dependency_group_engine.rb +++ b/updater/lib/dependabot/dependency_group_engine.rb @@ -5,81 +5,80 @@ # This class implements our strategy for keeping track of and matching dependency # groups that are defined by users in their dependabot config file. # -# This is a static class tied to the lifecycle of a Job -# - Each UpdateJob registers its own DependencyGroupEngine which calculates -# the grouped and ungrouped dependencies for a DependencySnapshot -# - Groups are only calculated once after the Job has registered its dependencies -# - All allowed dependencies should be passed in to the calculate_dependency_groups! method +# We instantiate the DependencyGroupEngine after parsing dependencies, configuring +# any groups from the job's configuration before assigning the dependency list to +# the groups. +# +# We permit dependencies to be in more than one group and also track those which +# have zero matches so they may be updated individuall. # # **Note:** This is currently an experimental feature which is not supported # in the service or as an integration point. # module Dependabot - module DependencyGroupEngine - @groups_calculated = false - @registered_groups = [] - - @dependency_groups = {} - @ungrouped_dependencies = [] + class DependencyGroupEngine + class ConfigurationError < StandardError; end - def self.reset! - @groups_calculated = false - @registered_groups = [] - - @dependency_groups = {} - @ungrouped_dependencies = [] - end + def self.from_job_config(job:) + groups = job.dependency_groups.map do |group| + Dependabot::DependencyGroup.new(name: group["name"], rules: group["rules"]) + end - # Eventually the key for a dependency group should be a hash since names _can_ conflict within jobs - def self.register(name, rules) - @registered_groups.push Dependabot::DependencyGroup.new(name: name, rules: rules) + new(dependency_groups: groups) end - def self.groups_for(dependency) - return [] if dependency.nil? - return [] unless dependency.instance_of?(Dependabot::Dependency) + attr_reader :dependency_groups, :groups_calculated, :ungrouped_dependencies - @registered_groups.select do |group| - group.contains?(dependency) - end + def find_group(name:) + dependency_groups.find { |group| group.name == name } end - # { group_name => [DependencyGroup], ... } - def self.dependency_groups(dependencies) - return @dependency_groups if @groups_calculated - - @groups_calculated = calculate_dependency_groups!(dependencies) + def assign_to_groups!(dependencies:) + raise ConfigurationError, "dependency groups have already been configured!" if @groups_calculated - @dependency_groups - end + if dependency_groups.any? + dependencies.each do |dependency| + matched_groups = @dependency_groups.each_with_object([]) do |group, matches| + next unless group.contains?(dependency) - # Returns a list of dependencies that do not belong to any of the groups - def self.ungrouped_dependencies(dependencies) - return @ungrouped_dependencies if @groups_calculated + group.dependencies.push(dependency) + matches << group + end - @groups_calculated = calculate_dependency_groups!(dependencies) + # If we had no matches, collect the dependency as ungrouped + @ungrouped_dependencies << dependency if matched_groups.empty? + end + else + @ungrouped_dependencies = dependencies + end - @ungrouped_dependencies + validate_groups + @groups_calculated = true end - def self.calculate_dependency_groups!(dependencies) - # If we try to calculate dependency groups when there are no groups registered - # then all of the dependencies end up in the ungrouped list which can break - # an UpdateAllVersions#dependencies check - return false unless @registered_groups.any? + private - dependencies.each do |dependency| - groups = groups_for(dependency) + def initialize(dependency_groups:) + @dependency_groups = dependency_groups + @ungrouped_dependencies = [] + @groups_calculated = false + end - @ungrouped_dependencies << dependency if groups.empty? + def validate_groups + empty_groups = dependency_groups.select { |group| group.dependencies.empty? } + warn_misconfigured_groups(empty_groups) if empty_groups.any? + end - groups.each do |group| - group.dependencies.push(dependency) - @dependency_groups[group.name.to_sym] = group - end - end + def warn_misconfigured_groups(groups) + Dependabot.logger.warn <<~WARN + Please check your configuration as there are groups no dependencies match: + #{groups.map { |g| "- #{g.name}" }.join("\n")} - true + This can happen if: + - the group's 'pattern' rules are mispelled + - your configuration's 'allow' rules do not permit any of the dependencies that match the group + - the dependencies that match the group rules have been removed from your project + WARN end end end diff --git a/updater/lib/dependabot/dependency_snapshot.rb b/updater/lib/dependabot/dependency_snapshot.rb index 97b5707264a..fedcec6a9d7 100644 --- a/updater/lib/dependabot/dependency_snapshot.rb +++ b/updater/lib/dependabot/dependency_snapshot.rb @@ -59,22 +59,18 @@ def job_group 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) + @job_group = @dependency_group_engine.find_group(name: job.dependency_group_to_refresh) end - # A dependency snapshot will always have the same set of dependencies since it only depends - # on the Job and dependency groups, which are static for a given commit. def groups - # The DependencyGroupEngine registers dependencies when the Job is created - # and it will memoize the dependency groups - Dependabot::DependencyGroupEngine.dependency_groups(allowed_dependencies) + @dependency_group_engine.dependency_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) + @dependency_group_engine.ungrouped_dependencies end private @@ -85,6 +81,8 @@ def initialize(job:, base_commit_sha:, dependency_files:) @dependency_files = dependency_files @dependencies = parse_files! + @dependency_group_engine = DependencyGroupEngine.from_job_config(job: job) + @dependency_group_engine.assign_to_groups!(dependencies: allowed_dependencies) end attr_reader :job diff --git a/updater/lib/dependabot/job.rb b/updater/lib/dependabot/job.rb index e841024bec9..4e8e0df899f 100644 --- a/updater/lib/dependabot/job.rb +++ b/updater/lib/dependabot/job.rb @@ -81,7 +81,7 @@ def self.standardise_keys(hash) # NOTE: "attributes" are fetched and injected at run time from # dependabot-api using the UpdateJobPrivateSerializer - def initialize(attributes) # rubocop:disable Metrics/AbcSize + def initialize(attributes) @id = attributes.fetch(:id) @allowed_updates = attributes.fetch(:allowed_updates) @commit_message_options = attributes.fetch(:commit_message_options, {}) @@ -111,12 +111,16 @@ def initialize(attributes) # rubocop:disable Metrics/AbcSize @update_subdependencies = attributes.fetch(:update_subdependencies) @updating_a_pull_request = attributes.fetch(:updating_a_pull_request) @vendor_dependencies = attributes.fetch(:vendor_dependencies, false) - @dependency_groups = attributes.fetch(:dependency_groups, []) + # 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. If we get a nil value, let's force it to be + # an array. + @dependency_groups = attributes.fetch(:dependency_groups, []) || [] @dependency_group_to_refresh = attributes.fetch(:dependency_group_to_refresh, nil) @repo_private = attributes.fetch(:repo_private, nil) register_experiments - register_dependency_groups end def clone? @@ -254,14 +258,6 @@ def security_advisories_for(dependency) end end - def register_dependency_groups - return if dependency_groups.nil? - - dependency_groups.each do |group| - Dependabot::DependencyGroupEngine.register(group["name"], group["rules"]) - end - end - def ignore_conditions_for(dependency) update_config.ignored_versions_for( dependency, diff --git a/updater/lib/dependabot/updater/group_update_creation.rb b/updater/lib/dependabot/updater/group_update_creation.rb index 9a7b69186cb..1fc2cb769b7 100644 --- a/updater/lib/dependabot/updater/group_update_creation.rb +++ b/updater/lib/dependabot/updater/group_update_creation.rb @@ -239,6 +239,20 @@ def peer_dependency_should_update_instead?(dependency_name, dependency_files, up end end + def warn_group_is_empty(group) + Dependabot.logger.warn( + "Skipping update group for '#{group.name}' as it does not match any allowed dependencies." + ) + + return unless Dependabot.logger.debug? + + Dependabot.logger.debug(<<~DEBUG.chomp) + The configuration for this group is: + + #{group.to_config_yaml} + DEBUG + end + def prepare_workspace return unless job.clone? && job.repo_contents_path diff --git a/updater/lib/dependabot/updater/operations/create_group_update_pull_request.rb b/updater/lib/dependabot/updater/operations/create_group_update_pull_request.rb index 12b440fd160..98e4b568480 100644 --- a/updater/lib/dependabot/updater/operations/create_group_update_pull_request.rb +++ b/updater/lib/dependabot/updater/operations/create_group_update_pull_request.rb @@ -36,6 +36,8 @@ def initialize(service:, job:, dependency_snapshot:, error_handler:, group:) end def perform + return warn_group_is_empty(group) if group.dependencies.empty? + Dependabot.logger.info("Starting update group for '#{group.name}'") dependency_change = compile_all_dependency_changes_for(group) 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 826651306b6..95ab63cc846 100644 --- a/updater/lib/dependabot/updater/operations/group_update_all_versions.rb +++ b/updater/lib/dependabot/updater/operations/group_update_all_versions.rb @@ -70,7 +70,7 @@ 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).") - dependency_snapshot.groups.each do |_group_hash, group| + dependency_snapshot.groups.each do |group| if pr_exists_for_dependency_group?(group) Dependabot.logger.info("Detected existing pull request for '#{group.name}'.") Dependabot.logger.info( diff --git a/updater/lib/dependabot/updater/operations/refresh_group_update_pull_request.rb b/updater/lib/dependabot/updater/operations/refresh_group_update_pull_request.rb index 43bad978624..c0616e77dfb 100644 --- a/updater/lib/dependabot/updater/operations/refresh_group_update_pull_request.rb +++ b/updater/lib/dependabot/updater/operations/refresh_group_update_pull_request.rb @@ -55,18 +55,29 @@ def perform ) service.capture_exception( - error: DependabotError.new("Attempted to update a missing group."), + error: DependabotError.new("Attempted to refresh a missing group."), job: job ) return end Dependabot.logger.info("Starting PR update job for #{job.source.repo}") - Dependabot.logger.info("Updating the '#{dependency_snapshot.job_group.name}' group") - dependency_change = compile_all_dependency_changes_for(dependency_snapshot.job_group) + if dependency_snapshot.job_group.dependencies.empty? + # If the group is empty that means any Dependencies that did match this group + # have been removed from the project or are no longer allowed by the config. + # + # Let's warn that the group is empty and then signal the PR should be closed + # so users are informed this group is no longer actionable by Dependabot. + warn_group_is_empty(dependency_snapshot.job_group) + close_pull_request(reason: :dependency_group_empty) + else + Dependabot.logger.info("Updating the '#{dependency_snapshot.job_group.name}' group") + + dependency_change = compile_all_dependency_changes_for(dependency_snapshot.job_group) - upsert_pull_request_with_error_handling(dependency_change) + upsert_pull_request_with_error_handling(dependency_change) + end end private diff --git a/updater/spec/dependabot/dependency_group_engine_spec.rb b/updater/spec/dependabot/dependency_group_engine_spec.rb index 089cde84f3d..05af8b299cd 100644 --- a/updater/spec/dependabot/dependency_group_engine_spec.rb +++ b/updater/spec/dependabot/dependency_group_engine_spec.rb @@ -7,223 +7,240 @@ require "dependabot/dependency_snapshot" require "dependabot/job" -# The DependencyGroupEngine is not accessed directly, but though a DependencySnapshot. -# So these tests use DependencySnapshot methods to check the DependencyGroupEngine works as expected RSpec.describe Dependabot::DependencyGroupEngine do include DependencyFileHelpers - let(:dependency_group_engine) { described_class } - - let(:job_json) { fixture("jobs/job_with_dependency_groups.json") } - - let(:dependency_files) do - [ - Dependabot::DependencyFile.new( - name: "Gemfile", - content: fixture("bundler_grouped/original/Gemfile"), - directory: "/" - ), - Dependabot::DependencyFile.new( - name: "Gemfile.lock", - content: fixture("bundler_grouped/original/Gemfile.lock"), - directory: "/" - ) - ] - end - - let(:base_commit_sha) do - "mock-sha" - end - - let(:job_definition) do - { - "base_commit_sha" => base_commit_sha, - "base64_dependency_files" => encode_dependency_files(dependency_files) - } - end - - let(:source) do - Dependabot::Source.new( - provider: "github", - repo: "dependabot-fixtures/dependabot-test-ruby-package", - directory: "/", - branch: nil, - api_endpoint: "https://api.github.com/", - hostname: "github.com" - ) - end + let(:dependency_group_engine) { described_class.from_job_config(job: job) } let(:job) do - Dependabot::Job.new_update_job( - job_id: anything, - job_definition: JSON.parse(job_json), - repo_contents_path: anything - ) - end - - def create_dependency_snapshot - Dependabot::DependencySnapshot.create_from_job_definition( - job: job, - job_definition: job_definition - ) - end - - describe "#register" do - after do - Dependabot::Experiments.reset! - Dependabot::DependencyGroupEngine.reset! - end - - it "registers the dependency groups" do - expect(dependency_group_engine.instance_variable_get(:@registered_groups)).to eq([]) - - # We have to call the original here for the DependencyGroupEngine to actually register the groups - expect(dependency_group_engine).to receive(:register).with("group-a", - { "exclude-patterns" => ["dummy-pkg-b"], - "patterns" => ["dummy-pkg-*"] }).and_call_original - expect(dependency_group_engine).to receive(:register).with("group-b", - { "patterns" => ["dummy-pkg-b"] }).and_call_original - - # Groups are registered by the job when a DependencySnapshot is created - create_dependency_snapshot - - expect(dependency_group_engine.instance_variable_get(:@registered_groups)).not_to eq([]) - end - end - - describe "#groups_for" do - after do - Dependabot::Experiments.reset! - Dependabot::DependencyGroupEngine.reset! - end - - it "returns the expected groups" do - snapshot = create_dependency_snapshot - - expect(dependency_group_engine.send(:groups_for, snapshot.dependencies[0]).count).to eq(1) - expect(dependency_group_engine.send(:groups_for, snapshot.dependencies[1]).count).to eq(1) - expect(dependency_group_engine.send(:groups_for, snapshot.dependencies[2]).count).to eq(0) - end - end - - describe "#dependency_groups" do - after do - Dependabot::Experiments.reset! - Dependabot::DependencyGroupEngine.reset! - end - - it "returns the dependency groups" do - snapshot = create_dependency_snapshot - allowed_dependencies = snapshot.allowed_dependencies - - expect(dependency_group_engine).to receive(:dependency_groups). - with(allowed_dependencies).at_least(:once).and_call_original - expect(dependency_group_engine).to receive(:calculate_dependency_groups!). - with(allowed_dependencies).once.and_call_original - - groups = snapshot.groups - - expect(groups.key?(:"group-a")).to be_truthy - expect(groups.key?(:"group-b")).to be_truthy - expect(groups[:"group-a"]).to be_a(Dependabot::DependencyGroup) - end - - it "does not call calculate_dependency_groups! again after groups are initially calculated" do - snapshot = create_dependency_snapshot - allowed_dependencies = snapshot.allowed_dependencies - - expect(dependency_group_engine.instance_variable_get(:@groups_calculated)).to be_falsey - expect(dependency_group_engine).to receive(:calculate_dependency_groups!). - with(allowed_dependencies).once.and_call_original - - snapshot.groups - snapshot.ungrouped_dependencies - - expect(dependency_group_engine.instance_variable_get(:@groups_calculated)).to be_truthy - end - end - - describe "#ungrouped_dependencies" do - after do - Dependabot::Experiments.reset! - Dependabot::DependencyGroupEngine.reset! - end - - it "returns the ungrouped dependencies" do - snapshot = create_dependency_snapshot - allowed_dependencies = snapshot.allowed_dependencies - - expect(dependency_group_engine).to receive(:calculate_dependency_groups!). - with(allowed_dependencies).once.and_call_original - expect(dependency_group_engine).to receive(:ungrouped_dependencies). - with(allowed_dependencies).at_least(:once).and_call_original - - ungrouped_dependencies = snapshot.ungrouped_dependencies - - expect(ungrouped_dependencies.first).to be_a(Dependabot::Dependency) - end - - it "does not call calculate_dependency_groups! again after ungrouped_dependencies are initially calculated" do - snapshot = create_dependency_snapshot - allowed_dependencies = snapshot.allowed_dependencies - - expect(dependency_group_engine.instance_variable_get(:@groups_calculated)).to be_falsey - expect(dependency_group_engine).to receive(:calculate_dependency_groups!). - with(allowed_dependencies).once.and_call_original - - snapshot.ungrouped_dependencies - snapshot.groups - - expect(dependency_group_engine.instance_variable_get(:@groups_calculated)).to be_truthy - end + instance_double(Dependabot::Job, dependency_groups: dependency_groups_config) end - describe "#reset!" do - after do - Dependabot::Experiments.reset! - Dependabot::DependencyGroupEngine.reset! + context "when a job has groups configured" do + let(:dependency_groups_config) do + [ + { + "name" => "group-a", + "rules" => { + "patterns" => ["dummy-pkg-*"], + "exclude-patterns" => ["dummy-pkg-b"] + } + }, + { + "name" => "group-b", + "rules" => { + "patterns" => %w(dummy-pkg-b dummy-pkg-c) + } + } + ] end - it "resets the dependency group engine" do - snapshot = create_dependency_snapshot - snapshot.groups - - expect(dependency_group_engine.instance_variable_get(:@groups_calculated)).to be_truthy - expect(dependency_group_engine.instance_variable_get(:@registered_groups)).not_to eq([]) - expect(dependency_group_engine.instance_variable_get(:@dependency_groups)).not_to eq({}) - expect(dependency_group_engine.instance_variable_get(:@ungrouped_dependencies)).not_to eq([]) - - dependency_group_engine.reset! - - expect(dependency_group_engine.instance_variable_get(:@groups_calculated)).to be_falsey - expect(dependency_group_engine.instance_variable_get(:@registered_groups)).to eq([]) - expect(dependency_group_engine.instance_variable_get(:@dependency_groups)).to eq({}) - expect(dependency_group_engine.instance_variable_get(:@ungrouped_dependencies)).to eq([]) + describe "::from_job_config" do + it "registers the dependency groups" do + expect(dependency_group_engine.dependency_groups.length).to eql(2) + expect(dependency_group_engine.dependency_groups.map(&:name)).to eql(%w(group-a group-b)) + expect(dependency_group_engine.dependency_groups.map(&:dependencies)).to all(be_empty) + end end - end - describe "#calculate_dependency_groups!" do - after do - Dependabot::Experiments.reset! - Dependabot::DependencyGroupEngine.reset! + describe "#find_group" do + it "retrieves a defined group by name" do + group_a = dependency_group_engine.find_group(name: "group-a") + expect(group_a.rules).to eql({ + "patterns" => ["dummy-pkg-*"], + "exclude-patterns" => ["dummy-pkg-b"] + }) + end + + it "returns nil if the group does not exist" do + expect(dependency_group_engine.find_group(name: "no-such-thing")).to be_nil + end end - it "runs once" do - snapshot = create_dependency_snapshot - allowed_dependencies = snapshot.allowed_dependencies - - expect(dependency_group_engine).to receive(:calculate_dependency_groups!). - with(allowed_dependencies).once.and_call_original - - snapshot.groups - snapshot.groups + describe "#assign_to_groups!" do + let(:dummy_pkg_a) do + Dependabot::Dependency.new( + name: "dummy-pkg-a", + package_manager: "bundler", + version: "1.1.0", + requirements: [ + { + file: "Gemfile", + requirement: "~> 1.1.0", + groups: ["default"], + source: nil + } + ] + ) + end + + let(:dummy_pkg_b) do + Dependabot::Dependency.new( + name: "dummy-pkg-b", + package_manager: "bundler", + version: "1.1.0", + requirements: [ + { + file: "Gemfile", + requirement: "~> 1.1.0", + groups: ["default"], + source: nil + } + ] + ) + end + + let(:dummy_pkg_c) do + Dependabot::Dependency.new( + name: "dummy-pkg-c", + package_manager: "bundler", + version: "1.1.0", + requirements: [ + { + file: "Gemfile", + requirement: "~> 1.1.0", + groups: ["default"], + source: nil + } + ] + ) + end + + let(:ungrouped_pkg) do + Dependabot::Dependency.new( + name: "ungrouped_pkg", + package_manager: "bundler", + version: "1.1.0", + requirements: [ + { + file: "Gemfile", + requirement: "~> 1.1.0", + groups: ["default"], + source: nil + } + ] + ) + end + + context "when all groups have at least one dependency that matches" do + let(:dependencies) { [dummy_pkg_a, dummy_pkg_b, dummy_pkg_c, ungrouped_pkg] } + + before do + dependency_group_engine.assign_to_groups!(dependencies: dependencies) + end + + it "adds dependencies to every group they match" do + group_a = dependency_group_engine.find_group(name: "group-a") + expect(group_a.dependencies).to eql([dummy_pkg_a, dummy_pkg_c]) + + group_b = dependency_group_engine.find_group(name: "group-b") + expect(group_b.dependencies).to eql([dummy_pkg_b, dummy_pkg_c]) + end + + it "keeps a list of any dependencies that do not match any groups" do + expect(dependency_group_engine.ungrouped_dependencies).to eql([ungrouped_pkg]) + end + + it "raises an exception if it is called a second time" do + expect { dependency_group_engine.assign_to_groups!(dependencies: dependencies) }. + to raise_error(described_class::ConfigurationError, "dependency groups have already been configured!") + end + end + + context "when one group has no matching dependencies" do + let(:dependencies) { [dummy_pkg_a] } + + it "warns that the group is misconfigured" do + expect(Dependabot.logger).to receive(:warn).with( + <<~WARN + Please check your configuration as there are groups no dependencies match: + - group-b + + This can happen if: + - the group's 'pattern' rules are mispelled + - your configuration's 'allow' rules do not permit any of the dependencies that match the group + - the dependencies that match the group rules have been removed from your project + WARN + ) + + dependency_group_engine.assign_to_groups!(dependencies: dependencies) + end + end + + context "when no groups have any matching dependencies" do + let(:dependencies) { [ungrouped_pkg] } + + it "warns that the groups are misconfigured" do + expect(Dependabot.logger).to receive(:warn).with( + <<~WARN + Please check your configuration as there are groups no dependencies match: + - group-a + - group-b + + This can happen if: + - the group's 'pattern' rules are mispelled + - your configuration's 'allow' rules do not permit any of the dependencies that match the group + - the dependencies that match the group rules have been removed from your project + WARN + ) + + dependency_group_engine.assign_to_groups!(dependencies: dependencies) + end + end end - it "returns true" do - snapshot = create_dependency_snapshot - allowed_dependencies = snapshot.allowed_dependencies - - expect(dependency_group_engine.calculate_dependency_groups!(allowed_dependencies)).to be_truthy + context "when a job has no groups configured" do + let(:dependency_groups_config) { [] } + + describe "::from_job_config" do + it "registers no dependency groups" do + expect(dependency_group_engine.dependency_groups).to be_empty + end + end + + describe "#assign_to_groups!" do + let(:dummy_pkg_a) do + Dependabot::Dependency.new( + name: "dummy-pkg-a", + package_manager: "bundler", + version: "1.1.0", + requirements: [ + { + file: "Gemfile", + requirement: "~> 1.1.0", + groups: ["default"], + source: nil + } + ] + ) + end + + let(:dummy_pkg_b) do + Dependabot::Dependency.new( + name: "dummy-pkg-b", + package_manager: "bundler", + version: "1.1.0", + requirements: [ + { + file: "Gemfile", + requirement: "~> 1.1.0", + groups: ["default"], + source: nil + } + ] + ) + end + + let(:dependencies) { [dummy_pkg_a, dummy_pkg_b] } + + before do + dependency_group_engine.assign_to_groups!(dependencies: dependencies) + end + + it "lists all dependencies as ungrouped" do + expect(dependency_group_engine.ungrouped_dependencies).to eql(dependencies) + end + end end end end diff --git a/updater/spec/dependabot/dependency_snapshot_spec.rb b/updater/spec/dependabot/dependency_snapshot_spec.rb index 39f011a936f..fb75fd66940 100644 --- a/updater/spec/dependabot/dependency_snapshot_spec.rb +++ b/updater/spec/dependabot/dependency_snapshot_spec.rb @@ -27,6 +27,8 @@ credentials: [], reject_external_code?: false, source: source, + dependency_groups: dependency_groups, + allowed_update?: true, experiments: { large_hadron_collider: true }) end @@ -45,6 +47,18 @@ ] end + let(:dependency_groups) do + [ + { + "name" => "group-a", + "rules" => { + "patterns" => ["dummy-pkg-*"], + "exclude-patterns" => ["dummy-pkg-b"] + } + } + ] + end + let(:base_commit_sha) do "mock-sha" end @@ -89,6 +103,21 @@ create_dependency_snapshot end + + it "correctly instantiates any configured dependency groups" do + snapshot = create_dependency_snapshot + + expect(snapshot.groups.length).to eql(1) + + group = snapshot.groups.last + + expect(group.name).to eql("group-a") + expect(group.dependencies.length).to eql(1) + expect(group.dependencies.first.name).to eql("dummy-pkg-a") + + expect(snapshot.ungrouped_dependencies.length).to eql(1) + expect(snapshot.ungrouped_dependencies.first.name).to eql("dummy-pkg-b") + end end context "when there is a parser error" do diff --git a/updater/spec/dependabot/job_spec.rb b/updater/spec/dependabot/job_spec.rb index acb692b9fa7..6757f6d3655 100644 --- a/updater/spec/dependabot/job_spec.rb +++ b/updater/spec/dependabot/job_spec.rb @@ -90,11 +90,6 @@ expect(ruby_credential["host"]).to eql("my.rubygems-host.org") expect(ruby_credential.keys).not_to include("token") end - - it "will register its dependency groups" do - expect_any_instance_of(described_class).to receive(:register_dependency_groups) - new_update_job - end end context "when lockfile_only is passed as true" do 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 61c2108b202..2e4d167fc5e 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 @@ -55,7 +55,6 @@ after do Dependabot::Experiments.reset! - Dependabot::DependencyGroupEngine.reset! end context "when only some dependencies match the defined group" do @@ -125,6 +124,61 @@ end end + context "when the snapshot has a group that does not match any dependencies" do + let(:job_definition) do + job_definition_fixture("bundler/version_updates/group_update_all_empty_group") + end + + let(:dependency_files) do + original_bundler_files + end + + before do + stub_rubygems_calls + end + + it "warns the group is empty" do + allow(Dependabot.logger).to receive(:warn) # permit any other warning calls that happen + expect(Dependabot.logger).to receive(:warn).with( + "Skipping update group for 'everything-everywhere-all-at-once' as it does not match any allowed dependencies." + ) + + # Expect us to handle the dependencies individually instead + expect(Dependabot::Updater::Operations::UpdateAllVersions).to receive(:new).and_return( + instance_double(Dependabot::Updater::Operations::UpdateAllVersions, perform: nil) + ) + + group_update_all.perform + end + + context "when debug is enabled" do + before do + allow(Dependabot.logger).to receive(:debug) + allow(Dependabot.logger).to receive(:debug?).and_return(true) + end + + it "renders the group configuration for us" do + expect(Dependabot.logger).to receive(:debug).with( + <<~DEBUG + The configuration for this group is: + + groups: + everything-everywhere-all-at-once: + patterns: + - "*bagel" + DEBUG + ) + + # Expect us to handle the dependencies individually instead + expect(Dependabot::Updater::Operations::UpdateAllVersions).to receive(:new).and_return( + instance_double(Dependabot::Updater::Operations::UpdateAllVersions, perform: nil) + ) + + group_update_all.perform + end + end + end + context "when there are two overlapping groups" do let(:job_definition) do job_definition_fixture("bundler/version_updates/group_update_all_overlapping_groups") 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 b75dd14e06b..9fe4101df84 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 @@ -54,7 +54,6 @@ after do Dependabot::Experiments.reset! - Dependabot::DependencyGroupEngine.reset! end context "when the same dependencies need to be updated to the same target versions" do @@ -199,4 +198,31 @@ group_update_all.perform end end + + context "when the target dependency group no longer matches any dependencies in the project" do + let(:job_definition) do + job_definition_fixture("bundler/version_updates/group_update_refresh_empty_group") + end + + let(:dependency_files) do + original_bundler_files + end + + before do + stub_rubygems_calls + end + + it "logs a warning and tells the service to close the Pull Request" do + # Our mocks will fail due to unexpected messages if any errors or PRs are dispatched + + allow(Dependabot.logger).to receive(:warn) + expect(Dependabot.logger).to receive(:warn).with( + "Skipping update group for 'everything-everywhere-all-at-once' as it does not match any allowed dependencies." + ) + + expect(mock_service).to receive(:close_pull_request).with(["dummy-pkg-b"], :dependency_group_empty) + + group_update_all.perform + end + end end diff --git a/updater/spec/dependabot/updater_spec.rb b/updater/spec/dependabot/updater_spec.rb index a723c853faa..49c9a727886 100644 --- a/updater/spec/dependabot/updater_spec.rb +++ b/updater/spec/dependabot/updater_spec.rb @@ -2237,7 +2237,6 @@ def expect_update_checker_with_ignored_versions(versions, dependency_matcher: an describe "#run with the grouped experiment enabled" do after do Dependabot::Experiments.reset! - Dependabot::DependencyGroupEngine.reset! end it "updates multiple dependencies in a single PR correctly" do diff --git a/updater/spec/fixtures/job_definitions/bundler/version_updates/group_update_all_empty_group.yaml b/updater/spec/fixtures/job_definitions/bundler/version_updates/group_update_all_empty_group.yaml new file mode 100644 index 00000000000..9a27e46ff58 --- /dev/null +++ b/updater/spec/fixtures/job_definitions/bundler/version_updates/group_update_all_empty_group.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: everything-everywhere-all-at-once + rules: + patterns: + - "*bagel" diff --git a/updater/spec/fixtures/job_definitions/bundler/version_updates/group_update_refresh_empty_group.yaml b/updater/spec/fixtures/job_definitions/bundler/version_updates/group_update_refresh_empty_group.yaml new file mode 100644 index 00000000000..77502ec14bb --- /dev/null +++ b/updater/spec/fixtures/job_definitions/bundler/version_updates/group_update_refresh_empty_group.yaml @@ -0,0 +1,45 @@ +job: + package-manager: bundler + source: + provider: github + repo: dependabot/smoke-tests + directory: "/bundler" + branch: + api-endpoint: https://api.github.com/ + hostname: github.com + dependencies: + - dummy-pkg-b + existing-pull-requests: [] + existing-group-pull-requests: + - dependency-group-name: everything-everywhere-all-at-once + dependencies: + - dependency-name: dummy-pkg-b + dependency-version: 1.2.0 + updating-a-pull-request: true + 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: everything-everywhere-all-at-once + rules: + patterns: + - "*bagel" + dependency-group-to-refresh: everything-everywhere-all-at-once diff --git a/updater/spec/fixtures/jobs/job_with_dependency_groups.json b/updater/spec/fixtures/jobs/job_with_dependency_groups.json deleted file mode 100644 index 41dba47b1df..00000000000 --- a/updater/spec/fixtures/jobs/job_with_dependency_groups.json +++ /dev/null @@ -1,53 +0,0 @@ -{ - "job": { - "allowed-updates": [ - { - "dependency-type": "direct", - "update-type": "all" - } - ], - "credentials-metadata": [ - { - "type": "git_source", - "host": "github.com" - } - ], - "dependencies": null, - "directory": "/", - "existing-pull-requests": [], - "ignore-conditions": [], - "security-advisories": [], - "package-manager": "bundler", - "repo-name": "dependabot-fixtures/dependabot-test-ruby-package", - "source": { - "provider": "github", - "repo": "dependabot-fixtures/dependabot-test-ruby-package", - "directory": "/", - "branch": null, - "hostname": "github.com", - "api-endpoint": "https://api.github.com/" - }, - "lockfile-only": false, - "requirements-update-strategy": null, - "update-subdependencies": false, - "updating-a-pull-request": false, - "vendor-dependencies": false, - "security-updates-only": false, - "dependency-groups": [ - { - "name": "group-a", - "rules": { - "patterns": ["dummy-pkg-*"], - "exclude-patterns": ["dummy-pkg-b"] - } - }, - { - "name": "group-b", - "rules": { - "patterns": ["dummy-pkg-b"] - } - } - ], - "experiments": { "grouped_updates_prototype": true } - } -}