-
Notifications
You must be signed in to change notification settings - Fork 937
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
Run Dependency Group updates #7075
Conversation
Add the #groups and #upgrouped_dependencies methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! I really like the split out responsibilities between DependencyGroup
and the DependencyGroupEngine
.
I have a few minor notes, but my longer thought on making the DependencyGroupEngine
an instance rather than a 'singleton-like' class is probably worth doing separately. Ruby class variables have a lot of gotchas so while they work it's generally not the preferred idiom and I think we get a side benefit that when we start to do more rule validation we can throw any parsing errors at a nice and clear point in the code later.
updater/lib/dependabot/updater/operations/group_update_all_versions.rb
Outdated
Show resolved
Hide resolved
updater/lib/dependabot/updater/operations/group_update_all_versions.rb
Outdated
Show resolved
Hide resolved
6754830
to
7557850
Compare
This PR is in kind of a weird state because I had it stacked on top of https://github.com/dependabot/dependabot-core/pull/7074/commits which I then squashed into main. The merge commit brings this branch back up to date with So those 2 commits are kind of unnecessary? If you diff them with main they look (mostly) empty. Anyhow, I'm going to continue rolling forward with commits on this branch. If we want to have a cleaner history I'd be happy to re-open this branch as a new PR but I don't want to rebase and lose @brrygrdn's comments here. |
Co-authored-by: Barry Gordon <896971+brrygrdn@users.noreply.github.com>
This is an artifact from an earlier iteration that skipped over ungrouped_dependencies Co-authored-by: Barry Gordon <896971+brrygrdn@users.noreply.github.com>
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>
Based on some of the comments from @brrygrdn, after this PR lands I'm going to follow up with a refactor of the DependencyGroupEngine with the goals being:
|
8d98717
to
0b9675f
Compare
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/updater/operations/group_update_all_versions.rb
Outdated
Show resolved
Hide resolved
Co-authored-by: Barry Gordon <896971+brrygrdn@users.noreply.github.com>
I see this test failure in the updater spec but I can't recreate it locally Failures:
1) Dependabot::Updater#run with the grouped experiment enabled updates multiple dependencies in a single PR correctly
Failure/Error:
expect(service).to receive(:create_pull_request) do |dependency_change, base_commit_sha|
expect(dependency_change.updated_dependencies.first).to have_attributes(name: "dummy-pkg-b")
expect(dependency_change.updated_dependency_files_hash).to eql(
[
{
"name" => "Gemfile",
"content" => fixture("bundler/updated/Gemfile"),
"directory" => "/",
"type" => "file",
"mode" => "100644",
(#<Dependabot::Service:0x00007ff524fb8d28 @client=#<InstanceDouble(Dependabot::ApiClient) (anonymous)>, @pull_requests=[], @errors=[]>).create_pull_request(*(any args))
expected: 1 time with any arguments
received: 2 times with any arguments
# ./spec/dependabot/updater_spec.rb:2206:in `block (3 levels) in <top (required)>'
# ./vendor/ruby/3.1.0/gems/webmock-3.18.1/lib/webmock/rspec.rb:37:in `block (2 levels) in <top (required)>' I was thinking of changing the test to be |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏻
# 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-*"] }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏻
Yeah, that's odd - it seems like it should only be called once. It might be worth just adding a I'm thinking that due to the |
Looks like it was a leaky dependency group! I fixed this in 9ac2f7e by no longer registering a DependencyGroup for the job spec. |
015f0cf
to
9ac2f7e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, let's 🚀
7e241c2
to
9ac2f7e
Compare
This PR is stacked on top of #7074
I have tried to make each commit represent a single file change for this PR.
Things to note:
The main contribution of this PR is that it introduces a new class, the
DependencyGroupEngine
, which registersDependencyGroups
based on values passed in from an Update Job.The engine is not called directly, but dependency groups are made available to the Updater through 2 methods:
DependencySnapshot#groups
DependencySnapshot#ungrouped_dependencies
Here is what the job summary for 2 dependency groups looks like:
This implementation will only run dependencies that belong to a dependency group. It does not try to generate any PRs for ungrouped dependencies.