[Updater] Implement an Operation capable of refreshing a Grouped Update PR. #7192

brrygrdn · 2023-04-28T13:29:11Z

"Refreshing" a Dependabot pull request essentially has three distinct outcomes, we recompute the DependencyChange on the current head of the target branch and:

determine that all the same dependencies change to the same versions as the existing PR; in this case we update the existing PR
determine that one or more dependencies are now involved or removed; in this case we close the existing PR and create a new one
determine that all the dependencies are the same, but one or more are now updating to a different version; in this case we create a new PR and leave it to the backend service to detect the new PR supersedes and existing one

This strategy is consistent with our normal single-dependency PRs with one exception; instead of looking at all existing PRs to determine whether it is updating/replacing/superseding it only considers PRs for the group rule that job is targeting.

Example output

updater | +---------------------------------------------------------------------------------------------------------------------------------------------------------+
updater | |                                                           Changes to Dependabot Pull Requests                                                           |
updater | +------------------------------+--------------------------------------------------------------------------------------------------------------------------+
updater | | closed: dependencies_changed | com.bugsnag:bugsnag,com.google.inject:guice,io.github.lukehutch:fast-classpath-scanner,net.kyori:text,net.lingala.zip... |
updater | | created                      | com.google.inject:guice ( from 4.2.0 to 5.1.0 ), net.lingala.zip4j:zip4j ( from 1.3.2 to 2.11.5 ), net.kyori:text ( f... |
updater | +------------------------------+--------------------------------------------------------------------------------------------------------------------------+

Significant changes

This PR introduces two new attributes to the "job definition"

existing_group_pull_requests: this serialises any grouped PRs separately to 'normal' PRs so we don't have unexpected interactions between single- and group-updates
- In future, we may merge this into the existing_pull_requests key for simplicity, but it would be a breaking change so we should do it incrementally.
dependency-group-to-refresh: the name of the dependency group to be refreshed
- As part of this process, we confirm that a group by this name still exists in the configuration for the repository, failing if not

brrygrdn · 2023-04-28T13:31:46Z

updater/lib/dependabot/dependency_change.rb

+    # This method combines checking the job's `updating_a_pull_request` flag
+    # with verification the dependencies involved remain the same.
+    #
+    # If the dependencies involved have changed, we should close the old PR
+    # rather than supersede it as the new changes don't necessarily follow
+    # from the previous ones; dependencies could have been removed from the
+    # project, or pinned by other changes.
+    def should_replace_existing_pr?
+      return false unless job.updating_a_pull_request?
+
+      # NOTE: Gradle, Maven and Nuget dependency names can be case-insensitive
+      # and the dependency name injected from a security advisory often doesn't
+      # match what users have specified in their manifest.
+      updated_dependencies.map(&:name).map(&:downcase) != job.dependencies.map(&:downcase)
+    end
+
+    def matches_existing_pr?
+      !!existing_pull_request
+    end


I didn't do it as part of this PR as we don't really have sufficient test coverage to be confident in making the change, but these methods should be used in preference in our other Refresh* jobs in future to DRY them out further.

brrygrdn · 2023-04-28T13:33:15Z

updater/lib/dependabot/dependency_snapshot.rb

+    def job_group_name
+      job.dependency_group_to_refresh
+    end


My thinking is that in future we might have a dependency_group_to_create key to allow us to just build a single group if we want to change the workload profile of group/version updates.

For now it might make sense to remove this indirection but I wanted to shorten the reference in a couple of places

brrygrdn · 2023-04-28T13:43:18Z

updater/lib/dependabot/updater/group_update_creation.rb

+#
+module Dependabot
+  class Updater
+    module GroupUpdateCreation


I considered two alternative approaches:

Let's make a GroupUpdateOperation base class!

Let's create a new GroupUpdaterBuilder object we instantiate and call!

I ended up 👎🏻 on both of those as I'd rather not add inherence into the Updater given the problems we've had with branching code and I wasn't totally sold on the value of a GroupUpdaterBuilder object as a separate component we might want to add tests on specifically.

I ended up defaulting to a coarse composition approach with a view that we can revisit this later as there's still some DRYing out and test improvement to be done on the other Operations and the right pattern is likely to become clearer with more examples.

brrygrdn · 2023-04-28T13:52:55Z

PR to make the CLI compatible: dependabot/cli#105

updater/lib/dependabot/updater/group_update_creation.rb

Nishnha · 2023-05-01T14:52:33Z

updater/lib/dependabot/dependency_change.rb

+      # NOTE: Gradle, Maven and Nuget dependency names can be case-insensitive
+      # and the dependency name injected from a security advisory often doesn't
+      # match what users have specified in their manifest.
+      updated_dependencies.map(&:name).map(&:downcase) != job.dependencies.map(&:downcase)


job.dependencies is a list of Dependabot::Dependency objects, so I think you would also need to map across the job dependency names

This caught me out too, dependency_snapshot.job_dependencies are hydrated Dependabot::Dependency objects, but job.dependencies is just the raw array from the job definition file.

We should probably do a naming cleanup pass on this to make that more obvious as I mocked tests incorrectly with objects as I had the same read as you and then had a surprise when I tried it with the CLI.

Nishnha · 2023-05-01T15:25:58Z

updater/spec/fixtures/job_definitions/bundler/version_updates/group_update_refresh.yaml

+    rules:
+      patterns:
+        - "*"
+  dependency-group-to-refresh: everything-everywhere-all-at-once


Nishnha · 2023-05-01T16:53:44Z

updater/lib/dependabot/dependency_change.rb

+      else
+        job.existing_pull_requests.find { |pr| Set.new(pr) == updated_dependencies_set }
+      end


We may want to break out the "existing grouped update PR?" check into its own method that we can call in #upsert_pull_request

Users could define a group that has a single dependency and matches an existing ungrouped PR, causing dependency_change.matches_existing_pr? to return true.
If that happens, then we would close the existing ungrouped dependency PR in favor of a grouped one (which could be okay but confusing from a user perspective?)

If we introduce a #matches_existing_grouped_pr? method then we can make sure we only upsert grouped updates during this operation

Users could define a group that has a single dependency and matches an existing ungrouped PR, causing dependency_change.matches_existing_pr? to return true.

This shouldn't happen at present as grouped_updated? on line 73 would be true for the grouped update meaning it wouldn't check the existing_pull_requests set for any matches, so it should keep both job types in their own lane.

We might want to consider doing a one-time check between strategies when users change their config to add a group ( i.e. close any old single PRs that match the new group ), but I think we can leave that for now and allow it to be managed manually so it's ok not to do any cross-checking and do a hard fork on grouped_updated? being true/false for the current job.

Nishnha · 2023-05-01T17:23:43Z

updater/spec/dependabot/updater/operations/refresh_group_update_pull_request_spec.rb

+
+    it "closes the pull request" do
+      expect(mock_error_handler).not_to receive(:handle_dependabot_error)
+      expect(mock_service).to receive(:close_pull_request).with(["dummy-pkg-b"], :up_to_date)


I doubled checked this and dummy-pkg-b is the only dependency in the "updated" gemfiles, but when using the "original" gemfiles dummy-pkg-c also updates as a transitive dependency 👍🏾

https://github.com/dependabot/dependabot-core/blob/ded459a52682161823aa9b94904790fb156f6af9/updater/spec/fixtures/bundler2/updated/Gemfile

updater/spec/dependabot/updater/operations/refresh_group_update_pull_request_spec.rb

Nishnha

In general this looks great! 🌟

I did leave a few comments, but other than jobs.dependencies being a list of Dependabot::Dependency objects, I don't think there is anything blocking.

I would be okay with this merging as-is and then addressing the comments in a follow-up PR

…a mixin

jeffwidman · 2023-05-02T16:22:30Z

In future, we may merge this into the existing_pull_requests key for simplicity, but it would be a breaking change so we should do it incrementally.

I have zero context on this PR as I haven't closely followed the changes you guys are working on... but I saw this comment and wanted to point out that for any breaking changes in the updater, no one uses that beyond us. And also for grouped updates, they're still alpha/beta... breaking isn't the end of the world. Only if this breaks normal "single" pull requests would this really matter IMO...

…pdates-can-refresh [Updater] Implement an Operation capable of refreshing a Grouped Update PR.

brrygrdn requested a review from a team as a code owner April 28, 2023 13:29

brrygrdn commented Apr 28, 2023

View reviewed changes

brrygrdn force-pushed the brrygrdn/grouped-updates-can-refresh branch from acfc5a2 to 9faebe9 Compare April 28, 2023 13:48

brrygrdn mentioned this pull request Apr 28, 2023

Add DependencyGroupToRefresh and ExistingGroupPullRequests to the Job model dependabot/cli#105

Merged

brrygrdn commented Apr 28, 2023

View reviewed changes

updater/lib/dependabot/updater/group_update_creation.rb Show resolved Hide resolved

Nishnha reviewed May 1, 2023

View reviewed changes

updater/spec/dependabot/updater/operations/refresh_group_update_pull_request_spec.rb Outdated Show resolved Hide resolved

Nishnha approved these changes May 1, 2023

View reviewed changes

brrygrdn added 15 commits May 2, 2023 15:00

Remove some limitations that aren't true anymore

1cc1fd9

Pass the actual group name instead of a placeholder on error

0b41385

A Dependabot::Job can specify a dependency group to refresh

a4664b7

Introduce an operation class to handle rebasing grouped updates

c360319

Extract everything needed to build an update for a single group into …

ae036e8

…a mixin

Remove the unused dependencies method in GroupUpdateAllVersions

d95c132

Add happy path for updating a PR

541d15c

Handle cases where the project is now up to date

cd45c74

Update fixtures to include existing-group-pull-request

9c2a101

Wire up recreation and superseding

81bbcb7

Avoid hijacking another group's PR

c71a38b

Tidy up the new operation class a little

76d6bbe

Let's just pass the name of the group to refresh

05fcae5

Flatten depndency-group-name in existing-group-pull-requests

0d9cc45

Add a tag name to the new class, Improve closed PR output

1366d67

brrygrdn force-pushed the brrygrdn/grouped-updates-can-refresh branch from 9faebe9 to 1366d67 Compare May 2, 2023 14:05

Remove a test we no longer need

10a2242

brrygrdn merged commit 656b8dc into main May 2, 2023

brrygrdn deleted the brrygrdn/grouped-updates-can-refresh branch May 2, 2023 15:05

brettfo pushed a commit to brettfo/dependabot-core that referenced this pull request Oct 11, 2023

Merge pull request dependabot#7192 from dependabot/brrygrdn/grouped-u…

54221a7

…pdates-can-refresh [Updater] Implement an Operation capable of refreshing a Grouped Update PR.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Updater] Implement an Operation capable of refreshing a Grouped Update PR. #7192

