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

Pull request updater for azure client #3153

Merged
merged 5 commits into from
Mar 3, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
22 changes: 22 additions & 0 deletions common/lib/dependabot/clients/azure.rb
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,28 @@ def create_pull_request(pr_name, source_branch, target_branch,
"/_apis/git/repositories/" + source.unscoped_repo +
"/pullrequests?api-version=5.0", content.to_json)
end

def fetch_pull_request(pull_request_id)
milind009 marked this conversation as resolved.
Show resolved Hide resolved
response = get(source.api_endpoint +
source.organization + "/" + source.project +
"/_apis/git/pullrequests/" + pull_request_id)

JSON.parse(response.body)
Copy link
Member

Choose a reason for hiding this comment

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

Not blocking for now, but we might want to handle non-200 responses here

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 see we have some sort of handling in the get method of the azure client. Can you specify the scenarios that we might want to cover here?

end

def update_ref(branch_name, old_commit, new_commit)
content = [
{
name: "refs/heads/" + branch_name,
oldObjectId: old_commit,
newObjectId: new_commit
}
]

post(source.api_endpoint + source.organization + "/" + source.project +
"/_apis/git/repositories/" + source.unscoped_repo +
"/refs?api-version=5.0", content.to_json)
end
# rubocop:enable Metrics/ParameterLists

def get(url)
Expand Down
12 changes: 12 additions & 0 deletions common/lib/dependabot/pull_request_updater.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ def update
case source.provider
when "github" then github_updater.update
when "gitlab" then gitlab_updater.update
when "azure" then azure_updater.update
else raise "Unsupported provider #{source.provider}"
end
end
Expand Down Expand Up @@ -56,5 +57,16 @@ def gitlab_updater
pull_request_number: pull_request_number
)
end

def azure_updater
Azure.new(
source: source,
base_commit: base_commit,
old_commit: old_commit,
files: files,
credentials: credentials,
pull_request_number: pull_request_number
)
end
end
end
110 changes: 110 additions & 0 deletions common/lib/dependabot/pull_request_updater/azure.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
# frozen_string_literal: true

require "dependabot/clients/azure"

module Dependabot
class PullRequestUpdater
class Azure
OBJECT_ID_FOR_BRANCH_DELETE = "0000000000000000000000000000000000000000"

attr_reader :source, :files, :base_commit, :old_commit, :credentials,
:pull_request_number

def initialize(source:, files:, base_commit:, old_commit:,
credentials:, pull_request_number:)
@source = source
@files = files
@base_commit = base_commit
@old_commit = old_commit
@credentials = credentials
@pull_request_number = pull_request_number
end

def update
return unless pull_request_exists? && source_branch_exists?

update_source_branch
end

private

def azure_client_for_source
@azure_client_for_source ||=
Dependabot::Clients::Azure.for_source(
source: source,
credentials: credentials
)
end

def pull_request_exists?
pull_request
rescue Dependabot::Clients::Azure::NotFound
false
end

def source_branch_exists?
azure_client_for_source.branch(source_branch_name)
rescue Dependabot::Clients::Azure::NotFound
false
end

# Currently the PR diff in ADO shows difference in commits instead of actual diff in files.
# This workaround is done to get the target branch commit history on the source branch alongwith file changes
def update_source_branch
# 1) Push the file changes to a newly created temporary branch (from base commit)
new_commit = create_temp_branch
# 2) Update PR source branch to point to the temp branch head commit.
update_branch(source_branch_name, old_source_branch_commit, new_commit)
# 3) Delete temp branch
update_branch(temp_branch_name, new_commit, OBJECT_ID_FOR_BRANCH_DELETE)
end

def pull_request
@pull_request ||=
azure_client_for_source.fetch_pull_request(pull_request_number.to_s)
end

def source_branch_name
@source_branch_name ||= pull_request&.fetch("sourceRefName")&.gsub("refs/heads/", "")
end

def create_temp_branch
response = azure_client_for_source.create_commit(
temp_branch_name,
base_commit,
commit_message,
files,
nil
)

JSON.parse(response.body).fetch("refUpdates").first.fetch("newObjectId")
end

def temp_branch_name
"#{source_branch_name}-temp"
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be worth generating a short id, e.g. #{source_branch_name}-temp-Digest::SHA1.hexdigest(source_branch_name)[0..6]?

Thinking of the case where two jobs try to update the same PR or someone has stashed some changes in a -temp version of the current branch, in which case it would be deleted. Seems unlikely but have a feeling I've created branches with temp suffixes in the past when wrangling a rebase for example.

Copy link
Contributor Author

@milind009 milind009 Feb 22, 2021

Choose a reason for hiding this comment

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

@feelepxyz Digest::SHA1.hexdigest(source_branch_name) produces consistent hashing. So for the case where we have two jobs trying to update the same PR, this might still result in issues right? I was thinking of using uuid (documentation) for uniquely identifying the temp branch. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes you're right, SHA1 is stable, uuid seems like a good candidate 👍

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 made an update accordingly

end

def update_branch(branch_name, old_commit, new_commit)
azure_client_for_source.update_ref(
branch_name,
old_commit,
new_commit
)
end

# For updating source branch, we require the latest commit for the source branch.
def commit_being_updated
@commit_being_updated ||=
azure_client_for_source.commits(source_branch_name).first
end

def old_source_branch_commit
commit_being_updated.fetch("commitId")
end

def commit_message
commit_being_updated.fetch("comment")
end
end
end
end
71 changes: 71 additions & 0 deletions common/spec/dependabot/clients/azure_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,77 @@
end
end

describe "#fetch_pull_request" do
subject { client.fetch_pull_request(pull_request_id) }

let(:pull_request_id) { "1" }
let(:pull_request_url) { base_url + "/_apis/git/pullrequests/#{pull_request_id}" }

context "when response is 200" do
response_body = fixture("azure", "pull_request_details.json")

before do
stub_request(:get, pull_request_url).
with(basic_auth: [username, password]).
to_return(status: 200, body: response_body)
end

specify { expect { subject }.to_not raise_error }

it { is_expected.to eq(JSON.parse(response_body)) }
end

context "when response is 404" do
before do
stub_request(:get, pull_request_url).
with(basic_auth: [username, password]).
to_return(status: 404)
end

it "raises a helpful error" do
expect { subject }.to raise_error(Dependabot::Clients::Azure::NotFound)
end
end
end

describe "#update_ref" do
subject(:update_ref) do
client.update_ref(
branch,
old_commit_id,
new_commit_id
)
end

let(:old_commit_id) { "oldcommitsha" }
let(:new_commit_id) { "newcommitsha" }
let(:update_ref_url) { repo_url + "/refs?api-version=5.0" }

it "sends update branch request with old and new commit id" do
stub_request(:post, update_ref_url).
with(basic_auth: [username, password]).
to_return(status: 200)

update_ref

expect(WebMock).
to(
have_requested(:post, update_ref_url).
with do |req|
json_body = JSON.parse(req.body)
expect(json_body.count).to eq(1)
ref_update_details = json_body.first
expect(ref_update_details.fetch("name")).
to eq("refs/heads/#{branch}")
expect(ref_update_details.fetch("oldObjectId")).
to eq(old_commit_id)
expect(ref_update_details.fetch("newObjectId")).
to eq(new_commit_id)
end
)
end
end

describe "#get" do
context "Using auth headers" do
token = ":test_token"
Expand Down
Loading