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.

Sign up for GitHub

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

Jump to bottom

removing more of dependency_has_directory feature flag #10252

Merged
merged 4 commits into from
Jul 19, 2024
Merged

removing more of dependency_has_directory feature flag #10252

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e9aaefa
revert add_handled_group_dependencies
jakecoffman Jul 19, 2024
a2810da
lint: remove enable which is not needed anymore
jakecoffman Jul 19, 2024
6de4df9
rename to be more clear what it's doing
jakecoffman Jul 19, 2024
ed129dc
Merge branch 'main' into remove-feature-flag
jakecoffman Jul 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 4 additions & 21 deletions updater/lib/dependabot/dependency_snapshot.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,17 +112,6 @@ def add_handled_dependencies(dependency_names)
@handled_dependencies[@current_directory] = set
end

sig { params(dependencies: T::Array[{ name: T.nilable(String), directory: T.nilable(String) }]).void }
def add_handled_group_dependencies(dependencies)
raise "Current directory not set" if @current_directory == ""

dependencies.group_by { |d| d[:directory] }.each do |dir, dependency_hash|
set = @handled_dependencies[dir] || Set.new
set.merge(dependency_hash.map { |d| d[:name] })
@handled_dependencies[dir] = set
end
end

sig { returns(T::Set[String]) }
def handled_dependencies
raise "Current directory not set" if @current_directory == ""
Expand All @@ -132,7 +121,7 @@ def handled_dependencies

# rubocop:disable Performance/Sum
sig { returns(T::Set[String]) }
def handled_group_dependencies
def handled_dependencies_all_directories
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 _all_directories reads as being very open to interpretation. I could see this signaling:

  • That the dependencies are mapped to all directories (a product-like result)
  • That the dependencies are returned agnostic of their directory 👈
  • That the dependencies are combined with their directories (and/or all directories)

From context, I suspect it's the one with 👈 next to it.

Are directories important here? If they are, I wonder if there's another signal we could use to make it unambiguous.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming is hard!

In #9610 what I was doing was rename all the directory-contextually-aware code to have a _current_directory suffix, would that be clearer? Then we can leave this as handled_dependencies or even all_handled_dependencies?

T.must(@handled_dependencies.values.reduce(&:+))
end
# rubocop:enable Performance/Sum
Expand All @@ -154,11 +143,11 @@ def ungrouped_dependencies
return allowed_dependencies unless groups.any?

if Dependabot::Experiments.enabled?(:dependency_has_directory)
return allowed_dependencies.reject { |dep| handled_group_dependencies.include?(dep.name) }
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| T.must(@handled_dependencies[@current_directory]).include?(dep.name) }
allowed_dependencies.reject { |dep| handled_dependencies.include?(dep.name) }
end

private
Expand All @@ -178,13 +167,7 @@ def initialize(job:, base_commit_sha:, dependency_files:) # rubocop:disable Metr
@dependencies = T.let({}, T::Hash[String, T::Array[Dependabot::Dependency]])
directories.each do |dir|
@current_directory = dir
if Dependabot::Experiments.enabled?(:dependency_has_directory)
dependencies = parse_files!
dependencies_with_dir = dependencies.each { |dep| dep.directory = dir }
@dependencies[dir] = dependencies_with_dir
else
@dependencies[dir] = parse_files!
end
@dependencies[dir] = parse_files!
end

