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

Strong type Dependabot::BranchNamer::DependencyGroupStrategy #8770

Merged
merged 4 commits into from
Jan 11, 2024
Merged

Strong type Dependabot::BranchNamer::DependencyGroupStrategy #8770

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
7124a87
Strong type `Dependabot::BranchNamer::DependencyGroupStrategy`
JamieMagee Jan 11, 2024
0584bbf
Lint fix
JamieMagee Jan 11, 2024
a709931
Merge branch 'main' into jamiemagee/strong-type-dependency-group-stra…
bdragon Jan 11, 2024
55ad493
Merge branch 'main' into jamiemagee/strong-type-dependency-group-stra…
JamieMagee Jan 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions common/lib/dependabot/pull_request_creator/branch_namer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,10 @@ def strategy
files: files,
target_branch: target_branch,
dependency_group: dependency_group,
includes_security_fixes: includes_security_fixes,
JamieMagee marked this conversation as resolved.
Show resolved Hide resolved
separator: separator,
prefix: prefix,
max_length: max_length,
includes_security_fixes: includes_security_fixes
JamieMagee marked this conversation as resolved.
Show resolved Hide resolved
max_length: max_length
)
end
end
Expand Down
38 changes: 32 additions & 6 deletions common/lib/dependabot/pull_request_creator/branch_namer/dependency_group_strategy.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,30 @@
# typed: true
# typed: strong
# frozen_string_literal: true

require "sorbet-runtime"
require "dependabot/pull_request_creator/branch_namer/base"

module Dependabot
class PullRequestCreator
class BranchNamer
class DependencyGroupStrategy < Base
def initialize(dependencies:, files:, target_branch:, dependency_group:,
separator: "/", prefix: "dependabot", max_length: nil, includes_security_fixes:)
extend T::Sig

