Skip to content
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

fix incorrect usage of add_handled_dependencies #10270

Merged
merged 10 commits into from
Jul 23, 2024
15 changes: 15 additions & 0 deletions silent/tests/testdata/err-glob-not-found.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
! dependabot update -f input.yml --local . --updater-image ghcr.io/dependabot/dependabot-updater-silent
stderr dependency_file_not_found

# Testing glob configuration when there are no manifests.

-- input.yml --
job:
package-manager: "silent"
source:
directories:
- "**/*"
provider: example
hostname: example.com
api-endpoint: https://example.com/api/v3
repo: dependabot/smoke-tests
2 changes: 0 additions & 2 deletions silent/tests/testdata/vu-group-multidir-all.txt
Original file line number Diff line number Diff line change
Expand Up @@ -120,5 +120,3 @@ job:
- dependency-name: dependency-c
dependency-version: 3.2.5
directory: "/bar"
experiments:
dependency_has_directory: true
10 changes: 7 additions & 3 deletions silent/tests/testdata/vu-group-multidir-semver.txt
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
dependabot update -f input.yml --local . --updater-image ghcr.io/dependabot/dependabot-updater-silent
stdout -count=2 create_pull_request
pr-created foo/expected-1.json bar/expected-1.json
pr-created foo/expected-2.json
pr-created foo/expected-2.json bar/expected-2.json

# When there is the same dependency in both directories, one is a major update the other a patch,
# and the user asked for majors in one group and minors in the other, we should create two group PRs
Expand Down Expand Up @@ -32,6 +32,12 @@ pr-created foo/expected-2.json
"dependency-b": { "version": "2.0.1" }
}

-- bar/expected-2.json --
{
"dependency-a": { "version": "2.0.1" },
"dependency-b": { "version": "1.0.0" }
}

-- foo/expected-2.json --
{
"dependency-a": { "version": "1.0.0" },
Expand Down Expand Up @@ -77,5 +83,3 @@ job:
update-types:
- minor
- patch
experiments:
dependency_has_directory: true
52 changes: 37 additions & 15 deletions updater/lib/dependabot/dependency_snapshot.rb
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,13 @@ def all_dependencies

sig { returns(T::Array[Dependabot::DependencyFile]) }
def dependency_files
assert_current_directory_set!
@dependency_files.select { |f| f.directory == @current_directory }
end

sig { returns(T::Array[Dependabot::Dependency]) }
def dependencies
assert_current_directory_set!
T.must(@dependencies[@current_directory])
end

Expand Down Expand Up @@ -103,29 +105,33 @@ 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)
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"] })
# 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))
end
end

sig { params(dependency_names: T.any(String, T::Array[String])).void }
def add_handled_dependencies(dependency_names)
abdulapopoola marked this conversation as resolved.
Show resolved Hide resolved
raise "Current directory not set" if @current_directory == ""

assert_current_directory_set!
set = @handled_dependencies[@current_directory] || Set.new
set += Array(dependency_names)
@handled_dependencies[@current_directory] = set
end

sig { returns(T::Set[String]) }
def handled_dependencies
raise "Current directory not set" if @current_directory == ""

assert_current_directory_set!
T.must(@handled_dependencies[@current_directory])
end

# rubocop:disable Performance/Sum
sig { returns(T::Set[String]) }
def handled_dependencies_all_directories
T.must(@handled_dependencies.values.reduce(&:+))
end
# rubocop:enable Performance/Sum

sig { params(dir: String).void }
def current_directory=(dir)
@current_directory = dir
Expand All @@ -142,10 +148,6 @@ def ungrouped_dependencies
# If no groups are defined, all dependencies are ungrouped by default.
return allowed_dependencies unless groups.any?

if Dependabot::Experiments.enabled?(:dependency_has_directory)
return allowed_dependencies.reject { |dep| handled_dependencies_all_directories.include?(dep.name) }
end

# Otherwise return dependencies that haven't been handled during the group update portion.
allowed_dependencies.reject { |dep| handled_dependencies.include?(dep.name) }
end
Expand Down Expand Up @@ -184,6 +186,8 @@ def initialize(job:, base_commit_sha:, dependency_files:) # rubocop:disable Metr
end

job.source.directory = @original_directory
# reset to ensure we don't accidentally use it later without setting it
@current_directory = ""
return unless job.source.directory

@current_directory = T.must(job.source.directory)
Expand All @@ -210,6 +214,7 @@ def parse_files!