[Updater] Implement an Operation capable of refreshing a Grouped Update PR. #7192

brrygrdn commented Apr 28, 2023 •

edited

Loading

brrygrdn Apr 28, 2023

brrygrdn Apr 28, 2023

brrygrdn Apr 28, 2023 •

edited

Loading

brrygrdn commented Apr 28, 2023

Nishnha May 1, 2023 •

edited

Loading

brrygrdn May 2, 2023 •

edited

Loading

Nishnha May 1, 2023

Nishnha May 1, 2023 •

edited

Loading

brrygrdn May 2, 2023

Nishnha May 1, 2023

Nishnha left a comment

jeffwidman commented May 2, 2023 •

edited

Loading

[Updater] Implement an Operation capable of refreshing a Grouped Update PR. #7192

[Updater] Implement an Operation capable of refreshing a Grouped Update PR. #7192

Conversation

brrygrdn commented Apr 28, 2023 • edited Loading

Example output

Significant changes

brrygrdn Apr 28, 2023

Choose a reason for hiding this comment

brrygrdn Apr 28, 2023

Choose a reason for hiding this comment

brrygrdn Apr 28, 2023 • edited Loading

Choose a reason for hiding this comment

brrygrdn commented Apr 28, 2023

Nishnha May 1, 2023 • edited Loading

Choose a reason for hiding this comment

brrygrdn May 2, 2023 • edited Loading

Choose a reason for hiding this comment

Nishnha May 1, 2023

Choose a reason for hiding this comment

Nishnha May 1, 2023 • edited Loading

Choose a reason for hiding this comment

brrygrdn May 2, 2023

Choose a reason for hiding this comment

Nishnha May 1, 2023

Choose a reason for hiding this comment

Nishnha left a comment

Choose a reason for hiding this comment

jeffwidman commented May 2, 2023 • edited Loading

brrygrdn commented Apr 28, 2023 •

edited

Loading

brrygrdn Apr 28, 2023 •

edited

Loading

Nishnha May 1, 2023 •

edited

Loading

brrygrdn May 2, 2023 •

edited

Loading

Nishnha May 1, 2023 •

edited

Loading

jeffwidman commented May 2, 2023 •

edited

Loading