sig do
params(
dependencies: T::Array[Dependabot::Dependency],
files: T::Array[Dependabot::DependencyFile],
target_branch: String,
dependency_group: Dependabot::DependencyGroup,
includes_security_fixes: T::Boolean,
separator: String,
prefix: String,
max_length: T.nilable(Integer)
)
.void
end
def initialize(dependencies:, files:, target_branch:, dependency_group:, includes_security_fixes:,
separator: "/", prefix: "dependabot", max_length: nil)
super(
dependencies: dependencies,
files: files,
Expand All @@ -22,14 +38,17 @@ def initialize(dependencies:, files:, target_branch:, dependency_group:,
@includes_security_fixes = includes_security_fixes
end

sig { returns(String) }
def new_branch_name
sanitize_branch_name(File.join(prefixes, group_name_with_dependency_digest))
end

private

sig { returns(Dependabot::DependencyGroup) }
attr_reader :dependency_group

sig { returns(T::Array[String]) }
def prefixes
[
prefix,
Expand All @@ -45,6 +64,7 @@ def prefixes
#
# Let's append a short hash digest of the dependency changes so that we can
# meet this guarantee.
sig { returns(String) }
def group_name_with_dependency_digest
if @includes_security_fixes
"group-security-#{package_manager}-#{dependency_digest}"
Expand All @@ -53,16 +73,22 @@ def group_name_with_dependency_digest
end
end

sig { returns(T.nilable(String)) }
def dependency_digest
@dependency_digest ||= Digest::MD5.hexdigest(dependencies.map do |dependency|
"#{dependency.name}-#{dependency.removed? ? 'removed' : dependency.version}"
end.sort.join(",")).slice(0, 10)
@dependency_digest ||= T.let(
Digest::MD5.hexdigest(dependencies.map do |dependency|
"#{dependency.name}-#{dependency.removed? ? 'removed' : dependency.version}"
end.sort.join(",")).slice(0, 10),
T.nilable(String)
)
end

sig { returns(String) }
def package_manager
T.must(dependencies.first).package_manager
end

sig { returns(String) }
def directory
T.must(files.first).directory.tr(" ", "-")
end
Expand Down
10 changes: 5 additions & 5 deletions common/spec/dependabot/pull_request_creator/branch_namer/dependency_group_strategy_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@

context "with defaults for separator, target branch and files in the root directory" do
let(:directory) { "/" }
let(:target_branch) { nil }
let(:target_branch) { "" }
let(:separator) { "/" }

it "returns the name of the dependency group prefixed correctly" do
Expand Down Expand Up @@ -130,7 +130,7 @@

context "with a grouped security update" do
let(:directory) { "/" }
let(:target_branch) { nil }
let(:target_branch) { "" }
JamieMagee marked this conversation as resolved.
Show resolved Hide resolved
let(:separator) { "/" }
let(:includes_security_fixes) { true }
let(:dependency_group) do
Expand All @@ -147,7 +147,7 @@

context "with a custom separator" do
let(:directory) { "/" }
let(:target_branch) { nil }
let(:target_branch) { "" }
let(:separator) { "_" }

it "returns the name of the dependency group prefixed correctly" do
Expand All @@ -157,7 +157,7 @@

context "with a maximum length" do
let(:directory) { "/" }
let(:target_branch) { nil }
let(:target_branch) { "" }
let(:separator) { "/" }

context "with a maximum length longer than branch name" do
Expand Down Expand Up @@ -202,7 +202,7 @@

context "for files in a non-root directory" do
let(:directory) { "rails app/" } # let's make sure we deal with spaces too
let(:target_branch) { nil }
let(:target_branch) { "" }
let(:separator) { "/" }

it "returns the name of the dependency group prefixed correctly" do
Expand Down
26 changes: 13 additions & 13 deletions common/spec/dependabot/pull_request_creator_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
let(:milestone) { nil }
let(:author_details) { nil }
let(:signature_key) { nil }
let(:source) { Dependabot::Source.new(provider: "github", repo: "gc/bump") }
let(:source) { Dependabot::Source.new(provider: "github", repo: "gc/bump", branch: "main") }
let(:files) { [gemfile] }
let(:base_commit) { "basecommitsha" }
let(:provider_metadata) { nil }
Expand Down Expand Up @@ -181,15 +181,15 @@
end

context "with a GitHub source" do
let(:source) { Dependabot::Source.new(provider: "github", repo: "gc/bp") }
let(:source) { Dependabot::Source.new(provider: "github", repo: "gc/bp", branch: "main") }
let(:dummy_creator) { instance_double(described_class::Github) }

it "delegates to PullRequestCreator::Github with correct params" do
expect(described_class::Github)
.to receive(:new)
.with(
source: source,
branch_name: "dependabot/bundler/business-1.5.0",
branch_name: "dependabot/bundler/main/business-1.5.0",
base_commit: base_commit,
credentials: credentials,
files: files,
Expand All @@ -211,7 +211,7 @@
end

context "with a GitLab source" do
let(:source) { Dependabot::Source.new(provider: "gitlab", repo: "gc/bp") }
let(:source) { Dependabot::Source.new(provider: "gitlab", repo: "gc/bp", branch: "main") }
let(:dummy_creator) { instance_double(described_class::Gitlab) }
let(:provider_metadata) { { target_project_id: 1 } }

Expand All @@ -220,7 +220,7 @@
.to receive(:new)
.with(
source: source,
branch_name: "dependabot/bundler/business-1.5.0",
branch_name: "dependabot/bundler/main/business-1.5.0",
base_commit: base_commit,
credentials: credentials,
files: files,
Expand All @@ -240,7 +240,7 @@
end

context "with a Bitbucket source" do
let(:source) { Dependabot::Source.new(provider: "bitbucket", repo: "gc/bp") }
let(:source) { Dependabot::Source.new(provider: "bitbucket", repo: "gc/bp", branch: "main") }
let(:dummy_creator) { instance_double(described_class::Bitbucket) }
let(:provider_metadata) { { work_item: 123 } }

Expand All @@ -249,7 +249,7 @@
.to receive(:new)
.with(
source: source,
branch_name: "dependabot/bundler/business-1.5.0",
branch_name: "dependabot/bundler/main/business-1.5.0",
base_commit: base_commit,
credentials: credentials,
files: files,
Expand All @@ -266,7 +266,7 @@
end

context "with an Azure source" do
let(:source) { Dependabot::Source.new(provider: "azure", repo: "gc/bp") }
let(:source) { Dependabot::Source.new(provider: "azure", repo: "gc/bp", branch: "main") }
let(:dummy_creator) { instance_double(described_class::Azure) }
let(:provider_metadata) { { work_item: 123 } }

Expand All @@ -275,7 +275,7 @@
.to receive(:new)
.with(
source: source,
branch_name: "dependabot/bundler/business-1.5.0",
branch_name: "dependabot/bundler/main/business-1.5.0",
base_commit: base_commit,
credentials: credentials,
files: files,
Expand Down Expand Up @@ -322,7 +322,7 @@
commit_message: commit_message
)
end
let(:source) { Dependabot::Source.new(provider: "github", repo: "gc/bp") }
let(:source) { Dependabot::Source.new(provider: "github", repo: "gc/bp", branch: "main") }
let(:dummy_creator) { instance_double(described_class::Github) }

%i(pr_name pr_message commit_message).each do |field|
Expand All @@ -336,7 +336,7 @@
.to receive(:new)
.with(
source: source,
branch_name: "dependabot/bundler/business-1.5.0",
branch_name: "dependabot/bundler/main/business-1.5.0",
base_commit: base_commit,
credentials: credentials,
files: files,
Expand All @@ -359,7 +359,7 @@

context "with a dependency group" do
let(:dependency_group) { Dependabot::DependencyGroup.new(name: "all-the-things", rules: { patterns: ["*"] }) }
let(:source) { Dependabot::Source.new(provider: "github", repo: "gc/bp") }
let(:source) { Dependabot::Source.new(provider: "github", repo: "gc/bp", branch: "main") }
let(:dummy_creator) { instance_double(described_class::Github) }

subject(:creator_with_group) do
Expand All @@ -385,7 +385,7 @@
.to receive(:new)
.with(
source: source,
branch_name: start_with("dependabot/bundler/all-the-things-"),
branch_name: start_with("dependabot/bundler/main/all-the-things-"),
base_commit: base_commit,
credentials: credentials,
files: files,
Expand Down