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

Add option to control PR description max length + set reasonable defaults for each platform #7487

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
a237476
Add truncate_pr_description and align rescue properly in fetch_file_c…
dwc0011 Jun 26, 2023
06461cd
Add specs for truncate method
dwc0011 Jun 29, 2023
55c1ac3
use stubbed client
dwc0011 Jun 29, 2023
1540d6d
Use client
dwc0011 Jun 29, 2023
25b499e
Fix logic on comparing truncated strings
dwc0011 Jun 29, 2023
09ad912
Move truncate to message builder and pass in the default values if no…
dwc0011 Jul 21, 2023
ac9ddc5
Remove spec that was unintentionally added
dwc0011 Jul 21, 2023
62a8f76
Remove the old pr description tests
dwc0011 Jul 21, 2023
946b1eb
Reuse properly configured builder
dwc0011 Jul 21, 2023
6c126c7
Fix reference to undefined variable
dwc0011 Jul 21, 2023
7429f9e
Add setter for new fields
dwc0011 Jul 21, 2023
514f5e2
Fix setters
dwc0011 Jul 21, 2023
13d79f0
return msg
dwc0011 Jul 21, 2023
610cae0
Add consistency to how max length is used, create encode variable
dwc0011 Jul 21, 2023
0c087c1
Fix lint and add param to encode
dwc0011 Jul 21, 2023
a2c90e9
Remove useless ||=
dwc0011 Jul 21, 2023
3c9ddf5
reference instance variables correctly and add missing pr_message_enc…
dwc0011 Jul 21, 2023
8aefb0b
Add return if message is not nil
dwc0011 Jul 21, 2023
8d1c02d
Reword comment
dwc0011 Jul 21, 2023
e157faa
add mention of why encoding is an option and what the return value wi…
dwc0011 Jul 21, 2023
f89d727
Rename variables
dwc0011 Jul 24, 2023
b477cfa
Add comment
dwc0011 Jul 24, 2023
dc2ba3a
Update common/lib/dependabot/pull_request_creator/azure.rb
jeffwidman Jul 25, 2023
08ec1bc
Update common/lib/dependabot/pull_request_creator/github.rb
jeffwidman Jul 25, 2023
cb325b8
Update common/lib/dependabot/pull_request_creator/codecommit.rb
jeffwidman Jul 25, 2023
0225914
Update common/lib/dependabot/pull_request_creator/github.rb
jeffwidman Jul 25, 2023
36e0687
Remove old variable that is now in pull_request_creator
dwc0011 Jul 25, 2023
0d82621
Update common/lib/dependabot/pull_request_creator/codecommit.rb
jeffwidman Jul 25, 2023
dbfd0bd
Update common/lib/dependabot/pull_request_creator/azure.rb
jeffwidman Jul 25, 2023
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
16 changes: 0 additions & 16 deletions common/lib/dependabot/clients/azure.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,6 @@ class TagsCreationForbidden < StandardError; end

RETRYABLE_ERRORS = [InternalServerError, BadGateway, ServiceNotAvailable].freeze

MAX_PR_DESCRIPTION_LENGTH = 3999

