Skip to content

Commit

Permalink
Do not close a PR when a dependency is superseded
Browse files Browse the repository at this point in the history
  • Loading branch information
judocode committed Jan 23, 2025
1 parent 02cd494 commit 527905f
Show file tree
Hide file tree
Showing 9 changed files with 210 additions and 35 deletions.
33 changes: 21 additions & 12 deletions updater/lib/dependabot/dependency_snapshot.rb
Original file line number Diff line number Diff line change
Expand Up @@ -119,18 +119,23 @@ def job_group
@dependency_group_engine.find_group(name: T.must(job.dependency_group_to_refresh))
end

sig { params(group: Dependabot::DependencyGroup).void }
def mark_group_handled(group)
sig { params(group: Dependabot::DependencyGroup, excluding_dependencies: T::Set[String]).void }
def mark_group_handled(group, excluding_dependencies = Set.new)
Dependabot.logger.info("Marking group '#{group.name}' as handled.")

directories.each do |directory|
@current_directory = directory

# add the existing dependencies in the group so individual updates don't try to update them
add_handled_dependencies(dependencies_in_existing_pr_for_group(group).filter_map { |d| d["dependency-name"] })
dependencies_in_existing_prs = dependencies_in_existing_pr_for_group(group)

# also add dependencies that might be in the group, as a rebase would add them;
# this avoids individual PR creation that immediately is superseded by a group PR supersede
add_handled_dependencies(group.dependencies.map(&:name))
# this avoids individual PR creation that immediately is superseded by a group PR supersede.
current_dependencies = group.dependencies.map(&:name).reject do |dep|
excluding_dependencies.include?(dep)
end

add_handled_dependencies(current_dependencies.concat(dependencies_in_existing_prs))
end
end

Expand Down Expand Up @@ -169,6 +174,17 @@ def ungrouped_dependencies
allowed_dependencies.reject { |dep| handled_dependencies.include?(dep.name) }
end

sig { params(group: Dependabot::DependencyGroup).returns(T::Array[String]) }
def dependencies_in_existing_pr_for_group(group)
existing = job.existing_group_pull_requests.find do |pr|
pr["dependency-group-name"] == group.name
end&.fetch("dependencies", []) || []

existing.filter_map do |dep|
dep["dependency-name"]
end
end

private

sig do
Expand Down Expand Up @@ -277,13 +293,6 @@ def dependency_file_parser
parser
end

sig { params(group: Dependabot::DependencyGroup).returns(T::Array[T::Hash[String, String]]) }
def dependencies_in_existing_pr_for_group(group)
job.existing_group_pull_requests.find do |pr|
pr["dependency-group-name"] == group.name
end&.fetch("dependencies", []) || []
end

sig { void }
def assert_current_directory_set!
if @current_directory == "" && directories.count == 1
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ def initialize(service:, job:, dependency_snapshot:, error_handler:)
end

sig { void }
def perform # rubocop:disable Metrics/AbcSize
def perform
# This guards against any jobs being performed where the data is malformed, this should not happen unless
# there was is defect in the service and we emitted a payload where the job and configuration data objects
# were out of sync.
Expand Down Expand Up @@ -100,24 +100,35 @@ def perform # rubocop:disable Metrics/AbcSize
# so users are informed this group is no longer actionable by Dependabot.
warn_group_is_empty(job_group)
close_pull_request(reason: :dependency_group_empty, group: job_group)
else
Dependabot.logger.info("Updating the '#{job_group.name}' group")
return
end

process_group_dependencies(job_group)
end

# Preprocess to discover existing group PRs and add their dependencies to the handled list before processing
# the refresh. This prevents multiple PRs from being created for the same dependency during the refresh.
dependency_snapshot.groups.each do |group|
next unless group.name != job_group.name && pr_exists_for_dependency_group?(group)
sig { params(job_group: Dependabot::DependencyGroup).void }
def process_group_dependencies(job_group)
Dependabot.logger.info("Updating the '#{job_group.name}' group")

dependency_snapshot.mark_group_handled(group)
end
existing_pr_dependencies = Set.new

if dependency_change.nil?
Dependabot.logger.info("Nothing could update for Dependency Group: '#{job_group.name}'")
return
end
# Preprocess to discover existing group PRs and add their dependencies to the handled list before processing
# the refresh. This prevents multiple PRs from being created for the same dependency during the refresh.
dependency_snapshot.groups.each do |group|
group_pr_deps = dependency_snapshot.dependencies_in_existing_pr_for_group(group)
existing_pr_dependencies.merge(group_pr_deps)

upsert_pull_request_with_error_handling(T.must(dependency_change), job_group)
next unless group.name != job_group.name && pr_exists_for_dependency_group?(group)

dependency_snapshot.mark_group_handled(group, existing_pr_dependencies)
end

if dependency_change.nil?
Dependabot.logger.info("Nothing could update for Dependency Group: '#{job_group.name}'")
return
end

upsert_pull_request_with_error_handling(T.must(dependency_change), job_group)
end

private
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,31 @@
refresh_group.perform
end
end

context "when there is an existing PR for the same group it has a minor version in another group" do
let(:job_definition) do
job_definition_fixture("bundler/version_updates/group_update_refresh_multiple_groups_unchaged")
end

let(:dependency_files) do
original_bundler_files(fixture: "bundler_multiple_groups")
end

before do
stub_rubygems_calls
end

