Skip to content

Commit

Permalink
Remove dedup_branch_names ff and existing_branches array (#10976)
Browse files Browse the repository at this point in the history
* Remove dedup_branch_names ff and existing_branches array

* lint
  • Loading branch information
Nishnha authored Nov 21, 2024
1 parent e24fe31 commit 429e1b2
Show file tree
Hide file tree
Showing 7 changed files with 14 additions and 63 deletions.
9 changes: 1 addition & 8 deletions common/lib/dependabot/pull_request_creator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -117,9 +117,6 @@ def initialize(cause, pull_request)
sig { returns(T.nilable(T.any(T::Array[String], Integer))) }
attr_reader :milestone

sig { returns(T::Array[String]) }
attr_reader :existing_branches

sig { returns(String) }
attr_reader :branch_name_separator

Expand Down Expand Up @@ -164,7 +161,6 @@ def initialize(cause, pull_request)
reviewers: Reviewers,
assignees: T.nilable(T.any(T::Array[String], T::Array[Integer])),
milestone: T.nilable(T.any(T::Array[String], Integer)),
existing_branches: T::Array[String],
branch_name_separator: String,
branch_name_prefix: String,
branch_name_max_length: T.nilable(Integer),
Expand All @@ -187,8 +183,7 @@ def initialize(source:, base_commit:, dependencies:, files:, credentials:,
pr_message_header: nil, pr_message_footer: nil,
custom_labels: nil, author_details: nil, signature_key: nil,
commit_message_options: {}, vulnerabilities_fixed: {},
reviewers: nil, assignees: nil, milestone: nil,
existing_branches: [], branch_name_separator: "/",
reviewers: nil, assignees: nil, milestone: nil, branch_name_separator: "/",
branch_name_prefix: "dependabot", branch_name_max_length: nil,
label_language: false, automerge_candidate: false,
github_redirection_service: DEFAULT_GITHUB_REDIRECTION_SERVICE,
Expand All @@ -210,7 +205,6 @@ def initialize(source:, base_commit:, dependencies:, files:, credentials:,
@assignees = assignees
@milestone = milestone
@vulnerabilities_fixed = vulnerabilities_fixed
@existing_branches = existing_branches
@branch_name_separator = branch_name_separator
@branch_name_prefix = branch_name_prefix
@branch_name_max_length = branch_name_max_length
Expand Down Expand Up @@ -404,7 +398,6 @@ def branch_namer
files: files,
target_branch: source.branch,
dependency_group: dependency_group,
existing_branches: existing_branches,
separator: branch_name_separator,
prefix: branch_name_prefix,
max_length: branch_name_max_length,
Expand Down
17 changes: 2 additions & 15 deletions common/lib/dependabot/pull_request_creator/branch_namer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,6 @@ class BranchNamer
sig { returns(T.nilable(String)) }
attr_reader :target_branch

sig { returns(T::Array[String]) }
attr_reader :existing_branches

sig { returns(String) }
attr_reader :separator

Expand All @@ -47,21 +44,19 @@ class BranchNamer
files: T::Array[Dependabot::DependencyFile],
target_branch: T.nilable(String),
dependency_group: T.nilable(Dependabot::DependencyGroup),
existing_branches: T::Array[String],
separator: String,
prefix: String,
max_length: T.nilable(Integer),
includes_security_fixes: T::Boolean
)
.void
end
def initialize(dependencies:, files:, target_branch:, dependency_group: nil, existing_branches: [],
separator: "/", prefix: "dependabot", max_length: nil, includes_security_fixes: false)
def initialize(dependencies:, files:, target_branch:, dependency_group: nil, separator: "/",
prefix: "dependabot", max_length: nil, includes_security_fixes: false)
@dependencies = dependencies
@files = files
@target_branch = target_branch
@dependency_group = dependency_group
@existing_branches = existing_branches
@separator = separator
@prefix = prefix
@max_length = max_length
Expand All @@ -77,19 +72,12 @@ def new_branch_name

sig { returns(Dependabot::PullRequestCreator::BranchNamer::Base) }
def strategy
if Dependabot::Experiments.enabled?(:dedup_branch_names) && existing_branches
Dependabot.logger.debug(
"Dependabot::PullRequestCreator::strategy : #{existing_branches}"
)
end

@strategy ||= T.let(
if dependency_group.nil?
SoloStrategy.new(
dependencies: dependencies,
files: files,
target_branch: target_branch,
existing_branches: existing_branches,
separator: separator,
prefix: prefix,
max_length: max_length
Expand All @@ -101,7 +89,6 @@ def strategy
target_branch: target_branch,
dependency_group: T.must(dependency_group),
includes_security_fixes: includes_security_fixes,
existing_branches: existing_branches,
separator: separator,
prefix: prefix,
max_length: max_length
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,6 @@ class Base
sig { returns(T.nilable(String)) }
attr_reader :target_branch

sig { returns(T::Array[String]) }
attr_reader :existing_branches

sig { returns(String) }
attr_reader :separator

Expand All @@ -35,19 +32,17 @@ class Base
dependencies: T::Array[Dependency],
files: T::Array[DependencyFile],
target_branch: T.nilable(String),
existing_branches: T::Array[String],
separator: String,
prefix: String,
max_length: T.nilable(Integer)
)
.void
end
def initialize(dependencies:, files:, target_branch:, existing_branches: [],
separator: "/", prefix: "dependabot", max_length: nil)
def initialize(dependencies:, files:, target_branch:, separator: "/",
prefix: "dependabot", max_length: nil)
@dependencies = dependencies
@files = files
@target_branch = target_branch
@existing_branches = existing_branches
@separator = separator
@prefix = prefix
@max_length = max_length
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,20 +17,18 @@ class DependencyGroupStrategy < Base
target_branch: T.nilable(String),
dependency_group: Dependabot::DependencyGroup,
includes_security_fixes: T::Boolean,
existing_branches: T::Array[String],
separator: String,
prefix: String,
max_length: T.nilable(Integer)
)
.void
end
def initialize(dependencies:, files:, target_branch:, dependency_group:, includes_security_fixes:,
existing_branches: [], separator: "/", prefix: "dependabot", max_length: nil)
separator: "/", prefix: "dependabot", max_length: nil)
super(
dependencies: dependencies,
files: files,
target_branch: target_branch,
existing_branches: existing_branches,
separator: separator,
prefix: prefix,
max_length: max_length,
Expand Down
7 changes: 1 addition & 6 deletions common/lib/dependabot/pull_request_creator/github.rb
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ def create
"Initiating Github pull request."
)

if experiment_duplicate_branch? && branch_exists?(branch_name) && no_pull_request_exists?
if branch_exists?(branch_name) && no_pull_request_exists?
Dependabot.logger.info(
"Existing branch \"#{branch_name}\" found. Pull request not created."
)
Expand Down Expand Up @@ -600,11 +600,6 @@ def raise_custom_error(base_err, type, message)
raise type, message
end
end

sig { returns(T::Boolean) }
def experiment_duplicate_branch?
Dependabot::Experiments.enabled?(:dedup_branch_names)
end
end
# rubocop:enable Metrics/ClassLength
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@
let(:previous_version) { "1.4.0" }
let(:files) { [gemfile] }
let(:target_branch) { nil }
let(:existing_branches) { [] }

let(:gemfile) do
Dependabot::DependencyFile.new(
Expand Down
30 changes: 7 additions & 23 deletions common/spec/dependabot/pull_request_creator/github_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -485,19 +485,10 @@
)
end

it "creates a PR with the right details" do
creator.create

expect(WebMock)
.to have_requested(:post, "#{repo_api_url}/pulls")
.with(
body: {
base: "master",
head: "dependabot/bundler/business-1.5.0",
title: "PR name",
body: "PR msg"
}
)
it "raises a helpful error" do
expect { creator.create }
.to raise_error(Dependabot::PullRequestCreator::BranchAlreadyExists, /business-1.5.0 already exists/)
expect(WebMock).not_to have_requested(:post, "#{repo_api_url}/pulls")
end
end

Expand Down Expand Up @@ -537,9 +528,10 @@
)
end

it "raises the error" do
it "raises a helpful error" do
expect { creator.create }
.to raise_error(Octokit::UnprocessableEntity)
.to raise_error(Dependabot::PullRequestCreator::BranchAlreadyExists, /business-1.5.0 already exists/)
expect(WebMock).not_to have_requested(:post, "#{repo_api_url}/pulls")
end
end

Expand Down Expand Up @@ -615,14 +607,6 @@
context "when the branch already exists" do
let(:service_pack_response) { fixture("git", "upload_packs", "existing-branch-with-no-pr") }

before do
Dependabot::Experiments.register(:dedup_branch_names, true)
end

after do
Dependabot::Experiments.register(:dedup_branch_names, false)
end

context "when the branch already exists" do
before do
url = "#{repo_api_url}/pulls?head=gocardless:#{branch_name}" \
Expand Down

0 comments on commit 429e1b2

Please sign in to comment.