Skip to content

Commit

Permalink
fix incorrect usage of add_handled_dependencies (#10270)
Browse files Browse the repository at this point in the history
* fix bug where no manifests are found causes assertion much too late
  • Loading branch information
jakecoffman authored Jul 23, 2024
1 parent 0e201cd commit d045d42
Show file tree
Hide file tree
Showing 10 changed files with 188 additions and 108 deletions.
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)
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
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?
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

0 comments on commit d045d42

Please sign in to comment.