it "updates the existing pull request without errors" do
expect(mock_service).not_to receive(:close_pull_request)
expect(mock_service).to receive(:update_pull_request) do |dependency_change|
expect(dependency_change.dependency_group.name).to eql("major")
expect(dependency_change.updated_dependency_files_hash)
.to eql(updated_bundler_files_hash(fixture: "bundler_multiple_groups"))
end

refresh_group.perform
end
end
end

describe "#deduce_updated_dependency" do
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# frozen_string_literal: true

source "https://rubygems.org"

gem "dummy-pkg-a", "~> 1.1.0"
gem "dummy-pkg-b", "~> 1.1.0"
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
GEM
remote: https://rubygems.org/
specs:
dummy-pkg-a (1.1.0)
dummy-pkg-b (1.1.0)

PLATFORMS
ruby

DEPENDENCIES
dummy-pkg-a (~> 1.1.0)
dummy-pkg-b (~> 1.1.0)

BUNDLED WITH
2.4.13
6 changes: 6 additions & 0 deletions updater/spec/fixtures/bundler_multiple_groups/updated/Gemfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# frozen_string_literal: true

source "https://rubygems.org"

gem "dummy-pkg-a", "~> 2.0.0"
gem "dummy-pkg-b", "~> 1.1.0"
15 changes: 15 additions & 0 deletions updater/spec/fixtures/bundler_multiple_groups/updated/Gemfile.lock
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
GEM
remote: https://rubygems.org/
specs:
dummy-pkg-a (2.0.0)
dummy-pkg-b (1.1.0)

PLATFORMS
ruby

DEPENDENCIES
dummy-pkg-a (~> 2.0.0)
dummy-pkg-b (~> 1.1.0)

BUNDLED WITH
2.4.13
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
job:
package-manager: bundler
source:
provider: github
repo: dependabot/dependabot-bug-version-types
branch:
api-endpoint: https://api.github.com/
hostname: github.com
directories:
- "/"
dependencies:
- "dummy-pkg-a"
existing-pull-requests: []
existing-group-pull-requests:
- dependency-group-name: major
dependencies:
- dependency-name: dummy-pkg-a
dependency-version: 2.0.0
directory: "/"
- dependency-group-name: minor
dependencies:
- dependency-name: dummy-pkg-b
dependency-version: 1.2.0
directory: "/"
updating-a-pull-request: true
lockfile-only: false
update-subdependencies: false
ignore-conditions: []
requirements-update-strategy: null
allowed-updates:
- dependency-type: direct
update-type: all
credentials-metadata:
- type: git_source
host: github.com
security-advisories: []
security-updates-only: false
max-updater-run-time: 2700
vendor-dependencies: false
repo-private: false
proxy-log-response-body-on-auth-failure: true
reject-external-code: false
commit-message-options:
prefix:
prefix-development:
include-scope:
dependency-group-to-refresh: major
dependency-groups:
- name: major
rules:
patterns:
- "dummy-pkg-a"
update-types:
- "major"
- name: minor
rules:
patterns:
- "*"
update-types:
- "patch"
- "minor"
experiments:
dependency-change-validation: true
enable-file-parser-python-local: true
enable-fix-for-pnpm-no-change-error: true
enable-record-ecosystem-meta: true
enable-shared-helpers-command-timeout: true
lead-security-dependency: true
move-job-token: true
npm-v6-deprecation-warning: true
npm-v6-unsupported-error: true
nuget-native-analysis: true
nuget-use-direct-discovery: true
proxy-cached: true
python-3-8-deprecation-warning: true
record-ecosystem-versions: true
record-update-job-unknown-error: true
29 changes: 20 additions & 9 deletions updater/spec/fixtures/rubygems-versions-a.json
Original file line number Diff line number Diff line change
@@ -1,4 +1,21 @@
[
{
"authors": "Dependabot",
"built_at": "2017-06-08T00:00:00.000Z",
"created_at": "2017-06-08T11:17:36.474Z",
"description": "",
"downloads_count": 301,
"metadata": {},
"number": "2.0.0",
"summary": "A dummy package for testing Dependabot",
"platform": "ruby",
"rubygems_version": ">= 0",
"ruby_version": ">= 0",
"prerelease": false,
"licenses": ["MIT"],
"requirements": [],
"sha": "8c79aa73922d7f7b38cd96b7af6d489f7c354c6a9fa25bd72772e75d018fbd96"
},
{
"authors": "Dependabot",
"built_at": "2017-06-08T00:00:00.000Z",
Expand All @@ -12,9 +29,7 @@
"rubygems_version": ">= 0",
"ruby_version": ">= 0",
"prerelease": false,
"licenses": [
"MIT"
],
"licenses": ["MIT"],
"requirements": [],
"sha": "51b99c7db0d39924d690e19282f63d1fba9cc002ef55a139d9b6a4b0469399a1"
},
Expand All @@ -31,9 +46,7 @@
"rubygems_version": ">= 0",
"ruby_version": ">= 0",
"prerelease": false,
"licenses": [
"MIT"
],
"licenses": ["MIT"],
"requirements": [],
"sha": "c8725691239b43d5f4c343b64d30afae6dd25ff1a79cca4d80534804c638b113"
},
Expand All @@ -50,9 +63,7 @@
"rubygems_version": ">= 0",
"ruby_version": ">= 0",
"prerelease": false,
"licenses": [
"MIT"
],
"licenses": ["MIT"],
"requirements": [],
"sha": "0147d64042d5ab109d185f54957fcfb88f1ff9158651944ff75c6c82d47ab444"
}
Expand Down

0 comments on commit 527905f

Please sign in to comment.