-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Run Dependency Group updates #7075
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
ff792bc
e8a2b4b
ccafaa6
740a469
e9f8b38
f35246a
45dd99b
60b3b24
f098677
d4023a8
ed11117
908c52e
64b9f95
62319d6
a8aa686
b95cf42
bff02ec
83f0b59
0b9675f
9ab1df5
109b1c5
99a5c63
4bc4163
4e357b6
8c55b80
62d47bc
0bcc0f7
9ac2f7e
71ab200
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,20 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require "wildcard_matcher" | ||
|
|
||
| module Dependabot | ||
| class DependencyGroup | ||
| attr_reader :name, :rules | ||
| attr_reader :name, :rules, :dependencies | ||
|
|
||
| def initialize(name:, rules:) | ||
| @name = name | ||
| @rules = rules | ||
| @dependencies = [] | ||
| end | ||
|
|
||
| def contains?(dependency) | ||
| @dependencies.include?(dependency) if @dependencies.any? | ||
| rules.any? { |rule| WildcardMatcher.match?(rule, dependency.name) } | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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: name, rules: rules) } | ||
| let(:name) { "test_group" } | ||
| let(:rules) { ["test-*"] } | ||
|
|
||
| let(:test_dependency1) 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_dependency2) 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(name: my_dependency_group_name, rules: anything) | ||
| 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 no dependencies are assigned to the group" do | ||
| it "returns an empty list" do | ||
| expect(dependency_group.dependencies).to eq([]) | ||
| end | ||
| end | ||
|
|
||
| context "when dependencies have been assigned" do | ||
| before do | ||
| dependency_group.dependencies << test_dependency1 | ||
| end | ||
|
|
||
| it "returns the dependencies" do | ||
| expect(dependency_group.dependencies).to include(test_dependency1) | ||
| expect(dependency_group.dependencies).not_to include(test_dependency2) | ||
| end | ||
| end | ||
| 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 | ||
| 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 "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 | ||
|
|
||
| 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_dependency1) | ||
| expect(dependency_group.contains?(test_dependency2)).to be_falsey | ||
| end | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,80 @@ | ||||||
| # 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. | ||||||
| # | ||||||
| # 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 | ||||||
| # | ||||||
| # **Note:** This is currently an experimental feature which is not supported | ||||||
| # in the service or as an integration point. | ||||||
| # | ||||||
| module Dependabot | ||||||
| module DependencyGroupEngine | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I think it's a little unusual to have a I think as a starting point it is fine to just make this a class and then prevent it being instantiated via: private
def initialize
endThat said, I do wonder if it might be easier to just simplify this class into something we instantiate in That would make the affordances and testing a little more idiomatic and it would have the benefit that if the group rule parsing blew up for any reason, it would happen at the point the snapshot is being set up, like parsing, rather than lazily once we start into the operation class. That would keep any group-related error handling ( i.e. invalid group names, etc ) out of the way of the actual update operation which would keep things nice and clear.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That said, I think we can leave this as-is since it is working for this PR and refactor it in a follow-up, overall how this works is nice and clear 👍🏻
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks I'll take a look at this in a follow up PR |
||||||
| @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, rules) | ||||||
| @registered_groups.push Dependabot::DependencyGroup.new(name: name, rules: rules) | ||||||
| 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.each do |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 | ||||||
Nishnha marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| end | ||||||
| end | ||||||
Uh oh!
There was an error while loading. Please reload this page.