#######################
# Constructor methods #
#######################
Expand Down Expand Up @@ -174,7 +172,6 @@ def create_commit(branch_name, base_commit, commit_message, files,
def create_pull_request(pr_name, source_branch, target_branch,
pr_description, labels,
reviewers = nil, assignees = nil, work_item = nil)
pr_description = truncate_pr_description(pr_description)

content = {
sourceRefName: "refs/heads/" + source_branch,
Expand Down Expand Up @@ -375,19 +372,6 @@ def auth_header_for(token)
end
end

def truncate_pr_description(pr_description)
# Azure DevOps only support descriptions up to 4000 characters in UTF-16
# encoding.
# https://developercommunity.visualstudio.com/content/problem/608770/remove-4000-character-limit-on-pull-request-descri.html
pr_description = pr_description.dup.force_encoding(Encoding::UTF_16)
if pr_description.length > MAX_PR_DESCRIPTION_LENGTH
truncated_msg = (+"...\n\n_Description has been truncated_").force_encoding(Encoding::UTF_16)
truncate_length = MAX_PR_DESCRIPTION_LENGTH - truncated_msg.length
pr_description = (pr_description[0..truncate_length] + truncated_msg)
end
pr_description.force_encoding(Encoding::UTF_8)
end

def tags_creation_forbidden?(response)
return if response.body.empty?

Expand Down
47 changes: 32 additions & 15 deletions common/lib/dependabot/pull_request_creator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ def initialize(cause, pull_request)
:commit_message_options, :vulnerabilities_fixed,
:reviewers, :assignees, :milestone, :branch_name_separator,
:branch_name_prefix, :branch_name_max_length, :github_redirection_service,
:custom_headers, :provider_metadata, :dependency_group
:custom_headers, :provider_metadata, :dependency_group, :pr_message_max_length,
:pr_message_encoding

def initialize(source:, base_commit:, dependencies:, files:, credentials:,
pr_message_header: nil, pr_message_footer: nil,
Expand All @@ -61,7 +62,8 @@ def initialize(source:, base_commit:, dependencies:, files:, credentials:,
automerge_candidate: false,
github_redirection_service: DEFAULT_GITHUB_REDIRECTION_SERVICE,
custom_headers: nil, require_up_to_date_base: false,
provider_metadata: {}, message: nil, dependency_group: nil)
provider_metadata: {}, message: nil, dependency_group: nil, pr_message_max_length: nil,
pr_message_encoding: nil)
@dependencies = dependencies
@source = source
@base_commit = base_commit
Expand All @@ -88,6 +90,8 @@ def initialize(source:, base_commit:, dependencies:, files:, credentials:,
@provider_metadata = provider_metadata
@message = message
@dependency_group = dependency_group
@pr_message_max_length = pr_message_max_length
Copy link
Member

@jeffwidman jeffwidman Jul 24, 2023

Choose a reason for hiding this comment

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

Any reason why you picked pr_message... rather than pr_description...?

My thinking is there are multiple messages tied to a PR, for example comments, reviews, etc but only one main description tied to it... Just checked and GitHub, AWS CodeCommit, BitBucket, GitLab, ADO all call it PR/MR Description, so appears that's relatively consistent terminology...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I named it that as the method called is pr_message in MessageBuilder

Copy link
Member

Choose a reason for hiding this comment

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

Ah, gotcha, yeah, it probably wouldn't hurt to rename that to DescriptionBuilder (totally out of scope of this PR)... I'll look into that after this is merged... let's leave this comment unresolved as a reminder to me.

@pr_message_encoding = pr_message_encoding

check_dependencies_have_previous_version
end
Expand Down Expand Up @@ -216,19 +220,32 @@ def codecommit_creator
end

def message
@message ||=
MessageBuilder.new(
source: source,
dependencies: dependencies,
files: files,
credentials: credentials,
commit_message_options: commit_message_options,
pr_message_header: pr_message_header,
pr_message_footer: pr_message_footer,
vulnerabilities_fixed: vulnerabilities_fixed,
github_redirection_service: github_redirection_service,
dependency_group: dependency_group
)
return @message unless @message.nil?

case source.provider
when "github"
@pr_message_max_length = Github::PR_DESCRIPTION_MAX_LENGTH if @pr_message_max_length.nil?
when "azure"
@pr_message_max_length = Azure::PR_DESCRIPTION_MAX_LENGTH if @pr_message_max_length.nil?
@pr_message_encoding = Azure::PR_DESCRIPTION_ENCODING if @pr_message_encoding.nil?
when "codecommit"
@pr_message_max_length = Codecommit::PR_DESCRIPTION_MAX_LENGTH if @pr_message_max_length.nil?
end

@message = MessageBuilder.new(
source: source,
dependencies: dependencies,
files: files,
credentials: credentials,
commit_message_options: commit_message_options,
pr_message_header: pr_message_header,
pr_message_footer: pr_message_footer,
vulnerabilities_fixed: vulnerabilities_fixed,
github_redirection_service: github_redirection_service,
dependency_group: dependency_group,
pr_message_max_length: pr_message_max_length,
pr_message_encoding: pr_message_encoding
)
end

def branch_namer
Expand Down
5 changes: 5 additions & 0 deletions common/lib/dependabot/pull_request_creator/azure.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@ class Azure
:files, :commit_message, :pr_description, :pr_name,
:author_details, :labeler, :reviewers, :assignees, :work_item

# Azure DevOps limits PR descriptions to a max of 4,000 characters in UTF-16 encoding:
# https://developercommunity.visualstudio.com/content/problem/608770/remove-4000-character-limit-on-pull-request-descri.html
PR_DESCRIPTION_MAX_LENGTH = 3_999 # 0 based count
PR_DESCRIPTION_ENCODING = Encoding::UTF_16

def initialize(source:, branch_name:, base_commit:, credentials:,
files:, commit_message:, pr_description:, pr_name:,
author_details:, labeler:, reviewers: nil, assignees: nil, work_item: nil)
Expand Down
4 changes: 4 additions & 0 deletions common/lib/dependabot/pull_request_creator/codecommit.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ class Codecommit
:files, :commit_message, :pr_description, :pr_name,
:author_details, :labeler

# CodeCommit limits PR descriptions to a max length of 10,240 characters:
# https://docs.aws.amazon.com/codecommit/latest/APIReference/API_PullRequest.html
PR_DESCRIPTION_MAX_LENGTH = 10_239 # 0 based count

def initialize(source:, branch_name:, base_commit:, credentials:,
files:, commit_message:, pr_description:, pr_name:,
author_details:, labeler:, require_up_to_date_base:)
Expand Down
16 changes: 3 additions & 13 deletions common/lib/dependabot/pull_request_creator/github.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ module Dependabot
class PullRequestCreator
# rubocop:disable Metrics/ClassLength
class Github
MAX_PR_DESCRIPTION_LENGTH = 65_536 # characters (see #create_pull_request)
dwc0011 marked this conversation as resolved.
Show resolved Hide resolved
# GitHub limits PR descriptions to a max of 65,536 characters:
# https://github.com/orgs/community/discussions/27190#discussioncomment-3726017
PR_DESCRIPTION_MAX_LENGTH = 65_535 # 0 based count

attr_reader :source, :branch_name, :base_commit, :credentials,
:files, :pr_description, :pr_name, :commit_message,
Expand Down Expand Up @@ -349,18 +351,6 @@ def add_milestone_to_pull_request(pull_request)
end

def create_pull_request
# Limit PR description to MAX_PR_DESCRIPTION_LENGTH (65,536) characters
# and truncate with message if over. The API limit is 262,144 bytes
# (https://github.community/t/maximum-length-for-the-comment-body-in-issues-and-pr/148867/2).
# As Ruby strings are UTF-8 encoded, this is a pessimistic limit: it
# presumes the case where all characters are 4 bytes.
pr_description = @pr_description.dup
if pr_description && pr_description.length > MAX_PR_DESCRIPTION_LENGTH
truncated_msg = "...\n\n_Description has been truncated_"
truncate_length = MAX_PR_DESCRIPTION_LENGTH - truncated_msg.length
pr_description = (pr_description[0, truncate_length] + truncated_msg)
end

github_client_for_source.create_pull_request(
source.repo,
target_branch,
Expand Down
35 changes: 31 additions & 4 deletions common/lib/dependabot/pull_request_creator/message_builder.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,16 @@ class MessageBuilder
attr_reader :source, :dependencies, :files, :credentials,
:pr_message_header, :pr_message_footer,
:commit_message_options, :vulnerabilities_fixed,
:github_redirection_service, :dependency_group
:github_redirection_service, :dependency_group, :pr_message_max_length,
:pr_message_encoding

TRUNCATED_MSG = "...\n\n_Description has been truncated_"

def initialize(source:, dependencies:, files:, credentials:,
pr_message_header: nil, pr_message_footer: nil,
commit_message_options: {}, vulnerabilities_fixed: {},
github_redirection_service: DEFAULT_GITHUB_REDIRECTION_SERVICE,
dependency_group: nil)
dependency_group: nil, pr_message_max_length: nil, pr_message_encoding: nil)
@dependencies = dependencies
@files = files
@source = source
Expand All @@ -39,22 +42,46 @@ def initialize(source:, dependencies:, files:, credentials:,
@vulnerabilities_fixed = vulnerabilities_fixed
@github_redirection_service = github_redirection_service
@dependency_group = dependency_group
@pr_message_max_length = pr_message_max_length
@pr_message_encoding = pr_message_encoding
end

attr_writer :pr_message_max_length

attr_writer :pr_message_encoding

def pr_name
name = dependency_group ? group_pr_name : solo_pr_name
name[0] = name[0].capitalize if pr_name_prefixer.capitalize_first_word?
"#{pr_name_prefix}#{name}"
end

def pr_message
suffixed_pr_message_header + commit_message_intro +
metadata_cascades + prefixed_pr_message_footer
msg = "#{suffixed_pr_message_header}#{commit_message_intro}#{metadata_cascades}#{prefixed_pr_message_footer}"
truncate_pr_message(msg)
rescue StandardError => e
Dependabot.logger.error("Error while generating PR message: #{e.message}")
suffixed_pr_message_header + prefixed_pr_message_footer
end

# Truncate PR message as determined by the pr_message_max_length and pr_message_encoding instance variables
# The encoding is used when calculating length, all messages are returned as ruby UTF_8 encoded string
def truncate_pr_message(msg)
return msg if pr_message_max_length.nil?

msg = msg.dup
msg = msg.force_encoding(pr_message_encoding) unless pr_message_encoding.nil?

if msg.length > pr_message_max_length
tr_msg = pr_message_encoding.nil? ? TRUNCATED_MSG : (+TRUNCATED_MSG).dup.force_encoding(pr_message_encoding)
trunc_length = pr_message_max_length - tr_msg.length
msg = (msg[0..trunc_length] + tr_msg)
end
# if we used a custom encoding for calculating length, then we need to force back to UTF-8
msg.force_encoding(Encoding::UTF_8) unless pr_message_encoding.nil?
dwc0011 marked this conversation as resolved.
Show resolved Hide resolved
msg
end

def commit_message
message = commit_subject + "\n\n"
message += commit_message_intro
Expand Down
17 changes: 0 additions & 17 deletions common/spec/dependabot/pull_request_creator/azure_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -168,23 +168,6 @@
end
end

context "with e very long pr description" do
let(:pr_description) { ("a" * 3997) + "💣 kaboom" }
it "truncates the description respecting azures encoding" do
creator.create

expect(WebMock).
to(
have_requested(:post, "#{repo_api_url}/pullrequests?api-version=5.0").
with do |req|
description = JSON.parse(req.body).fetch("description")
expect(description.length).to eq 4000
expect(description).to end_with("\n\n_Description has been truncated_")
end
)
end
end

context "with author details provided" do
let(:author_details) do
{ email: "[email protected]", name: "dependabot" }
Expand Down
19 changes: 0 additions & 19 deletions common/spec/dependabot/pull_request_creator/github_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1088,25 +1088,6 @@
)
end
end

context "the PR description is too long" do
let(:pr_description) { "a" * (described_class::MAX_PR_DESCRIPTION_LENGTH + 1) }

it "truncates the description" 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: ->(body) { expect(body.length).to be <= described_class::MAX_PR_DESCRIPTION_LENGTH }
}
)
end
end
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -2572,4 +2572,68 @@ def commits_details(base:, head:)
its(:pr_message) { should eq(pr_message) }
its(:commit_message) { should eq(commit_message) }
end

subject(:message_builder) { builder }
describe "#truncate_pr_message" do
context "when pr_message_max_length is not provided" do
let(:message) { "This is a normal length PR description and it should not be truncated." }

it "returns the original message" do
expect(message_builder.truncate_pr_message(message)).to eq(message)
end

let(:message) { "This is a test message with special characters: © ®" }

it "returns the original encoding of the message" do
message_builder.pr_message_encoding = Encoding::UTF_16
expect(message_builder.truncate_pr_message(message)).to eq(message)
end
end

context "when pr_message_max_length is provided" do
let(:message) { "A" * 10_250 } # Exceeds the maximum length of 10,239
let(:pr_message_max_length) { 10_239 }

it "truncates the message to the specified length" do
truncated_msg = "...\n\n_Description has been truncated_"
truncate_length = pr_message_max_length - truncated_msg.length
expected_truncated_description = "#{message[0..truncate_length]}#{truncated_msg}"

message_builder.pr_message_max_length = pr_message_max_length
expect(message_builder.truncate_pr_message(message)).to eq(expected_truncated_description)
end

let(:message) { "© ®" * 100 } # Exceeds the maximum length of 100
let(:pr_message_max_length) { 100 }

it "truncates and maintains the specified encoding" do
encode_utf16 = Encoding::UTF_16
msg = message.dup.force_encoding(encode_utf16)
trunc_msg = (+"...\n\n_Description has been truncated_").force_encoding(encode_utf16)
trunc_length = pr_message_max_length - trunc_msg.length
msg = "#{msg[0..trunc_length]}#{trunc_msg}"
msg = msg.force_encoding(Encoding::UTF_8)

message_builder.pr_message_max_length = pr_message_max_length
message_builder.pr_message_encoding = encode_utf16
expect(message_builder.truncate_pr_message(message)).to eq(msg)
end
end

context "when the pull request description is an empty string" do
let(:message) { "" }
let(:pr_message_max_length) { 100 }

it "returns an empty string" do
message_builder.pr_message_max_length = pr_message_max_length
expect(message_builder.truncate_pr_message(message)).to eq("")
end

it "returns an empty string when encoded" do
message_builder.pr_message_max_length = pr_message_max_length
message_builder.pr_message_encoding = Encoding::UTF_16
expect(message_builder.truncate_pr_message(message)).to eq("")
end
end
end
end