@dependency_group_engine = T.let(DependencyGroupEngine.from_job_config(job: job),
Expand Down
29 changes: 4 additions & 25 deletions updater/lib/dependabot/updater/group_update_creation.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +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_group_dependencies here
# 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,
# but we should skip it if it's been updated in _the same_ manifest file
if dependency_snapshot.handled_dependencies.include?(dependency.name)
Expand Down Expand Up @@ -189,7 +189,6 @@ def create_change_for(lead_dependency, updated_dependencies, dependency_files, d
#
# This method **must** must return an Array when it errors
#
# rubocop:disable Metrics/AbcSize, Metrics/PerceivedComplexity
sig do
params(
dependency: Dependabot::Dependency,
Expand All @@ -213,12 +212,7 @@ def compile_updates_for(dependency, dependency_files, group) # rubocop:disable M

# Consider the dependency handled so no individual PR is raised since it is in this group.
# Even if update is not possible, etc.
if Dependabot::Experiments.enabled?(:dependency_has_directory)
dependency_snapshot.add_handled_group_dependencies([{ name: dependency.name,
directory: job.source.directory }])
else
dependency_snapshot.add_handled_dependencies(dependency.name)
end
dependency_snapshot.add_handled_dependencies(dependency.name)

if checker.up_to_date?
log_up_to_date(dependency)
Expand All @@ -239,12 +233,7 @@ def compile_updates_for(dependency, dependency_files, group) # rubocop:disable M
requirements_to_unlock: requirements_to_unlock
)
rescue Dependabot::InconsistentRegistryResponse => e
if Dependabot::Experiments.enabled?(:dependency_has_directory)
dependency_snapshot.add_handled_group_dependencies([{ name: dependency.name,
directory: job.source.directory }])
else
dependency_snapshot.add_handled_dependencies(dependency.name)
end
dependency_snapshot.add_handled_dependencies(dependency.name)
error_handler.log_dependency_error(
dependency: dependency,
error: e,
Expand All @@ -255,16 +244,10 @@ def compile_updates_for(dependency, dependency_files, group) # rubocop:disable M
rescue StandardError => e
# If there was an error we might not be able to determine if the dependency is in this
# group due to semver grouping, so we consider it handled to avoid raising an individual PR.
if Dependabot::Experiments.enabled?(:dependency_has_directory)
dependency_snapshot.add_handled_group_dependencies([{ name: dependency.name,
directory: job.source.directory }])
else
dependency_snapshot.add_handled_dependencies(dependency.name)
end
dependency_snapshot.add_handled_dependencies(dependency.name)
error_handler.handle_dependency_error(error: e, dependency: dependency, dependency_group: group)
[] # return an empty set
end
# rubocop:enable Metrics/AbcSize, Metrics/PerceivedComplexity

sig { params(dependency: Dependabot::Dependency).void }
def log_up_to_date(dependency)
Expand Down Expand Up @@ -476,10 +459,6 @@ def deduce_updated_dependency(dependency, original_dependency)
package_manager: dependency.package_manager
}

if Dependabot::Experiments.enabled?(:dependency_has_directory)
dependency_params[:directory] = dependency.directory
end

Dependabot::Dependency.new(**dependency_params)
end
end
Expand Down
36 changes: 9 additions & 27 deletions updater/lib/dependabot/updater/operations/group_update_all_versions.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ def perform
sig { returns(Dependabot::Updater::ErrorHandler) }
attr_reader :error_handler

# rubocop:disable Metrics/AbcSize, Metrics/PerceivedComplexity
# 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 @@ -114,31 +114,13 @@ def run_grouped_dependency_updates
"Deferring creation of a new pull request. The existing pull request will update in a separate job."
)

if Dependabot::Experiments.enabled?(:dependency_has_directory)

# A grouped version update gets its directories from user-defined update configs.
# A multi-directory grouped update will iterate each group over every directory.
# Therefore, we can skip a grouped dependency if it's been updated in *any* directory
# add the dependencies in the group so individual updates don't try to update them
dependency_snapshot.add_handled_group_dependencies(
dependencies_in_existing_pr_for_group(group)
.map { |d| { name: d["dependency-name"], directory: d["directory"] } }
)
# 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_group_dependencies(
group.dependencies.map { |d| { name: d.name, directory: d.directory } }
)
else
# 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))
end

# 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))
next
end

Expand All @@ -156,7 +138,7 @@ def run_grouped_dependency_updates
end
end
end
# rubocop:enable Metrics/AbcSize, Metrics/PerceivedComplexity
# rubocop:enable Metrics/AbcSize

sig { params(group: Dependabot::DependencyGroup).returns(T.nilable(Dependabot::DependencyChange)) }
def run_grouped_update_for(group)
Expand Down
25 changes: 10 additions & 15 deletions updater/spec/dependabot/dependency_snapshot_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@
end
end

describe "::add_handled_group_dependencies" do
describe "::handled_dependencies_all_directories" do
subject(:create_dependency_snapshot) do
described_class.create_from_job_definition(
job: job,
Expand All @@ -157,11 +157,8 @@

it "handles dependencies" do
snapshot = create_dependency_snapshot
snapshot.add_handled_group_dependencies([
{ name: "a", directory: job.source.directory },
{ name: "b", directory: job.source.directory }
])
expect(snapshot.handled_group_dependencies).to eq(Set.new(%w(a b)))
snapshot.add_handled_dependencies(%w(a b))
expect(snapshot.handled_dependencies_all_directories).to eq(Set.new(%w(a b)))
end

context "when there are multiple directories" do
Expand All @@ -185,14 +182,13 @@
it "is agnostic of the current directory" do
snapshot = create_dependency_snapshot
snapshot.current_directory = "/foo"
snapshot.add_handled_group_dependencies([
{ name: "a", directory: "/foo" },
{ name: "b", directory: "/bar" }
])
snapshot.add_handled_dependencies("a")
snapshot.current_directory = "/bar"
snapshot.add_handled_dependencies("b")

expect(snapshot.handled_group_dependencies).to eq(Set.new(%w(a b)))
expect(snapshot.handled_dependencies_all_directories).to eq(Set.new(%w(a b)))
snapshot.current_directory = "/bar"
expect(snapshot.handled_group_dependencies).to eq(Set.new(%w(a b)))
expect(snapshot.handled_dependencies_all_directories).to eq(Set.new(%w(a b)))
end
end
end
Expand Down Expand Up @@ -251,9 +247,8 @@

expect(snapshot.ungrouped_dependencies.length).to be(2)

snapshot.add_handled_group_dependencies([
{ name: group.dependencies.find { |d| d.name == "dummy-pkg-a" }.name, directory: job.source.directory }
])
snapshot.current_directory = directory
snapshot.add_handled_dependencies("dummy-pkg-a")
expect(snapshot.ungrouped_dependencies.first.name).to eql("dummy-pkg-b")

Dependabot::Experiments.reset!
Expand Down
Loading