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

Adding update PR elements in azure PR updater #3486

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
44 changes: 44 additions & 0 deletions common/lib/dependabot/clients/azure.rb
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,28 @@ def create_pull_request(pr_name, source_branch, target_branch,
"/pullrequests?api-version=5.0", content.to_json)
end

def update_pull_request(pull_request_id:, status: nil, pr_name: nil, description: nil,
completion_options: nil, merge_options: nil,
auto_complete_setby_user_id: nil, target_branch: nil)
content = {
autoCompleteSetBy: ({ id: auto_complete_setby_user_id } unless
auto_complete_setby_user_id.nil? || auto_complete_setby_user_id.strip.empty?),
title: pr_name,
description: truncate_pr_description(description),
status: status,
completionOptions: completion_options,
mergeOptions: merge_options,
targetRefName: ("refs/heads/" + target_branch unless target_branch.nil? || target_branch.strip.empty?)
}.compact

return if content.empty?

patch(source.api_endpoint +
source.organization + "/" + source.project +
"/_apis/git/repositories/" + source.unscoped_repo +
"/pullrequests/" + pull_request_id.to_s + "?api-version=5.0", content.to_json)
end

def pull_request(pull_request_id)
response = get(source.api_endpoint +
source.organization + "/" + source.project +
Expand Down Expand Up @@ -251,6 +273,26 @@ def post(url, json)
response
end

def patch(url, json)
response = Excon.patch(
url,
body: json,
user: credentials&.fetch("username", nil),
password: credentials&.fetch("password", nil),
idempotent: true,
**SharedHelpers.excon_defaults(
headers: auth_header.merge(
{
"Content-Type" => "application/json"
}
)
)
)
raise NotFound if response.status == 404

response
end

private

def retry_connection_failures
Expand Down Expand Up @@ -279,6 +321,8 @@ def auth_header_for(token)
end

def truncate_pr_description(pr_description)
return if pr_description.nil? || pr_description.empty?

# 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
Expand Down
15 changes: 15 additions & 0 deletions common/lib/dependabot/pull_request_updater/azure.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,21 @@ def update
update_source_branch
end

def update_pr_elements(elements = {})
Copy link
Member

Choose a reason for hiding this comment

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

What's the idea behind updating certain elements sometimes vs the entire PR other times?

A concern is that we now no longer follow the same API for different updaters.

I don't know if that's necessarily problematic, but up until now we've kept the interfaces the same between these different clients. At this point I think it's pretty unlikely that someone will agnostically open PRs against different providers, so deviating might be fine. @feelepxyz wdyt?

I've been thinking that it might even make sense to completely split off the Azure and GitLab clients from core and finding long-term maintainers for those, as we're probably not the best folks to maintain this code.

Copy link
Contributor

Choose a reason for hiding this comment

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

@milind009 would be good to understand the use case for this and why the existing update method is not working for you? Slightly worried we're adding support for use-cases we don't understand and we end up with several slightly different ways to do the same thing.

Keen to figure if we can improve the existing update method instead or leave this as something that's done outside of dependabot if it's specific to your workflow.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm just re-reading your PR description and sounds like we won't be able to easily fix the existing update method to do what you want. Looks like we only pass in pr_name in the creator for example and assuming you want to be able to update this?

I'm inclined to not merge this into core and encourage you to write a wrapper for this functionality in your dependabot setup as it seems specific to what you are doing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@feelepxyz Currently the update function that we have for the azure client is used force push new changes on to the PR. However, we have a requirement to set a PR to autocomplete (azure create PR API does have an option to set PR to autocomplete but unfortunately it does not work, hence we need to create a PR and then set it to autocomplete)/abandon existing PR for which we need to use the azure update PR API. So I created to separate method which can be used for updating other PR aesthetics like title, description,etc.

Copy link
Member

Choose a reason for hiding this comment

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

@milind009 is this something that could maybe live in the project that you use to run dependabot? It doesn't seem to rely on anything dependabot-specific?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool makes sense. I'll add this in our project. Thanks for the prompt replies!

return unless elements

azure_client_for_source.update_pull_request(
pull_request_id: pull_request_number,
status: elements[:status],
pr_name: elements[:pr_name],
description: elements[:description],
completion_options: elements[:completion_options],
merge_options: elements[:merge_options],
auto_complete_setby_user_id: elements[:auto_complete_setby_user_id],
target_branch: elements[:target_branch]
)
end

private

def azure_client_for_source
Expand Down
54 changes: 54 additions & 0 deletions common/spec/dependabot/clients/azure_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,60 @@
end
end

describe "#update_pull_request" do
subject(:update_pull_request) do
client.update_pull_request(
pull_request_id: pull_request_id,
auto_complete_setby_user_id: auto_complete_setby_user_id,
pr_name: pr_name
)
end

let(:pull_request_id) { 1 }
let(:auto_complete_setby_user_id) { "id" }
let(:pr_name) { "Test PR" }
let(:update_pull_request_url) { repo_url + "/pullrequests/" + pull_request_id.to_s + "?api-version=5.0" }

it "returns nil when there are no parameters to update" do
response = client.update_pull_request(pull_request_id: pull_request_id)

expect(response).to be_nil
end

context "sends update PR request with updated parameter values" do
it "successfully updates the PR" do
stub_request(:patch, update_pull_request_url).
with(basic_auth: [username, password]).
to_return(status: 200)

update_pull_request

expect(WebMock).
to(
have_requested(:patch, update_pull_request_url).
with do |req|
pr_update_details = JSON.parse(req.body)
expect(pr_update_details).
to eq(
{
"title" => pr_name,
"autoCompleteSetBy" => { "id" => auto_complete_setby_user_id }
}
)
end
)
end

it "raises helpful error when response is 404" do
stub_request(:patch, update_pull_request_url).
with(basic_auth: [username, password]).
to_return(status: 404)

expect { update_pull_request }.to raise_error(Dependabot::Clients::Azure::NotFound)
end
end
end

describe "#get" do
context "Using auth headers" do
token = ":test_token"
Expand Down
35 changes: 35 additions & 0 deletions common/spec/dependabot/pull_request_updater/azure_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -166,4 +166,39 @@
)
end
end

describe "#update_pr_elements" do
let(:update_pull_request_url) { repo_url + "/pullrequests/" + pull_request_number.to_s + "?api-version=5.0" }

context "returns nil" do
it "when elements hash is empty" do
expect(updater.update_pr_elements).to be_nil
end

it "when elements hash does not contain expected parameters for update" do
elements = { test_parameter: "test" }

expect(updater.update_pr_elements(elements)).to be_nil
end
end

it "updates the given pr elements" do
elements = { pr_name: "Test PR", auto_complete_setby_user_id: "id" }

stub_request(:patch, update_pull_request_url).
to_return(status: 200)

updater.update_pr_elements(elements)

expect(WebMock).
to(
have_requested(:patch, update_pull_request_url).
with(body:
{
title: elements[:pr_name],
autoCompleteSetBy: { id: elements[:auto_complete_setby_user_id] }
})
)
end
end
end