From ff792bcbb07c4fc116fe9a5625943153ded95048 Mon Sep 17 00:00:00 2001 From: Nish Sinha Date: Wed, 5 Apr 2023 17:41:32 -0400 Subject: [PATCH 01/25] GroupRule -> DependencyGroup --- .../{group_rule.rb => dependency_group.rb} | 2 +- common/lib/dependabot/pull_request_creator.rb | 2 +- .../pull_request_creator/branch_namer.rb | 12 ++++---- .../branch_namer/dependency_group_strategy.rb | 28 +++++++++++++++++++ .../branch_namer/group_rule_strategy.rb | 28 ------------------- .../spec/dependabot/dependency_group_spec.rb | 14 ++++++++++ common/spec/dependabot/group_rule_spec.rb | 14 ---------- .../dependency_group_strategy_spec.rb | 19 +++++++++++++ .../branch_namer/group_rule_strategy_spec.rb | 19 ------------- .../pull_request_creator/branch_namer_spec.rb | 18 ++++++------ updater/lib/dependabot/api_client.rb | 4 +-- updater/lib/dependabot/dependency_change.rb | 10 +++---- .../dependabot/dependency_change_builder.rb | 10 +++---- .../operations/group_update_all_versions.rb | 18 ++++++------ updater/spec/dependabot/api_client_spec.rb | 4 +-- .../dependency_change_builder_spec.rb | 4 +-- .../spec/dependabot/dependency_change_spec.rb | 4 +-- 17 files changed, 106 insertions(+), 104 deletions(-) rename common/lib/dependabot/{group_rule.rb => dependency_group.rb} (84%) create mode 100644 common/lib/dependabot/pull_request_creator/branch_namer/dependency_group_strategy.rb delete mode 100644 common/lib/dependabot/pull_request_creator/branch_namer/group_rule_strategy.rb create mode 100644 common/spec/dependabot/dependency_group_spec.rb delete mode 100644 common/spec/dependabot/group_rule_spec.rb create mode 100644 common/spec/dependabot/pull_request_creator/branch_namer/dependency_group_strategy_spec.rb delete mode 100644 common/spec/dependabot/pull_request_creator/branch_namer/group_rule_strategy_spec.rb diff --git a/common/lib/dependabot/group_rule.rb b/common/lib/dependabot/dependency_group.rb similarity index 84% rename from common/lib/dependabot/group_rule.rb rename to common/lib/dependabot/dependency_group.rb index 582f83b4d9e..f6163ae97bc 100644 --- a/common/lib/dependabot/group_rule.rb +++ b/common/lib/dependabot/dependency_group.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module Dependabot - class GroupRule + class DependencyGroup attr_reader :name def initialize(name) diff --git a/common/lib/dependabot/pull_request_creator.rb b/common/lib/dependabot/pull_request_creator.rb index 34fb0bf93c4..4b86e8ec45b 100644 --- a/common/lib/dependabot/pull_request_creator.rb +++ b/common/lib/dependabot/pull_request_creator.rb @@ -235,7 +235,7 @@ def branch_namer dependencies: dependencies, files: files, target_branch: source.branch, - group_rule: nil, + dependency_group: nil, separator: branch_name_separator, prefix: branch_name_prefix, max_length: branch_name_max_length diff --git a/common/lib/dependabot/pull_request_creator/branch_namer.rb b/common/lib/dependabot/pull_request_creator/branch_namer.rb index 3dbfef36365..049b3c8fe55 100644 --- a/common/lib/dependabot/pull_request_creator/branch_namer.rb +++ b/common/lib/dependabot/pull_request_creator/branch_namer.rb @@ -9,14 +9,14 @@ module Dependabot class PullRequestCreator class BranchNamer - attr_reader :dependencies, :files, :target_branch, :separator, :prefix, :max_length, :group_rule + attr_reader :dependencies, :files, :target_branch, :separator, :prefix, :max_length, :dependency_group - def initialize(dependencies:, files:, target_branch:, group_rule: nil, + def initialize(dependencies:, files:, target_branch:, dependency_group: nil, separator: "/", prefix: "dependabot", max_length: nil) @dependencies = dependencies @files = files @target_branch = target_branch - @group_rule = group_rule + @dependency_group = dependency_group @separator = separator @prefix = prefix @max_length = max_length @@ -30,7 +30,7 @@ def new_branch_name def strategy @strategy ||= - if group_rule.nil? + if dependency_group.nil? SoloStrategy.new( dependencies: dependencies, files: files, @@ -40,11 +40,11 @@ def strategy max_length: max_length ) else - GroupRuleStrategy.new( + DependencyGroupStrategy.new( dependencies: dependencies, files: files, target_branch: target_branch, - group_rule: group_rule, + dependency_group: dependency_group, separator: separator, prefix: prefix, max_length: max_length diff --git a/common/lib/dependabot/pull_request_creator/branch_namer/dependency_group_strategy.rb b/common/lib/dependabot/pull_request_creator/branch_namer/dependency_group_strategy.rb new file mode 100644 index 00000000000..8b84dfa5fbf --- /dev/null +++ b/common/lib/dependabot/pull_request_creator/branch_namer/dependency_group_strategy.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +module Dependabot + class PullRequestCreator + class BranchNamer + class DependencyGroupStrategy + def initialize(dependencies:, files:, target_branch:, dependency_group:, + separator: "/", prefix: "dependabot", max_length: nil) + @dependencies = dependencies + @files = files + @target_branch = target_branch + @dependency_group = dependency_group + @separator = separator + @prefix = prefix + @max_length = max_length + end + + def new_branch_name + dependency_group.name + end + + private + + attr_reader :dependency_group + end + end + end +end diff --git a/common/lib/dependabot/pull_request_creator/branch_namer/group_rule_strategy.rb b/common/lib/dependabot/pull_request_creator/branch_namer/group_rule_strategy.rb deleted file mode 100644 index 65b6f4d46ba..00000000000 --- a/common/lib/dependabot/pull_request_creator/branch_namer/group_rule_strategy.rb +++ /dev/null @@ -1,28 +0,0 @@ -# frozen_string_literal: true - -module Dependabot - class PullRequestCreator - class BranchNamer - class GroupRuleStrategy - def initialize(dependencies:, files:, target_branch:, group_rule:, - separator: "/", prefix: "dependabot", max_length: nil) - @dependencies = dependencies - @files = files - @target_branch = target_branch - @group_rule = group_rule - @separator = separator - @prefix = prefix - @max_length = max_length - end - - def new_branch_name - group_rule.name - end - - private - - attr_reader :group_rule - end - end - end -end diff --git a/common/spec/dependabot/dependency_group_spec.rb b/common/spec/dependabot/dependency_group_spec.rb new file mode 100644 index 00000000000..a5e4bc578a3 --- /dev/null +++ b/common/spec/dependabot/dependency_group_spec.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +require "dependabot/dependency_group" + +RSpec.describe Dependabot::DependencyGroup do + describe "#name" do + it "returns the name" do + my_dependency_group_name = "Darren from work" + dependency_group = described_class.new(my_dependency_group_name) + + expect(dependency_group.name).to eq(my_dependency_group_name) + end + end +end diff --git a/common/spec/dependabot/group_rule_spec.rb b/common/spec/dependabot/group_rule_spec.rb deleted file mode 100644 index e2443c38890..00000000000 --- a/common/spec/dependabot/group_rule_spec.rb +++ /dev/null @@ -1,14 +0,0 @@ -# frozen_string_literal: true - -require "dependabot/group_rule" - -RSpec.describe Dependabot::GroupRule do - describe "#name" do - it "returns the name" do - my_group_rule_name = "Darren from work" - group_rule = described_class.new(my_group_rule_name) - - expect(group_rule.name).to eq(my_group_rule_name) - end - end -end diff --git a/common/spec/dependabot/pull_request_creator/branch_namer/dependency_group_strategy_spec.rb b/common/spec/dependabot/pull_request_creator/branch_namer/dependency_group_strategy_spec.rb new file mode 100644 index 00000000000..4b64b64b810 --- /dev/null +++ b/common/spec/dependabot/pull_request_creator/branch_namer/dependency_group_strategy_spec.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +require "dependabot/pull_request_creator/branch_namer/dependency_group_strategy" + +RSpec.describe Dependabot::PullRequestCreator::BranchNamer::DependencyGroupStrategy do + describe "#new_branch_name" do + it "returns the name of the dependency group" do + dependency_group = double("DependencyGroup", name: "my_dependency_group") + strategy = described_class.new( + dependencies: [], + files: [], + target_branch: "main", + dependency_group: dependency_group + ) + + expect(strategy.new_branch_name).to eq(dependency_group.name) + end + end +end diff --git a/common/spec/dependabot/pull_request_creator/branch_namer/group_rule_strategy_spec.rb b/common/spec/dependabot/pull_request_creator/branch_namer/group_rule_strategy_spec.rb deleted file mode 100644 index 7e201659261..00000000000 --- a/common/spec/dependabot/pull_request_creator/branch_namer/group_rule_strategy_spec.rb +++ /dev/null @@ -1,19 +0,0 @@ -# frozen_string_literal: true - -require "dependabot/pull_request_creator/branch_namer/group_rule_strategy" - -RSpec.describe Dependabot::PullRequestCreator::BranchNamer::GroupRuleStrategy do - describe "#new_branch_name" do - it "returns the name of the group rule" do - group_rule = double("GroupRule", name: "my_group_rule") - strategy = described_class.new( - dependencies: [], - files: [], - target_branch: "main", - group_rule: group_rule - ) - - expect(strategy.new_branch_name).to eq(group_rule.name) - end - end -end diff --git a/common/spec/dependabot/pull_request_creator/branch_namer_spec.rb b/common/spec/dependabot/pull_request_creator/branch_namer_spec.rb index 9a52e690ccd..3c37ff9be29 100644 --- a/common/spec/dependabot/pull_request_creator/branch_namer_spec.rb +++ b/common/spec/dependabot/pull_request_creator/branch_namer_spec.rb @@ -6,7 +6,7 @@ require "dependabot/dependency_file" require "dependabot/pull_request_creator/branch_namer" require "dependabot/pull_request_creator/branch_namer/solo_strategy" -require "dependabot/pull_request_creator/branch_namer/group_rule_strategy" +require "dependabot/pull_request_creator/branch_namer/dependency_group_strategy" RSpec.describe Dependabot::PullRequestCreator::BranchNamer do subject(:namer) do @@ -657,7 +657,7 @@ end end - context "when no group rule is present" do + context "when no dependency group is present" do it "delegates to a solo strategy" do strategy = instance_double(described_class::SoloStrategy) allow(described_class::SoloStrategy).to receive(:new).and_return(strategy) @@ -667,7 +667,7 @@ dependencies: dependencies, files: files, target_branch: target_branch, - group_rule: nil + dependency_group: nil ) expect(strategy).to receive(:new_branch_name) @@ -676,18 +676,18 @@ end end - context "when a group rule is present" do - it "delegates to a group rule strategy" do - strategy = instance_double(described_class::GroupRuleStrategy) - allow(described_class::GroupRuleStrategy).to receive(:new).and_return(strategy) + context "when a dependency group is present" do + it "delegates to a dependency group strategy" do + strategy = instance_double(described_class::DependencyGroupStrategy) + allow(described_class::DependencyGroupStrategy).to receive(:new).and_return(strategy) - group_rule = double("GroupRule", name: "my_group_rule") + dependency_group = double("DependencyGroup", name: "my_dependency_group") branch_namer = described_class.new( dependencies: dependencies, files: files, target_branch: target_branch, - group_rule: group_rule + dependency_group: dependency_group ) expect(strategy).to receive(:new_branch_name) diff --git a/updater/lib/dependabot/api_client.rb b/updater/lib/dependabot/api_client.rb index bfdf01fc8d5..7fd6c256c21 100644 --- a/updater/lib/dependabot/api_client.rb +++ b/updater/lib/dependabot/api_client.rb @@ -182,10 +182,10 @@ def create_pull_request_data(dependency_change, base_commit_sha) "updated-dependency-files": dependency_change.updated_dependency_files_hash, "base-commit-sha": base_commit_sha }.merge({ - # TODO: Replace this flag with a group-rule object + # TODO: Replace this flag with a dependency-group object # # In future this should be something like: - # "group-rule": dependency_change.group_rule_hash + # "dependency-group": dependency_change.dependency_group_hash # # This will allow us to pass back the rule id and other parameters # to allow Dependabot API to augment PR creation and associate it diff --git a/updater/lib/dependabot/dependency_change.rb b/updater/lib/dependabot/dependency_change.rb index c257388b886..7f38d989123 100644 --- a/updater/lib/dependabot/dependency_change.rb +++ b/updater/lib/dependabot/dependency_change.rb @@ -5,7 +5,7 @@ # # It includes a list of changed Dependabot::Dependency objects, an array of # Dependabot::DependencyFile objects which contain the changes to be applied -# along with any Dependabot::GroupRule that was used to generate the change. +# along with any Dependabot::DependencyGroup that was used to generate the change. # # This class provides methods for presenting the change set which can be used # by adapters to create a Pull Request, apply the changes on disk, etc. @@ -13,11 +13,11 @@ module Dependabot class DependencyChange attr_reader :job, :updated_dependencies, :updated_dependency_files - def initialize(job:, updated_dependencies:, updated_dependency_files:, group_rule: nil) + def initialize(job:, dependencies:, updated_dependency_files:, dependency_group: nil) @job = job @updated_dependencies = updated_dependencies @updated_dependency_files = updated_dependency_files - @group_rule = group_rule + @dependency_group = dependency_group end def pr_message @@ -42,11 +42,11 @@ def updated_dependency_files_hash updated_dependency_files.map(&:to_h) end - # FIXME: This is a placeholder for using a concrete GroupRule object to create + # FIXME: This is a placeholder for using a concrete DependencyGroup object to create # as grouped rule hash to pass to the Dependabot API client. For now, we just # use a flag on whether a rule has been assigned to the change. def grouped_update? - !!@group_rule + !!@dependency_group end end end diff --git a/updater/lib/dependabot/dependency_change_builder.rb b/updater/lib/dependabot/dependency_change_builder.rb index b9aaf3589bb..16fa26aac9d 100644 --- a/updater/lib/dependabot/dependency_change_builder.rb +++ b/updater/lib/dependabot/dependency_change_builder.rb @@ -3,7 +3,7 @@ require "dependabot/dependency" require "dependabot/dependency_change" require "dependabot/file_updaters" -require "dependabot/group_rule" +require "dependabot/dependency_group" # This class is responsible for generating a DependencyChange for a given # set of dependencies and dependency files. @@ -18,7 +18,7 @@ # The set of dependency updates to be applied to the dependency files # - change_source: # A change can be generated from either a single 'lead' Dependency or -# a GroupRule +# a DependencyGroup module Dependabot class DependencyChangeBuilder def self.create_from(**kwargs) @@ -47,7 +47,7 @@ def run job: job, updated_dependencies: updated_deps, updated_dependency_files: updated_files, - group_rule: source_group_rule + dependency_group: source_dependency_group ) end @@ -61,8 +61,8 @@ def source_dependency_name change_source.name end - def source_group_rule - return nil unless change_source.is_a? Dependabot::GroupRule + def source_dependency_group + return nil unless change_source.is_a? Dependabot::DependencyGroup change_source 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 78911de2054..4cec6f77865 100644 --- a/updater/lib/dependabot/updater/operations/group_update_all_versions.rb +++ b/updater/lib/dependabot/updater/operations/group_update_all_versions.rb @@ -10,7 +10,7 @@ # - It disregards any ignore rules for sake of simplicity # - It has no superseding logic, so every time this strategy runs for a repo # it will create a new Pull Request regardless of any existing, open PR -# - The concept of a 'group rule' or 'update group' which configures which +# - The concept of a 'dependency group' or 'update group' which configures which # dependencies should go together is stubbed out; it currently makes best # effort to update everything it can in one pass. module Dependabot @@ -22,7 +22,8 @@ class GroupUpdateAllVersions def self.applies_to?(job:) return false if job.security_updates_only? return false if job.updating_a_pull_request? - return false if job.dependencies&.any? + #return false if job.dependencies&.any? + return false if job.dependency_groups.empty? Dependabot::Experiments.enabled?(:grouped_updates_prototype) end @@ -37,7 +38,8 @@ def initialize(service:, job:, dependency_snapshot:, error_handler:) @dependency_snapshot = dependency_snapshot @error_handler = error_handler # This is a placeholder for a real rule object obtained from config in future - @group_rule = Dependabot::GroupRule.new(name: GROUP_NAME_PLACEHOLDER) + #@dependency_group = Dependabot::DependencyGroup.new(name: GROUP_NAME_PLACEHOLDER) + @dependency_group = dependency_snapshot.dependency_groups end def perform @@ -55,14 +57,14 @@ def perform # FIXME: This is a workround for not having a single Dependency to report against # # We could use all_updated_deps.first, but that could be misleading. It may - # make more sense to handle the group rule as a Dependancy-ish object + # make more sense to handle the dependency group as a Dependancy-ish object group_dependency = OpenStruct.new(name: "group-all") raise if ErrorHandler::RUN_HALTING_ERRORS.keys.any? { |err| e.is_a?(err) } error_handler.handle_dependabot_error(error: e, dependency: group_dependency) end else - Dependabot.logger.info("Nothing to update for Group Rule: '#{GROUP_NAME_PLACEHOLDER}'") + Dependabot.logger.info("Nothing to update for Dependency Group: '#{GROUP_NAME_PLACEHOLDER}'") end end @@ -72,7 +74,7 @@ def perform :service, :dependency_snapshot, :error_handler, - :group_rule + :dependency_group def dependencies if dependency_snapshot.dependencies.any? && dependency_snapshot.allowed_dependencies.none? @@ -125,7 +127,7 @@ def compile_all_dependency_changes job: job, updated_dependencies: all_updated_dependencies, updated_dependency_files: updated_files, - group_rule: group_rule + dependency_group: dependency_group ) end @@ -138,7 +140,7 @@ def create_change_for(lead_dependency, updated_dependencies, dependency_files) job: job, dependency_files: dependency_files, updated_dependencies: updated_dependencies, - change_source: group_rule + change_source: dependency_group ) rescue Dependabot::InconsistentRegistryResponse => e error_handler.log_error( diff --git a/updater/spec/dependabot/api_client_spec.rb b/updater/spec/dependabot/api_client_spec.rb index 2ce3ccb887c..a806c5aa96d 100644 --- a/updater/spec/dependabot/api_client_spec.rb +++ b/updater/spec/dependabot/api_client_spec.rb @@ -185,12 +185,12 @@ end) end - it "flags the PR as a grouped-update if the dependency change has a group rule assigned" do + it "flags the PR as a grouped-update if the dependency change has a dependency group assigned" do grouped_dependency_change = Dependabot::DependencyChange.new( job: job, updated_dependencies: dependencies, updated_dependency_files: dependency_files, - group_rule: anything + dependency_group: anything ) client.create_pull_request(grouped_dependency_change, base_commit) diff --git a/updater/spec/dependabot/dependency_change_builder_spec.rb b/updater/spec/dependabot/dependency_change_builder_spec.rb index af0b8f6611e..19ea8e012ab 100644 --- a/updater/spec/dependabot/dependency_change_builder_spec.rb +++ b/updater/spec/dependabot/dependency_change_builder_spec.rb @@ -105,9 +105,9 @@ end end - context "when the source is a group rule" do + context "when the source is a dependency group" do let(:change_source) do - Dependabot::GroupRule.new(name: "dummy-pkg-*") + Dependabot::DependencyGroup.new(name: "dummy-pkg-*", rules: ["dummy-pkg-*"]) end it "creates a new DependencyChange flagged as a grouped update" do diff --git a/updater/spec/dependabot/dependency_change_spec.rb b/updater/spec/dependabot/dependency_change_spec.rb index 829ab97fea1..5e4f59738f7 100644 --- a/updater/spec/dependabot/dependency_change_spec.rb +++ b/updater/spec/dependabot/dependency_change_spec.rb @@ -111,13 +111,13 @@ expect(dependency_change.grouped_update?).to be false end - context "when a group rule is assigned" do + context "when a dependency group is assigned" do it "is true" do rule = described_class.new( job: job, updated_dependencies: updated_dependencies, updated_dependency_files: updated_dependency_files, - group_rule: anything # For now the group_rule parameter is treated permissively as any non-nil value + dependency_group: anything # For now the dependency_group parameter is treated permissively as any non-nil value ) expect(rule.grouped_update?).to be true From e8a2b4b0f8deb2e1a2a367fa6e120cd0a33461a9 Mon Sep 17 00:00:00 2001 From: Nish Sinha Date: Wed, 12 Apr 2023 18:17:42 -0400 Subject: [PATCH 02/25] Fix typo on DependencyChange --- updater/lib/dependabot/dependency_change.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/updater/lib/dependabot/dependency_change.rb b/updater/lib/dependabot/dependency_change.rb index 7f38d989123..5d3dda0a2fa 100644 --- a/updater/lib/dependabot/dependency_change.rb +++ b/updater/lib/dependabot/dependency_change.rb @@ -13,7 +13,7 @@ module Dependabot class DependencyChange attr_reader :job, :updated_dependencies, :updated_dependency_files - def initialize(job:, dependencies:, updated_dependency_files:, dependency_group: nil) + def initialize(job:, updated_dependencies:, updated_dependency_files:, dependency_group: nil) @job = job @updated_dependencies = updated_dependencies @updated_dependency_files = updated_dependency_files From ccafaa623f5e78d1471a3dea3e57426f22cc2a31 Mon Sep 17 00:00:00 2001 From: Nish Sinha Date: Wed, 5 Apr 2023 21:07:10 -0400 Subject: [PATCH 03/25] Add dependency_groups to a Job --- updater/lib/dependabot/job.rb | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/updater/lib/dependabot/job.rb b/updater/lib/dependabot/job.rb index 899e0431007..cd10d3086f7 100644 --- a/updater/lib/dependabot/job.rb +++ b/updater/lib/dependabot/job.rb @@ -37,6 +37,7 @@ class Job update_subdependencies updating_a_pull_request vendor_dependencies + dependency_groups ) attr_reader :allowed_updates, @@ -51,7 +52,8 @@ class Job :security_updates_only, :source, :token, - :vendor_dependencies + :vendor_dependencies, + :dependency_groups def self.new_fetch_job(job_id:, job_definition:, repo_contents_path: nil) attrs = standardise_keys(job_definition["job"]).slice(*PERMITTED_KEYS) @@ -94,6 +96,7 @@ def initialize(attributes) @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, []) register_experiments end @@ -233,6 +236,17 @@ def security_advisories_for(dependency) end end + def register_dependency_groups + dependency_groups.each do |group| + Dependabot::DependencyGroupEngine.register(group["name"], group["rules"]) + end + end + + def belongs_to_dependency_group?(dependency) + return false unless dependency_groups.any? + Dependabot::DependencyGroupEngine.groups_for(dependency).any? + end + def ignore_conditions_for(dependency) update_config.ignored_versions_for( dependency, From 740a46938f1c97dc31dabff69069eea87e468561 Mon Sep 17 00:00:00 2001 From: Nish Sinha Date: Wed, 12 Apr 2023 21:35:57 -0400 Subject: [PATCH 04/25] Add a dependency groups job fixture --- .../jobs/job_with_dependency_groups.json | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 updater/spec/fixtures/jobs/job_with_dependency_groups.json diff --git a/updater/spec/fixtures/jobs/job_with_dependency_groups.json b/updater/spec/fixtures/jobs/job_with_dependency_groups.json new file mode 100644 index 00000000000..e1b5660569e --- /dev/null +++ b/updater/spec/fixtures/jobs/job_with_dependency_groups.json @@ -0,0 +1,48 @@ +{ + "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": ["dummy-pkg-*"] + }, + { + "name": "group-b", + "rules": ["dummy-pkg-b"] + } + ], + "experiments": { "grouped_updates_prototype": true } + } +} From e9f8b38be0598d63a1850786b76fb90c6f20d406 Mon Sep 17 00:00:00 2001 From: Nish Sinha Date: Wed, 12 Apr 2023 18:03:19 -0400 Subject: [PATCH 05/25] Add #rules and #contains? to DependencyGroups --- common/lib/dependabot/dependency_group.rb | 24 +++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/common/lib/dependabot/dependency_group.rb b/common/lib/dependabot/dependency_group.rb index f6163ae97bc..1f0cbc26dc1 100644 --- a/common/lib/dependabot/dependency_group.rb +++ b/common/lib/dependabot/dependency_group.rb @@ -2,10 +2,30 @@ module Dependabot class DependencyGroup - attr_reader :name + attr_reader :name, :rules, :dependencies - def initialize(name) + def initialize(name, rule) @name = name + @rules = rule + @dependencies = [] end + + def contains?(dependency) + @dependencies.include?(dependency) if @dependencies.any? + rules.any? { |rule| WildcardMatcher.match?(rule, dependency.name) } + end + end +end + +# Copied from updater/lib/wildcard_matcher.rb +class WildcardMatcher + def self.match?(wildcard_string, candidate_string) + return false unless wildcard_string && candidate_string + + regex_string = "a#{wildcard_string.downcase}a".split("*"). + map { |p| Regexp.quote(p) }. + join(".*").gsub(/^a|a$/, "") + regex = /^#{regex_string}$/ + regex.match?(candidate_string.downcase) end end From f35246ac1cfa6742063b22e689b4e11ea023214b Mon Sep 17 00:00:00 2001 From: Nish Sinha Date: Wed, 12 Apr 2023 21:27:43 -0400 Subject: [PATCH 06/25] Add DependencyGroup tests --- .../spec/dependabot/dependency_group_spec.rb | 97 ++++++++++++++++++- 1 file changed, 94 insertions(+), 3 deletions(-) diff --git a/common/spec/dependabot/dependency_group_spec.rb b/common/spec/dependabot/dependency_group_spec.rb index a5e4bc578a3..5761ec290a3 100644 --- a/common/spec/dependabot/dependency_group_spec.rb +++ b/common/spec/dependabot/dependency_group_spec.rb @@ -1,14 +1,105 @@ # frozen_string_literal: true require "dependabot/dependency_group" +require "dependabot/dependency" +# TODO: Once the Updater has been merged into Core, we should test this +# using the DependencyGroupEngine methods instead of mocking the functionality RSpec.describe Dependabot::DependencyGroup do + let(:dependency_group) { described_class.new(name, rules) } + let(:name) { "test_group" } + let(:rules) { ["test-*"] } + + let(:test_dependency_1) do + Dependabot::Dependency.new( + name: "test-dependency-1", + package_manager: "bundler", + version: "1.1.0", + requirements: [ + { + file: "Gemfile", + requirement: "~> 1.1.0", + groups: [], + source: nil + } + ] + ) + end + + let(:test_dependency_2) do + Dependabot::Dependency.new( + name: "another-test-dependency", + package_manager: "bundler", + version: "1.1.0", + requirements: [ + { + file: "Gemfile", + requirement: "~> 1.1.0", + groups: [], + source: nil + } + ] + ) + end + describe "#name" do it "returns the name" do - my_dependency_group_name = "Darren from work" - dependency_group = described_class.new(my_dependency_group_name) + expect(dependency_group.name).to eq(name) + end + end + + describe "#rules" do + it "returns a list of rules" do + expect(dependency_group.rules).to eq(rules) + end + end + + describe "#dependencies" do + context "when calculate_dependency_groups! has not been run" do + it "returns an empty list" do + expect(dependency_group.dependencies).to eq([]) + end + end + + context "after calculate_dependency_groups! has been run" do + before do + dependency_group.dependencies << test_dependency_1 + end + + it "returns the dependencies" do + expect(dependency_group.dependencies).to include(test_dependency_1) + expect(dependency_group.dependencies).not_to include(test_dependency_2) + end + end + end + + describe "#contains?" do + context "before calculate_dependency_groups! has been run" do + it "returns true if the dependency matches a rule" do + expect(dependency_group.dependencies).to eq([]) + expect(dependency_group.contains?(test_dependency_1)).to be_truthy + end + + it "returns false if the dependency does not match a rule" do + expect(dependency_group.dependencies).to eq([]) + expect(dependency_group.contains?(test_dependency_2)).to be_falsey + end + end + + context "after calculate_dependency_groups! has been run" do + before do + dependency_group.dependencies << test_dependency_1 + end + + it "returns true if the dependency is in the dependency list" do + expect(dependency_group.dependencies).to include(test_dependency_1) + expect(dependency_group.contains?(test_dependency_1)).to be_truthy + end - expect(dependency_group.name).to eq(my_dependency_group_name) + 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_dependency_1) + expect(dependency_group.contains?(test_dependency_2)).to be_falsey + end end end end From 45dd99b4a0089da41454a0930a52466cc7547ad0 Mon Sep 17 00:00:00 2001 From: Nish Sinha Date: Wed, 12 Apr 2023 18:32:37 -0400 Subject: [PATCH 07/25] Introduce the DependencyGroupEngine --- .../lib/dependabot/dependency_group_engine.rb | 83 +++++++++++++++++++ 1 file changed, 83 insertions(+) create mode 100644 updater/lib/dependabot/dependency_group_engine.rb diff --git a/updater/lib/dependabot/dependency_group_engine.rb b/updater/lib/dependabot/dependency_group_engine.rb new file mode 100644 index 00000000000..cbcf33b7c5e --- /dev/null +++ b/updater/lib/dependabot/dependency_group_engine.rb @@ -0,0 +1,83 @@ +# frozen_string_literal: true + +require "dependabot/dependency_group" + +# This class implements our strategy for keeping track of and matching dependency +# groups that are defined by users in their dependabot config file. +# +# Each UpdateJob registers its own DependencyGroupEngine which calculates +# the grouped and ungrouped dependencies for a DependencySnapshot +# +# This is a static class tied to the lifecycle of a Job +# Its methods should only be called with Dependabot::DependencyGroupEngine +# +# 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 +# +# **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 = [] + + def self.reset! + @groups_calculated = false + @registered_groups = [] + + @dependency_groups = {} + @ungrouped_dependencies = [] + end + + # Eventually the key for a dependency group should be a hash since names _can_ conflict within jobs + def self.register(name, rule) + @registered_groups.push Dependabot::DependencyGroup.new(name, rule) + end + + def self.groups_for(dependency) + return [] if dependency.nil? + return [] unless dependency.instance_of?(Dependabot::Dependency) + + @registered_groups.select do |group| + group.contains?(dependency) + end + end + + # { group_name => [DependencyGroup], ... } + def self.dependency_groups(dependencies) + return @dependency_groups if @groups_calculated + + @groups_calculated = calculate_dependency_groups!(dependencies) + + @dependency_groups + end + + # 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 + + @groups_calculated = calculate_dependency_groups!(dependencies) + + @ungrouped_dependencies + end + + def self.calculate_dependency_groups!(dependencies) + dependencies.inject(@dependency_groups) do |_dependency_groups, dependency| + groups = groups_for(dependency) + + @ungrouped_dependencies << dependency if groups.empty? + + groups.each do |group| + group.dependencies.push(dependency) + @dependency_groups[group.name.to_sym] = group + end + end + + true + end + end +end From 60b3b24f89df37710cc1cc8f935b29a4173bc68a Mon Sep 17 00:00:00 2001 From: Nish Sinha Date: Wed, 12 Apr 2023 18:33:14 -0400 Subject: [PATCH 08/25] Wire the DependencyGroupEngine into GroupUpdateAllVersions --- .../updater/operations/group_update_all_versions.rb | 10 +++------- 1 file changed, 3 insertions(+), 7 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 4cec6f77865..6a949e06113 100644 --- a/updater/lib/dependabot/updater/operations/group_update_all_versions.rb +++ b/updater/lib/dependabot/updater/operations/group_update_all_versions.rb @@ -22,7 +22,7 @@ class GroupUpdateAllVersions def self.applies_to?(job:) return false if job.security_updates_only? return false if job.updating_a_pull_request? - #return false if job.dependencies&.any? + return false if job.dependencies&.any? return false if job.dependency_groups.empty? Dependabot::Experiments.enabled?(:grouped_updates_prototype) @@ -37,9 +37,6 @@ def initialize(service:, job:, dependency_snapshot:, error_handler:) @job = job @dependency_snapshot = dependency_snapshot @error_handler = error_handler - # This is a placeholder for a real rule object obtained from config in future - #@dependency_group = Dependabot::DependencyGroup.new(name: GROUP_NAME_PLACEHOLDER) - @dependency_group = dependency_snapshot.dependency_groups end def perform @@ -74,7 +71,6 @@ def perform :service, :dependency_snapshot, :error_handler, - :dependency_group def dependencies if dependency_snapshot.dependencies.any? && dependency_snapshot.allowed_dependencies.none? @@ -127,7 +123,7 @@ def compile_all_dependency_changes job: job, updated_dependencies: all_updated_dependencies, updated_dependency_files: updated_files, - dependency_group: dependency_group + dependency_group: Dependabot::DependencyGroupEngine.group_for(all_updated_dependencies.first) ) end @@ -140,7 +136,7 @@ def create_change_for(lead_dependency, updated_dependencies, dependency_files) job: job, dependency_files: dependency_files, updated_dependencies: updated_dependencies, - change_source: dependency_group + change_source: Dependabot::DependencyGroupEngine.group_for(lead_dependency) ) rescue Dependabot::InconsistentRegistryResponse => e error_handler.log_error( From f0986770aeda75d57d923612e3e8bb78f7e721d3 Mon Sep 17 00:00:00 2001 From: Nish Sinha Date: Wed, 12 Apr 2023 17:55:51 -0400 Subject: [PATCH 09/25] Wire the DependencyGroupEngine to a Job --- updater/lib/dependabot/job.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/updater/lib/dependabot/job.rb b/updater/lib/dependabot/job.rb index cd10d3086f7..0e0eac78575 100644 --- a/updater/lib/dependabot/job.rb +++ b/updater/lib/dependabot/job.rb @@ -2,6 +2,7 @@ require "dependabot/config/ignore_condition" require "dependabot/config/update_config" +require "dependabot/dependency_group_engine" require "dependabot/experiments" require "dependabot/source" require "wildcard_matcher" @@ -99,6 +100,7 @@ def initialize(attributes) @dependency_groups = attributes.fetch(:dependency_groups, []) register_experiments + register_dependency_groups end def clone? @@ -244,6 +246,7 @@ def register_dependency_groups def belongs_to_dependency_group?(dependency) return false unless dependency_groups.any? + Dependabot::DependencyGroupEngine.groups_for(dependency).any? end From d4023a8008f35cda86c31d33b03f75902be1fe4d Mon Sep 17 00:00:00 2001 From: Nish Sinha Date: Wed, 12 Apr 2023 21:31:02 -0400 Subject: [PATCH 10/25] Test that a Job registers its dependency groups --- updater/spec/dependabot/job_spec.rb | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/updater/spec/dependabot/job_spec.rb b/updater/spec/dependabot/job_spec.rb index a9e51b81752..f64df4ae1c7 100644 --- a/updater/spec/dependabot/job_spec.rb +++ b/updater/spec/dependabot/job_spec.rb @@ -39,7 +39,8 @@ vendor_dependencies: vendor_dependencies, experiments: experiments, commit_message_options: commit_message_options, - security_updates_only: security_updates_only + security_updates_only: security_updates_only, + dependency_groups: dependency_groups } end @@ -62,6 +63,7 @@ let(:experiments) { nil } let(:commit_message_options) { nil } let(:vendor_dependencies) { false } + let(:dependency_groups) { [] } describe "::new_update_job" do let(:job_json) { fixture("jobs/job_with_credentials.json") } @@ -85,6 +87,11 @@ 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(:job).to receive(:register_dependency_groups).with(dependency_groups) + new_update_job + end end describe "#allowed_update?" do From ed1111710d3da8dd2ff1d83a854add049bbb2eae Mon Sep 17 00:00:00 2001 From: Nish Sinha Date: Wed, 12 Apr 2023 21:49:27 -0400 Subject: [PATCH 11/25] Add dependency group gem fixtures --- updater/spec/fixtures/bundler/grouped/Gemfile | 7 +++++++ .../spec/fixtures/bundler/grouped/Gemfile.lock | 18 ++++++++++++++++++ 2 files changed, 25 insertions(+) create mode 100644 updater/spec/fixtures/bundler/grouped/Gemfile create mode 100644 updater/spec/fixtures/bundler/grouped/Gemfile.lock diff --git a/updater/spec/fixtures/bundler/grouped/Gemfile b/updater/spec/fixtures/bundler/grouped/Gemfile new file mode 100644 index 00000000000..01ede77b86f --- /dev/null +++ b/updater/spec/fixtures/bundler/grouped/Gemfile @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +source "https://rubygems.org" + +gem "dummy-pkg-a", "~> 2.0.0" +gem "dummy-pkg-b", "~> 1.1.0" +gem "ungrouped-dummy-pkg-c", "~> 1.0.0" diff --git a/updater/spec/fixtures/bundler/grouped/Gemfile.lock b/updater/spec/fixtures/bundler/grouped/Gemfile.lock new file mode 100644 index 00000000000..48feef3532e --- /dev/null +++ b/updater/spec/fixtures/bundler/grouped/Gemfile.lock @@ -0,0 +1,18 @@ +GEM + remote: https://rubygems.org/ + specs: + dummy-pkg-a (2.0.0) + dummy-pkg-b (1.1.0) + dummy-pkg-a (~> 2.0) + ungrouped-dummy-pkg-c (1.0.0) + +PLATFORMS + ruby + +DEPENDENCIES + dummy-pkg-a (~> 2.0.0) + dummy-pkg-b (~> 1.1.0) + ungrouped-dummy-pkg-c (~> 1.0.0) + +BUNDLED WITH + 1.14.6 From 908c52ead87b4a1b6d195b3280edf607dc40a150 Mon Sep 17 00:00:00 2001 From: Nish Sinha Date: Wed, 12 Apr 2023 21:36:41 -0400 Subject: [PATCH 12/25] Add DependencyGroupEngine tests --- .../dependency_group_engine_spec.rb | 225 ++++++++++++++++++ 1 file changed, 225 insertions(+) create mode 100644 updater/spec/dependabot/dependency_group_engine_spec.rb diff --git a/updater/spec/dependabot/dependency_group_engine_spec.rb b/updater/spec/dependabot/dependency_group_engine_spec.rb new file mode 100644 index 00000000000..a8cdff8a670 --- /dev/null +++ b/updater/spec/dependabot/dependency_group_engine_spec.rb @@ -0,0 +1,225 @@ +# frozen_string_literal: true + +require "debug" +require "spec_helper" +require "dependabot/dependency_group_engine" +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 + 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/Gemfile"), + directory: "/" + ), + Dependabot::DependencyFile.new( + name: "Gemfile.lock", + content: fixture("bundler/grouped/Gemfile.lock"), + directory: "/" + ) + ] + end + + let(:base_commit_sha) do + "mock-sha" + end + + let(:encoded_dependency_files) do + dependency_files.map do |file| + base64_file = file.dup + base64_file.content = Base64.encode64(file.content) + base64_file.to_h + end + end + + let(:job_definition) do + { + "base_commit_sha" => base_commit_sha, + "base64_dependency_files" => encoded_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(: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 + before do + dependency_group_engine.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", ["dummy-pkg-*"]).and_call_original + expect(dependency_group_engine).to receive(:register).with("group-b", ["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 + before do + dependency_group_engine.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(2) + expect(dependency_group_engine.send(:groups_for, snapshot.dependencies[2]).count).to eq(0) + end + end + + describe "#dependency_groups" do + before do + dependency_group_engine.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"].first).to be_a(Dependabot::Dependency) + 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 + before do + dependency_group_engine.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 + end + + describe "#reset!" do + before do + dependency_group_engine.reset! + 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([]) + end + end + + describe "#calculate_dependency_groups!" do + before do + dependency_group_engine.reset! + 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 + 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 + end + end +end From 64b9f953c5603d53aa3229c88e5bee5573b56950 Mon Sep 17 00:00:00 2001 From: Nish Sinha Date: Wed, 12 Apr 2023 18:08:16 -0400 Subject: [PATCH 13/25] Update one DependencySnapshot group at a time in GroupUpdateAllVersions Add the #groups and #upgrouped_dependencies methods --- updater/lib/dependabot/dependency_snapshot.rb | 12 +++ .../operations/group_update_all_versions.rb | 85 +++++++++++-------- 2 files changed, 62 insertions(+), 35 deletions(-) diff --git a/updater/lib/dependabot/dependency_snapshot.rb b/updater/lib/dependabot/dependency_snapshot.rb index 26a04b7c7ef..2645d657bfd 100644 --- a/updater/lib/dependabot/dependency_snapshot.rb +++ b/updater/lib/dependabot/dependency_snapshot.rb @@ -53,6 +53,18 @@ def job_dependencies end 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) + end + + def ungrouped_dependencies + Dependabot::DependencyGroupEngine.ungrouped_dependencies(allowed_dependencies) + end + private def initialize(job:, base_commit_sha:, dependency_files:) 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 6a949e06113..05fc2278873 100644 --- a/updater/lib/dependabot/updater/operations/group_update_all_versions.rb +++ b/updater/lib/dependabot/updater/operations/group_update_all_versions.rb @@ -17,8 +17,6 @@ module Dependabot class Updater module Operations class GroupUpdateAllVersions - GROUP_NAME_PLACEHOLDER = "*" - def self.applies_to?(job:) return false if job.security_updates_only? return false if job.updating_a_pull_request? @@ -39,38 +37,46 @@ def initialize(service:, job:, dependency_snapshot:, error_handler:) @error_handler = error_handler end + # rubocop:disable Metrics/AbcSize def perform - Dependabot.logger.info("[Experimental] Starting grouped update job for #{job.source.repo}") - # We should log the rule being executed, let's just hard-code wildcard for now - # since the prototype makes best-effort to do everything in one pass. - Dependabot.logger.info("Starting update group for '#{GROUP_NAME_PLACEHOLDER}'") - dependency_change = compile_all_dependency_changes - - if dependency_change.updated_dependencies.any? - Dependabot.logger.info("Creating a pull request for '#{GROUP_NAME_PLACEHOLDER}'") - begin - service.create_pull_request(dependency_change, dependency_snapshot.base_commit_sha) - rescue StandardError => e - # FIXME: This is a workround for not having a single Dependency to report against - # - # We could use all_updated_deps.first, but that could be misleading. It may - # make more sense to handle the dependency group as a Dependancy-ish object - group_dependency = OpenStruct.new(name: "group-all") - raise if ErrorHandler::RUN_HALTING_ERRORS.keys.any? { |err| e.is_a?(err) } - - error_handler.handle_dependabot_error(error: e, dependency: group_dependency) + dependency_snapshot.groups.each do |_group_hash, group| + Dependabot.logger.info("[Experimental] Starting grouped update job for #{job.source.repo}") + # We should log the rule being executed, let's just hard-code wildcard for now + # since the prototype makes best-effort to do everything in one pass. + # This should be replaced with the actual Dependabot::DependencyGroup instances that have names + # dependency_snapshot.groups will need to be updated to return a list of + # Dependabot::DependencyGroup instances + 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 + # FIXME: This is a workround for not having a single Dependency to report against + # + # We could use all_updated_deps.first, but that could be misleading. It may + # make more sense to handle the dependency group as a Dependancy-ish object + group_dependency = OpenStruct.new(name: "group-all") + raise if ErrorHandler::RUN_HALTING_ERRORS.keys.any? { |err| e.is_a?(err) } + + error_handler.handle_dependabot_error(error: e, dependency: group_dependency) + end + else + Dependabot.logger.info("Nothing to update for Dependency Group: '#{group.name}'") end - else - Dependabot.logger.info("Nothing to update for Dependency Group: '#{GROUP_NAME_PLACEHOLDER}'") end - end - - private - attr_reader :job, - :service, - :dependency_snapshot, - :error_handler, + # Let's not worry about this for now, but eventually we'd try to do single updates + # for anything that didn't appear in _at least one_ group. + dependency_snapshot.ungrouped_dependencies.each do |dependency| + # dependency_change = create_dependency_change_for(dependency) + # create_pull_request(dependency_change) + end + end + # rubocop:enable Metrics/AbcSize def dependencies if dependency_snapshot.dependencies.any? && dependency_snapshot.allowed_dependencies.none? @@ -81,12 +87,21 @@ def dependencies dependency_snapshot.allowed_dependencies end + private + + attr_reader :job, + :service, + :dependency_snapshot, + :error_handler + # Returns a Dependabot::DependencyChange object that encapsulates the # outcome of attempting to update every dependency iteratively which # can be used for PR creation. - def compile_all_dependency_changes + def compile_all_dependency_changes_for(group) all_updated_dependencies = [] updated_files = dependencies.inject(dependency_snapshot.dependency_files) do |dependency_files, dependency| + next dependency_files unless group.contains?(dependency) + updated_dependencies = compile_updates_for(dependency, dependency_files) if updated_dependencies.any? @@ -94,7 +109,7 @@ def compile_all_dependency_changes dep.name.casecmp(dependency.name).zero? end - dependency_change = create_change_for(lead_dependency, updated_dependencies, dependency_files) + dependency_change = create_change_for(lead_dependency, updated_dependencies, dependency_files, group) # Move on to the next dependency using the existing files if we # could not create a change for any reason @@ -123,7 +138,7 @@ def compile_all_dependency_changes job: job, updated_dependencies: all_updated_dependencies, updated_dependency_files: updated_files, - dependency_group: Dependabot::DependencyGroupEngine.group_for(all_updated_dependencies.first) + dependency_group: group ) end @@ -131,12 +146,12 @@ def compile_all_dependency_changes # list of dependencies to be updated # # This method **must** return false in the event of an error - def create_change_for(lead_dependency, updated_dependencies, dependency_files) + def create_change_for(lead_dependency, updated_dependencies, dependency_files, dependency_group) Dependabot::DependencyChangeBuilder.create_from( job: job, dependency_files: dependency_files, updated_dependencies: updated_dependencies, - change_source: Dependabot::DependencyGroupEngine.group_for(lead_dependency) + change_source: dependency_group ) rescue Dependabot::InconsistentRegistryResponse => e error_handler.log_error( From a8aa68625d5fee1e33cea734459af984d3045252 Mon Sep 17 00:00:00 2001 From: Nish Sinha Date: Thu, 13 Apr 2023 16:15:27 -0400 Subject: [PATCH 14/25] Clean up the wording of tests and comments Co-authored-by: Barry Gordon <896971+brrygrdn@users.noreply.github.com> --- common/spec/dependabot/dependency_group_spec.rb | 8 ++++---- updater/lib/dependabot/dependency_group_engine.rb | 11 ++++------- .../operations/group_update_all_versions.rb | 15 +++++---------- 3 files changed, 13 insertions(+), 21 deletions(-) diff --git a/common/spec/dependabot/dependency_group_spec.rb b/common/spec/dependabot/dependency_group_spec.rb index 5761ec290a3..402f61d9250 100644 --- a/common/spec/dependabot/dependency_group_spec.rb +++ b/common/spec/dependabot/dependency_group_spec.rb @@ -55,13 +55,13 @@ end describe "#dependencies" do - context "when calculate_dependency_groups! has not been run" do + context "when no dependencies are assigned to the group" do it "returns an empty list" do expect(dependency_group.dependencies).to eq([]) end end - context "after calculate_dependency_groups! has been run" do + context "when dependencies have been assigned" do before do dependency_group.dependencies << test_dependency_1 end @@ -74,7 +74,7 @@ end describe "#contains?" do - context "before calculate_dependency_groups! has been run" 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_dependency_1)).to be_truthy @@ -86,7 +86,7 @@ end end - context "after calculate_dependency_groups! has been run" do + context "after dependencies are assigned to the group" do before do dependency_group.dependencies << test_dependency_1 end diff --git a/updater/lib/dependabot/dependency_group_engine.rb b/updater/lib/dependabot/dependency_group_engine.rb index cbcf33b7c5e..7f6b65d4a10 100644 --- a/updater/lib/dependabot/dependency_group_engine.rb +++ b/updater/lib/dependabot/dependency_group_engine.rb @@ -5,14 +5,11 @@ # This class implements our strategy for keeping track of and matching dependency # groups that are defined by users in their dependabot config file. # -# Each UpdateJob registers its own DependencyGroupEngine which calculates -# the grouped and ungrouped dependencies for a DependencySnapshot -# # This is a static class tied to the lifecycle of a Job -# Its methods should only be called with Dependabot::DependencyGroupEngine -# -# 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 +# - 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 # # **Note:** This is currently an experimental feature which is not supported # in the service or as an integration point. 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 c86abc2d22b..e5207308961 100644 --- a/updater/lib/dependabot/updater/operations/group_update_all_versions.rb +++ b/updater/lib/dependabot/updater/operations/group_update_all_versions.rb @@ -42,11 +42,6 @@ def initialize(service:, job:, dependency_snapshot:, error_handler:) def perform dependency_snapshot.groups.each do |_group_hash, group| Dependabot.logger.info("[Experimental] Starting grouped update job for #{job.source.repo}") - # We should log the rule being executed, let's just hard-code wildcard for now - # since the prototype makes best-effort to do everything in one pass. - # This should be replaced with the actual Dependabot::DependencyGroup instances that have names - # dependency_snapshot.groups will need to be updated to return a list of - # Dependabot::DependencyGroup instances Dependabot.logger.info("Starting update group for '#{group.name}'") dependency_change = compile_all_dependency_changes_for(group) @@ -70,12 +65,12 @@ def perform end end - # Let's not worry about this for now, but eventually we'd try to do single updates + # FIXME: Let's not worry about this for now but eventually we'd try to do single updates # for anything that didn't appear in _at least one_ group. - dependency_snapshot.ungrouped_dependencies.each do |dependency| - # dependency_change = create_dependency_change_for(dependency) - # create_pull_request(dependency_change) - end + # dependency_snapshot.ungrouped_dependencies.each do |dependency| + # dependency_change = create_dependency_change_for(dependency) + # create_pull_request(dependency_change) + # end end # rubocop:enable Metrics/AbcSize From b95cf42cf1057f54398ebf00efd21c32338fbf17 Mon Sep 17 00:00:00 2001 From: Nish Sinha Date: Thu, 13 Apr 2023 16:16:44 -0400 Subject: [PATCH 15/25] Remove the unused #belongs_to_dependency_group? method This is an artifact from an earlier iteration that skipped over ungrouped_dependencies Co-authored-by: Barry Gordon <896971+brrygrdn@users.noreply.github.com> --- updater/lib/dependabot/job.rb | 6 ------ 1 file changed, 6 deletions(-) diff --git a/updater/lib/dependabot/job.rb b/updater/lib/dependabot/job.rb index 0e0eac78575..d8238a93b3c 100644 --- a/updater/lib/dependabot/job.rb +++ b/updater/lib/dependabot/job.rb @@ -244,12 +244,6 @@ def register_dependency_groups end end - def belongs_to_dependency_group?(dependency) - return false unless dependency_groups.any? - - Dependabot::DependencyGroupEngine.groups_for(dependency).any? - end - def ignore_conditions_for(dependency) update_config.ignored_versions_for( dependency, From bff02ecd24db5d2c2205a73116c7ed4a72a2dbb4 Mon Sep 17 00:00:00 2001 From: Nish Sinha Date: Thu, 13 Apr 2023 16:17:51 -0400 Subject: [PATCH 16/25] Use #each instead of #inject since we are adding to an instance variable, we can just use each here Co-authored-by: Barry Gordon <896971+brrygrdn@users.noreply.github.com> --- updater/lib/dependabot/dependency_group_engine.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/updater/lib/dependabot/dependency_group_engine.rb b/updater/lib/dependabot/dependency_group_engine.rb index 7f6b65d4a10..42523f315c2 100644 --- a/updater/lib/dependabot/dependency_group_engine.rb +++ b/updater/lib/dependabot/dependency_group_engine.rb @@ -63,7 +63,7 @@ def self.ungrouped_dependencies(dependencies) end def self.calculate_dependency_groups!(dependencies) - dependencies.inject(@dependency_groups) do |_dependency_groups, dependency| + dependencies.each do |dependency| groups = groups_for(dependency) @ungrouped_dependencies << dependency if groups.empty? From 83f0b59f6d37d7334c948c3d8425b5a658cb5764 Mon Sep 17 00:00:00 2001 From: Nish Sinha Date: Thu, 13 Apr 2023 16:34:40 -0400 Subject: [PATCH 17/25] Move WildcardMatcher to common --- {updater => common}/lib/wildcard_matcher.rb | 0 {updater => common}/spec/dependabot/wildcard_matcher_spec.rb | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename {updater => common}/lib/wildcard_matcher.rb (100%) rename {updater => common}/spec/dependabot/wildcard_matcher_spec.rb (100%) diff --git a/updater/lib/wildcard_matcher.rb b/common/lib/wildcard_matcher.rb similarity index 100% rename from updater/lib/wildcard_matcher.rb rename to common/lib/wildcard_matcher.rb diff --git a/updater/spec/dependabot/wildcard_matcher_spec.rb b/common/spec/dependabot/wildcard_matcher_spec.rb similarity index 100% rename from updater/spec/dependabot/wildcard_matcher_spec.rb rename to common/spec/dependabot/wildcard_matcher_spec.rb From 0b9675f9ff7800d8992c9743da59aba1c77379a8 Mon Sep 17 00:00:00 2001 From: Nish Sinha Date: Thu, 13 Apr 2023 16:35:45 -0400 Subject: [PATCH 18/25] Use the WildcardMatcher in common --- common/lib/dependabot/dependency_group.rb | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/common/lib/dependabot/dependency_group.rb b/common/lib/dependabot/dependency_group.rb index 1f0cbc26dc1..5a77ae8bcf9 100644 --- a/common/lib/dependabot/dependency_group.rb +++ b/common/lib/dependabot/dependency_group.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require 'wildcard_matcher' + module Dependabot class DependencyGroup attr_reader :name, :rules, :dependencies @@ -16,16 +18,3 @@ def contains?(dependency) end end end - -# Copied from updater/lib/wildcard_matcher.rb -class WildcardMatcher - def self.match?(wildcard_string, candidate_string) - return false unless wildcard_string && candidate_string - - regex_string = "a#{wildcard_string.downcase}a".split("*"). - map { |p| Regexp.quote(p) }. - join(".*").gsub(/^a|a$/, "") - regex = /^#{regex_string}$/ - regex.match?(candidate_string.downcase) - end -end From 9ab1df5a11c9a74d48d22e5b73eeb9df1c002f2f Mon Sep 17 00:00:00 2001 From: Nish Sinha Date: Thu, 13 Apr 2023 16:47:24 -0400 Subject: [PATCH 19/25] Fix how DependencyGroups are registered DependencyGroups have a name and rules, where a rule is a Hash that currently accepts one key, "patterns". Other keys may be supported in the future, but for now we can just pass in `group["rules]["patterns"]` directly --- updater/lib/dependabot/job.rb | 2 +- updater/spec/dependabot/dependency_change_builder_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/updater/lib/dependabot/job.rb b/updater/lib/dependabot/job.rb index d8238a93b3c..c707d797b64 100644 --- a/updater/lib/dependabot/job.rb +++ b/updater/lib/dependabot/job.rb @@ -240,7 +240,7 @@ def security_advisories_for(dependency) def register_dependency_groups dependency_groups.each do |group| - Dependabot::DependencyGroupEngine.register(group["name"], group["rules"]) + Dependabot::DependencyGroupEngine.register(group["name"], group["rules"]["patterns"]) end end diff --git a/updater/spec/dependabot/dependency_change_builder_spec.rb b/updater/spec/dependabot/dependency_change_builder_spec.rb index 19ea8e012ab..8e0d4acea01 100644 --- a/updater/spec/dependabot/dependency_change_builder_spec.rb +++ b/updater/spec/dependabot/dependency_change_builder_spec.rb @@ -107,7 +107,7 @@ context "when the source is a dependency group" do let(:change_source) do - Dependabot::DependencyGroup.new(name: "dummy-pkg-*", rules: ["dummy-pkg-*"]) + Dependabot::DependencyGroup.new(name: "dummy-pkg-*", rules: { "patterns" => ["dummy-pkg-*"] }) end it "creates a new DependencyChange flagged as a grouped update" do From 109b1c57c25a181001a5a0520b400c65b8308d77 Mon Sep 17 00:00:00 2001 From: Nish Sinha Date: Thu, 13 Apr 2023 17:46:50 -0400 Subject: [PATCH 20/25] Update Job fixture to use the correct dependency groups format --- .../spec/fixtures/jobs/job_with_dependency_groups.json | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/updater/spec/fixtures/jobs/job_with_dependency_groups.json b/updater/spec/fixtures/jobs/job_with_dependency_groups.json index e1b5660569e..299b4088db9 100644 --- a/updater/spec/fixtures/jobs/job_with_dependency_groups.json +++ b/updater/spec/fixtures/jobs/job_with_dependency_groups.json @@ -36,11 +36,15 @@ "dependency-groups": [ { "name": "group-a", - "rules": ["dummy-pkg-*"] + "rules": { + "patterns": ["dummy-pkg-*"] + } }, { "name": "group-b", - "rules": ["dummy-pkg-b"] + "rules": { + "patterns": ["dummy-pkg-b"] + } } ], "experiments": { "grouped_updates_prototype": true } From 99a5c63423f5c15282a2414554538958b829bb2c Mon Sep 17 00:00:00 2001 From: Nish Sinha Date: Mon, 17 Apr 2023 03:14:53 -0400 Subject: [PATCH 21/25] Use named parameters for DependencyGroup --- common/lib/dependabot/dependency_group.rb | 6 ++--- .../spec/dependabot/dependency_group_spec.rb | 26 +++++++++---------- .../lib/dependabot/dependency_group_engine.rb | 4 +-- .../dependency_change_builder_spec.rb | 5 +++- updater/spec/dependabot/job_spec.rb | 9 +++++-- 5 files changed, 29 insertions(+), 21 deletions(-) diff --git a/common/lib/dependabot/dependency_group.rb b/common/lib/dependabot/dependency_group.rb index 5a77ae8bcf9..3233238020a 100644 --- a/common/lib/dependabot/dependency_group.rb +++ b/common/lib/dependabot/dependency_group.rb @@ -1,14 +1,14 @@ # frozen_string_literal: true -require 'wildcard_matcher' +require "wildcard_matcher" module Dependabot class DependencyGroup attr_reader :name, :rules, :dependencies - def initialize(name, rule) + def initialize(name:, rules:) @name = name - @rules = rule + @rules = rules @dependencies = [] end diff --git a/common/spec/dependabot/dependency_group_spec.rb b/common/spec/dependabot/dependency_group_spec.rb index 402f61d9250..e758e1e44b5 100644 --- a/common/spec/dependabot/dependency_group_spec.rb +++ b/common/spec/dependabot/dependency_group_spec.rb @@ -6,11 +6,11 @@ # TODO: Once the Updater has been merged into Core, we should test this # using the DependencyGroupEngine methods instead of mocking the functionality RSpec.describe Dependabot::DependencyGroup do - let(:dependency_group) { described_class.new(name, rules) } + let(:dependency_group) { described_class.new(name: name, rules: rules) } let(:name) { "test_group" } let(:rules) { ["test-*"] } - let(:test_dependency_1) do + let(:test_dependency1) do Dependabot::Dependency.new( name: "test-dependency-1", package_manager: "bundler", @@ -26,7 +26,7 @@ ) end - let(:test_dependency_2) do + let(:test_dependency2) do Dependabot::Dependency.new( name: "another-test-dependency", package_manager: "bundler", @@ -63,12 +63,12 @@ context "when dependencies have been assigned" do before do - dependency_group.dependencies << test_dependency_1 + dependency_group.dependencies << test_dependency1 end it "returns the dependencies" do - expect(dependency_group.dependencies).to include(test_dependency_1) - expect(dependency_group.dependencies).not_to include(test_dependency_2) + expect(dependency_group.dependencies).to include(test_dependency1) + expect(dependency_group.dependencies).not_to include(test_dependency2) end end end @@ -77,28 +77,28 @@ 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_dependency_1)).to be_truthy + expect(dependency_group.contains?(test_dependency1)).to be_truthy end it "returns false if the dependency does not match a rule" do expect(dependency_group.dependencies).to eq([]) - expect(dependency_group.contains?(test_dependency_2)).to be_falsey + expect(dependency_group.contains?(test_dependency2)).to be_falsey end end context "after dependencies are assigned to the group" do before do - dependency_group.dependencies << test_dependency_1 + 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_dependency_1) - expect(dependency_group.contains?(test_dependency_1)).to be_truthy + 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 not in the dependency list and does not match a rule" do - expect(dependency_group.dependencies).to include(test_dependency_1) - expect(dependency_group.contains?(test_dependency_2)).to be_falsey + expect(dependency_group.dependencies).to include(test_dependency1) + expect(dependency_group.contains?(test_dependency2)).to be_falsey end end end diff --git a/updater/lib/dependabot/dependency_group_engine.rb b/updater/lib/dependabot/dependency_group_engine.rb index 42523f315c2..d58968792e4 100644 --- a/updater/lib/dependabot/dependency_group_engine.rb +++ b/updater/lib/dependabot/dependency_group_engine.rb @@ -31,8 +31,8 @@ def self.reset! end # Eventually the key for a dependency group should be a hash since names _can_ conflict within jobs - def self.register(name, rule) - @registered_groups.push Dependabot::DependencyGroup.new(name, rule) + def self.register(name, rules) + @registered_groups.push Dependabot::DependencyGroup.new(name: name, rules: rules) end def self.groups_for(dependency) diff --git a/updater/spec/dependabot/dependency_change_builder_spec.rb b/updater/spec/dependabot/dependency_change_builder_spec.rb index 8e0d4acea01..2c5779c7fd7 100644 --- a/updater/spec/dependabot/dependency_change_builder_spec.rb +++ b/updater/spec/dependabot/dependency_change_builder_spec.rb @@ -107,7 +107,10 @@ context "when the source is a dependency group" do let(:change_source) do - Dependabot::DependencyGroup.new(name: "dummy-pkg-*", rules: { "patterns" => ["dummy-pkg-*"] }) + # FIXME: rules are actually a hash but for the purposes of this pass we can leave it as a list + # Once this is refactored we should create a DependencyGroup like so + # Dependabot::DependencyGroup.new(name: "dummy-pkg-*", rules: { "patterns" => ["dummy-pkg-*"] }) + Dependabot::DependencyGroup.new(name: "dummy-pkg-*", rules: ["dummy-pkg-*"]) end it "creates a new DependencyChange flagged as a grouped update" do diff --git a/updater/spec/dependabot/job_spec.rb b/updater/spec/dependabot/job_spec.rb index f64df4ae1c7..61236ba19d4 100644 --- a/updater/spec/dependabot/job_spec.rb +++ b/updater/spec/dependabot/job_spec.rb @@ -63,7 +63,12 @@ let(:experiments) { nil } let(:commit_message_options) { nil } let(:vendor_dependencies) { false } - let(:dependency_groups) { [] } + # FIXME: rules are actually a hash but for the purposes of this pass we can leave it as a list + # Once this is refactored we should create a DependencyGroup like so + # Dependabot::DependencyGroup.new(name: "dummy-pkg-*", rules: { "patterns" => ["dummy-pkg-*"] }) + let(:dependency_groups) do + [{ "name" => "dummy-pkg-*", "rules" => { "patterns" => ["dummy-pkg-*"] } }] + end describe "::new_update_job" do let(:job_json) { fixture("jobs/job_with_credentials.json") } @@ -89,7 +94,7 @@ end it "will register its dependency groups" do - expect(:job).to receive(:register_dependency_groups).with(dependency_groups) + expect_any_instance_of(described_class).to receive(:register_dependency_groups) new_update_job end end From 4bc4163b12473bec9f906585a8c2e775a8959dae Mon Sep 17 00:00:00 2001 From: Nish Sinha Date: Mon, 17 Apr 2023 03:15:29 -0400 Subject: [PATCH 22/25] Preserve default group-all behavior --- updater/lib/dependabot/job.rb | 2 ++ .../updater/operations/group_update_all_versions.rb | 12 ++++++++++-- .../spec/dependabot/dependency_group_engine_spec.rb | 3 +-- 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/updater/lib/dependabot/job.rb b/updater/lib/dependabot/job.rb index c707d797b64..ae3d4c0ba54 100644 --- a/updater/lib/dependabot/job.rb +++ b/updater/lib/dependabot/job.rb @@ -239,6 +239,8 @@ def security_advisories_for(dependency) end def register_dependency_groups + return if dependency_groups.empty? + dependency_groups.each do |group| Dependabot::DependencyGroupEngine.register(group["name"], group["rules"]["patterns"]) 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 e5207308961..36d0ead636f 100644 --- a/updater/lib/dependabot/updater/operations/group_update_all_versions.rb +++ b/updater/lib/dependabot/updater/operations/group_update_all_versions.rb @@ -21,7 +21,6 @@ def self.applies_to?(job:) return false if job.security_updates_only? return false if job.updating_a_pull_request? return false if job.dependencies&.any? - return false if job.dependency_groups.empty? Dependabot::Experiments.enabled?(:grouped_updates_prototype) end @@ -35,11 +34,14 @@ def initialize(service:, job:, dependency_snapshot:, error_handler:) @job = job @dependency_snapshot = dependency_snapshot @error_handler = error_handler - @dependency_group = dependency_snapshot.dependency_groups 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 if job.dependency_groups.empty? + dependency_snapshot.groups.each do |_group_hash, group| Dependabot.logger.info("[Experimental] Starting grouped update job for #{job.source.repo}") Dependabot.logger.info("Starting update group for '#{group.name}'") @@ -90,6 +92,12 @@ def dependencies :dependency_snapshot, :error_handler + def register_all_dependencies_group + all_dependencies_group = { "name" => "group-all", "rules" => { "patterns" => ["*"] } } + Dependabot::DependencyGroupEngine.register(all_dependencies_group["name"], + all_dependencies_group["rules"]["patterns"]) + end + # Returns a Dependabot::DependencyChange object that encapsulates the # outcome of attempting to update every dependency iteratively which # can be used for PR creation. diff --git a/updater/spec/dependabot/dependency_group_engine_spec.rb b/updater/spec/dependabot/dependency_group_engine_spec.rb index a8cdff8a670..ef414d2a838 100644 --- a/updater/spec/dependabot/dependency_group_engine_spec.rb +++ b/updater/spec/dependabot/dependency_group_engine_spec.rb @@ -1,6 +1,5 @@ # frozen_string_literal: true -require "debug" require "spec_helper" require "dependabot/dependency_group_engine" require "dependabot/dependency_snapshot" @@ -124,7 +123,7 @@ def create_dependency_snapshot expect(groups.key?(:"group-a")).to be_truthy expect(groups.key?(:"group-b")).to be_truthy - expect(groups[:"group-a"].first).to be_a(Dependabot::Dependency) + expect(groups[:"group-a"]).to be_a(Dependabot::DependencyGroup) end it "does not call calculate_dependency_groups! again after groups are initially calculated" do From 8c55b80a8b9560fb6a108a5e365fbc0138cf110b Mon Sep 17 00:00:00 2001 From: Nish Sinha Date: Mon, 17 Apr 2023 10:47:35 -0400 Subject: [PATCH 23/25] Fix flakey test and smoke tests --- updater/lib/dependabot/job.rb | 2 +- updater/spec/dependabot/updater/operations_spec.rb | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/updater/lib/dependabot/job.rb b/updater/lib/dependabot/job.rb index ae3d4c0ba54..5f5c609be1e 100644 --- a/updater/lib/dependabot/job.rb +++ b/updater/lib/dependabot/job.rb @@ -239,7 +239,7 @@ def security_advisories_for(dependency) end def register_dependency_groups - return if dependency_groups.empty? + return if dependency_groups.nil? dependency_groups.each do |group| Dependabot::DependencyGroupEngine.register(group["name"], group["rules"]["patterns"]) diff --git a/updater/spec/dependabot/updater/operations_spec.rb b/updater/spec/dependabot/updater/operations_spec.rb index f2eea617d98..601f4a0bfb9 100644 --- a/updater/spec/dependabot/updater/operations_spec.rb +++ b/updater/spec/dependabot/updater/operations_spec.rb @@ -7,6 +7,10 @@ RSpec.describe Dependabot::Updater::Operations do describe "::class_for" do + before do + Dependabot::Experiments.reset! + end + it "returns nil if no operation matches" do # We always expect jobs that update a pull request to specify their # existing dependency changes, a job with this set of conditions From 62d47bc276e03fcfc55d4d29bb14efdc4c1fe78c Mon Sep 17 00:00:00 2001 From: Nish Sinha Date: Mon, 17 Apr 2023 13:40:35 -0400 Subject: [PATCH 24/25] register all dependencies group unless any dependency groups exist Co-authored-by: Barry Gordon <896971+brrygrdn@users.noreply.github.com> --- .../dependabot/updater/operations/group_update_all_versions.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 36d0ead636f..fefd885de10 100644 --- a/updater/lib/dependabot/updater/operations/group_update_all_versions.rb +++ b/updater/lib/dependabot/updater/operations/group_update_all_versions.rb @@ -40,7 +40,7 @@ def initialize(service:, job:, dependency_snapshot:, error_handler:) 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 if job.dependency_groups.empty? + register_all_dependencies_group unless job.dependency_groups&.any? dependency_snapshot.groups.each do |_group_hash, group| Dependabot.logger.info("[Experimental] Starting grouped update job for #{job.source.repo}") From 9ac2f7e0a4c982187c23b2a0c6d0c7026d4df674 Mon Sep 17 00:00:00 2001 From: Nish Sinha Date: Tue, 18 Apr 2023 10:30:31 -0400 Subject: [PATCH 25/25] Don't register any dependency groups in the job spec --- updater/spec/dependabot/job_spec.rb | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/updater/spec/dependabot/job_spec.rb b/updater/spec/dependabot/job_spec.rb index 61236ba19d4..92118cdfb4e 100644 --- a/updater/spec/dependabot/job_spec.rb +++ b/updater/spec/dependabot/job_spec.rb @@ -63,12 +63,7 @@ let(:experiments) { nil } let(:commit_message_options) { nil } let(:vendor_dependencies) { false } - # FIXME: rules are actually a hash but for the purposes of this pass we can leave it as a list - # Once this is refactored we should create a DependencyGroup like so - # Dependabot::DependencyGroup.new(name: "dummy-pkg-*", rules: { "patterns" => ["dummy-pkg-*"] }) - let(:dependency_groups) do - [{ "name" => "dummy-pkg-*", "rules" => { "patterns" => ["dummy-pkg-*"] } }] - end + let(:dependency_groups) { [] } describe "::new_update_job" do let(:job_json) { fixture("jobs/job_with_credentials.json") }