sig { returns(Dependabot::FileParsers::Base) }
def dependency_file_parser
assert_current_directory_set!
job.source.directory = @current_directory
Dependabot::FileParsers.for_package_manager(job.package_manager).new(
dependency_files: dependency_files,
Expand All @@ -220,5 +225,22 @@ def dependency_file_parser
options: job.experiments
)
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
jakecoffman marked this conversation as resolved.
Show resolved Hide resolved
end&.fetch("dependencies", []) || []
end

sig { void }
def assert_current_directory_set!
if @current_directory == "" && directories.count == 1
@current_directory = T.must(directories.first)
return
end

raise DependabotError, "Assertion failed: Current directory not set" if @current_directory == ""
end
end
end
8 changes: 8 additions & 0 deletions updater/lib/dependabot/file_fetcher_command.rb
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ def file_fetcher_for_directory(directory)
@file_fetchers[directory] ||= create_file_fetcher(directory: directory)
end

# rubocop:disable Metrics/PerceivedComplexity
def dependency_files_for_multi_directories
return @dependency_files_for_multi_directories if defined?(@dependency_files_for_multi_directories)

Expand Down Expand Up @@ -128,7 +129,14 @@ def dependency_files_for_multi_directories
post_ecosystem_versions(ff) if should_record_ecosystem_versions?
files
end.compact

if @dependency_files_for_multi_directories.empty?
raise Dependabot::DependencyFileNotFound, job.source.directories.join(", ")
end

@dependency_files_for_multi_directories
end
# rubocop:enable Metrics/PerceivedComplexity

def dependency_files
return @dependency_files if defined?(@dependency_files)
Expand Down
10 changes: 1 addition & 9 deletions updater/lib/dependabot/updater/group_update_creation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,7 @@ def compile_all_dependency_changes_for(group)

Dependabot.logger.info("Updating the #{job.source.directory} directory.")
group.dependencies.each do |dependency|
# We check dependency_snapshot.handled_dependencies instead of handled_dependencies_all_directories here
# because we still want to update a dependency if it's been updated in another manifest files,
# We still want to update a dependency if it's been updated in another manifest files,
# but we should skip it if it's been updated in _the same_ manifest file
if dependency_snapshot.handled_dependencies.include?(dependency.name)
Dependabot.logger.info(
Expand Down Expand Up @@ -427,13 +426,6 @@ def pr_exists_for_dependency_group?(group)
job.existing_group_pull_requests.any? { |pr| pr["dependency-group-name"] == group.name }
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 do
params(
dependency: T.nilable(Dependabot::Dependency),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,6 @@ def perform
sig { returns(Dependabot::Updater::ErrorHandler) }
attr_reader :error_handler

# rubocop:disable Metrics/AbcSize
sig { returns(T::Array[Dependabot::DependencyGroup]) }
def run_grouped_dependency_updates
Dependabot.logger.info("Starting grouped update job for #{job.source.repo}")
Expand All @@ -113,32 +112,19 @@ def run_grouped_dependency_updates
Dependabot.logger.info(
"Deferring creation of a new pull request. The existing pull request will update in a separate job."
)

# add the dependencies in the group so individual updates don't try to update them
dependency_snapshot.add_handled_dependencies(
dependencies_in_existing_pr_for_group(group).filter_map { |d| d["dependency-name"] }
)
# 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
dependency_snapshot.add_handled_dependencies(group.dependencies.map(&:name))
dependency_snapshot.mark_group_handled(group)
next
end

group
end

groups_without_pr.each do |group|
grouped_update_result = run_grouped_update_for(group)
if grouped_update_result
# Add the actual updated dependencies to the handled list so they don't get updated individually.
dependency_snapshot.add_handled_dependencies(grouped_update_result.updated_dependencies.map(&:name))
else
# The update failed, add the suspected dependencies to the handled list so they don't update individually.
dependency_snapshot.add_handled_dependencies(group.dependencies.map(&:name))
end
dependency_change = run_grouped_update_for(group)
# The update failed, add the suspected dependencies to the handled list so they don't update individually.
dependency_snapshot.mark_group_handled(group) if dependency_change.nil?
jakecoffman marked this conversation as resolved.
Show resolved Hide resolved
end
end
# rubocop:enable Metrics/AbcSize

sig { params(group: Dependabot::DependencyGroup).returns(T.nilable(Dependabot::DependencyChange)) }
def run_grouped_update_for(group)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,7 @@ def perform # rubocop:disable Metrics/AbcSize
dependency_snapshot.groups.each do |group|
next unless group.name != job_group.name && pr_exists_for_dependency_group?(group)

dependency_snapshot.add_handled_dependencies(
dependencies_in_existing_pr_for_group(group).filter_map { |d| d["dependency-name"] }
)
dependency_snapshot.mark_group_handled(group)
end

if dependency_change.nil?
Expand Down
Loading
Loading