From 5c2eb8b59d97372342513c6a3582727e898dae77 Mon Sep 17 00:00:00 2001 From: milind009 Date: Wed, 14 Apr 2021 21:21:37 +0530 Subject: [PATCH 1/4] adding error handling for failure while updating PR --- common/lib/dependabot/clients/azure.rb | 8 +++++--- common/lib/dependabot/errors.rb | 9 +++++++++ common/lib/dependabot/pull_request_updater/azure.rb | 4 +++- 3 files changed, 17 insertions(+), 4 deletions(-) diff --git a/common/lib/dependabot/clients/azure.rb b/common/lib/dependabot/clients/azure.rb index 5338c92dbfd..3b8a3b76ecf 100644 --- a/common/lib/dependabot/clients/azure.rb +++ b/common/lib/dependabot/clients/azure.rb @@ -201,9 +201,11 @@ def update_ref(branch_name, old_commit, new_commit) } ] - post(source.api_endpoint + source.organization + "/" + source.project + - "/_apis/git/repositories/" + source.unscoped_repo + - "/refs?api-version=5.0", content.to_json) + response = post(source.api_endpoint + source.organization + "/" + source.project + + "/_apis/git/repositories/" + source.unscoped_repo + + "/refs?api-version=5.0", content.to_json) + + JSON.parse(response.body).fetch("value").first end # rubocop:enable Metrics/ParameterLists diff --git a/common/lib/dependabot/errors.rb b/common/lib/dependabot/errors.rb index bd8ffc360c7..6340782f1e8 100644 --- a/common/lib/dependabot/errors.rb +++ b/common/lib/dependabot/errors.rb @@ -71,6 +71,15 @@ def initialize(source, msg = nil) end end + class PullRequestUpdatefailed < DependabotError + attr_reader :pull_request_id + + def initialize(pull_request_id, msg = nil) + @pull_request_id = pull_request_id + super(msg) + end + end + ##################### # File level errors # ##################### diff --git a/common/lib/dependabot/pull_request_updater/azure.rb b/common/lib/dependabot/pull_request_updater/azure.rb index d2a506b7c9a..03a3c5accb8 100644 --- a/common/lib/dependabot/pull_request_updater/azure.rb +++ b/common/lib/dependabot/pull_request_updater/azure.rb @@ -55,9 +55,11 @@ 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) + response = 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) + + raise Dependabot::PullRequestUpdateFailed.new(pull_request_number, response.fetch('customMessage', nil)) unless response.fetch('success', false) end def pull_request From 53d786fa7047faba3b9f68693f36ad5fe2fed946 Mon Sep 17 00:00:00 2001 From: milind009 Date: Wed, 14 Apr 2021 22:51:29 +0530 Subject: [PATCH 2/4] modifying tests --- common/lib/dependabot/errors.rb | 2 +- .../dependabot/pull_request_updater/azure.rb | 7 +- common/spec/dependabot/clients/azure_spec.rb | 2 +- .../pull_request_updater/azure_spec.rb | 114 ++++++++++-------- common/spec/fixtures/azure/update_ref.json | 14 +++ .../fixtures/azure/update_ref_failed.json | 14 +++ 6 files changed, 100 insertions(+), 53 deletions(-) create mode 100644 common/spec/fixtures/azure/update_ref.json create mode 100644 common/spec/fixtures/azure/update_ref_failed.json diff --git a/common/lib/dependabot/errors.rb b/common/lib/dependabot/errors.rb index 6340782f1e8..ae64fe75c4c 100644 --- a/common/lib/dependabot/errors.rb +++ b/common/lib/dependabot/errors.rb @@ -71,7 +71,7 @@ def initialize(source, msg = nil) end end - class PullRequestUpdatefailed < DependabotError + class PullRequestUpdateFailed < DependabotError attr_reader :pull_request_id def initialize(pull_request_id, msg = nil) diff --git a/common/lib/dependabot/pull_request_updater/azure.rb b/common/lib/dependabot/pull_request_updater/azure.rb index 03a3c5accb8..abeca96c1ab 100644 --- a/common/lib/dependabot/pull_request_updater/azure.rb +++ b/common/lib/dependabot/pull_request_updater/azure.rb @@ -59,7 +59,12 @@ def update_source_branch # 3) Delete temp branch update_branch(temp_branch_name, new_commit, OBJECT_ID_FOR_BRANCH_DELETE) - raise Dependabot::PullRequestUpdateFailed.new(pull_request_number, response.fetch('customMessage', nil)) unless response.fetch('success', false) + unless response.fetch( + "success", false + ) + raise Dependabot::PullRequestUpdateFailed.new(pull_request_number, + response.fetch("customMessage", nil)) + end end def pull_request diff --git a/common/spec/dependabot/clients/azure_spec.rb b/common/spec/dependabot/clients/azure_spec.rb index e93e9d15233..f5dd7f21606 100644 --- a/common/spec/dependabot/clients/azure_spec.rb +++ b/common/spec/dependabot/clients/azure_spec.rb @@ -188,7 +188,7 @@ 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) + to_return(status: 200, body: fixture("azure", "update_ref.json")) update_ref diff --git a/common/spec/dependabot/pull_request_updater/azure_spec.rb b/common/spec/dependabot/pull_request_updater/azure_spec.rb index 7e3fbf83712..e1d95291852 100644 --- a/common/spec/dependabot/pull_request_updater/azure_spec.rb +++ b/common/spec/dependabot/pull_request_updater/azure_spec.rb @@ -113,57 +113,71 @@ end end - it "updates source branch head commit in AzureDevOps" do - stub_request(:get, source_branch_commits_url). - to_return(status: 200, - body: fixture("azure", "commits.json"), - headers: json_header) - stub_request(:get, repo_contents_tree_url). - to_return(status: 200, - body: fixture("azure", "repo_contents_treeroot.json"), - headers: json_header) - stub_request(:get, repo_contents_url). - to_return(status: 200, - body: fixture("azure", "repo_contents.json"), - headers: json_header) - stub_request(:post, create_commit_url). - with(body: { - refUpdates: [ - { name: "refs/heads/#{temp_branch}", oldObjectId: base_commit } - ], - commits: [ - { - comment: commit_message, - changes: files.map do |file| - { - changeType: "edit", - item: { path: file.path }, - newContent: { - content: Base64.encode64(file.content), - contentType: "base64encoded" + context "tries updating source branch head commit in AzureDevOps" do + before do + stub_request(:get, source_branch_commits_url). + to_return(status: 200, + body: fixture("azure", "commits.json"), + headers: json_header) + stub_request(:get, repo_contents_tree_url). + to_return(status: 200, + body: fixture("azure", "repo_contents_treeroot.json"), + headers: json_header) + stub_request(:get, repo_contents_url). + to_return(status: 200, + body: fixture("azure", "repo_contents.json"), + headers: json_header) + stub_request(:post, create_commit_url). + with(body: { + refUpdates: [ + { name: "refs/heads/#{temp_branch}", oldObjectId: base_commit } + ], + commits: [ + { + comment: commit_message, + changes: files.map do |file| + { + changeType: "edit", + item: { path: file.path }, + newContent: { + content: Base64.encode64(file.content), + contentType: "base64encoded" + } } - } - end - } - ] - }). - to_return(status: 201, - body: fixture("azure", "create_new_branch.json"), - headers: json_header) - stub_request(:post, branch_update_url). - to_return(status: 201) - - allow(updater).to receive(:temp_branch_name).and_return(temp_branch) - updater.update - - expect(WebMock). - to( - have_requested(:post, branch_update_url). - with(body: [ - { name: "refs/heads/#{source_branch}", oldObjectId: source_branch_old_commit, - newObjectId: source_branch_new_commit } - ].to_json) - ) + end + } + ] + }). + to_return(status: 201, + body: fixture("azure", "create_new_branch.json"), + headers: json_header) + + allow(updater).to receive(:temp_branch_name).and_return(temp_branch) + end + + it "sends request to AzureDevOps to update source branch head commit" do + stub_request(:post, branch_update_url). + to_return(status: 201, body: fixture("azure", "update_ref.json")) + + allow(updater).to receive(:temp_branch_name).and_return(temp_branch) + updater.update + + expect(WebMock). + to( + have_requested(:post, branch_update_url). + with(body: [ + { name: "refs/heads/#{source_branch}", oldObjectId: source_branch_old_commit, + newObjectId: source_branch_new_commit } + ].to_json) + ) + end + + it "raises a helpful error when source branch update is unsuccessful" do + stub_request(:post, branch_update_url). + to_return(status: 200, body: fixture("azure", "update_ref_failed.json")) + + expect { updater.update }.to raise_error(Dependabot::PullRequestUpdateFailed) + end end end end diff --git a/common/spec/fixtures/azure/update_ref.json b/common/spec/fixtures/azure/update_ref.json new file mode 100644 index 00000000000..fc453488727 --- /dev/null +++ b/common/spec/fixtures/azure/update_ref.json @@ -0,0 +1,14 @@ +{ + "value": [ + { + "repositoryId": "d3d1760b-311c-4175-a726-20dfc6a7f885", + "name": "refs/heads/vsts-api-sample/answer-woman-flame", + "oldObjectId": "ggf0dcb632e11e8e71f433956183349746fec562", + "newObjectId": "ffe9cba521f00d7f60e322845072238635edb451", + "isLocked": false, + "updateStatus": "succeeded", + "success": true + } + ], + "count": 1 +} \ No newline at end of file diff --git a/common/spec/fixtures/azure/update_ref_failed.json b/common/spec/fixtures/azure/update_ref_failed.json new file mode 100644 index 00000000000..aac16e49da6 --- /dev/null +++ b/common/spec/fixtures/azure/update_ref_failed.json @@ -0,0 +1,14 @@ +{ + "value": [ + { + "repositoryId": "d3d1760b-311c-4175-a726-20dfc6a7f885", + "name": "refs/heads/vsts-api-sample/answer-woman-flame", + "oldObjectId": "ggf0dcb632e11e8e71f433956183349746fec562", + "customMessage": "The user does not have force push permissions", + "isLocked": false, + "updateStatus": "failed", + "success": false + } + ], + "count": 1 +} \ No newline at end of file From 46343eec71386d84bcd6630ae783fba3d45a2f60 Mon Sep 17 00:00:00 2001 From: Philip Harrison Date: Thu, 15 Apr 2021 10:58:56 +0100 Subject: [PATCH 3/4] Update common/lib/dependabot/pull_request_updater/azure.rb Co-authored-by: Jurre --- common/lib/dependabot/pull_request_updater/azure.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/common/lib/dependabot/pull_request_updater/azure.rb b/common/lib/dependabot/pull_request_updater/azure.rb index abeca96c1ab..cb0f6cfd03a 100644 --- a/common/lib/dependabot/pull_request_updater/azure.rb +++ b/common/lib/dependabot/pull_request_updater/azure.rb @@ -59,9 +59,7 @@ def update_source_branch # 3) Delete temp branch update_branch(temp_branch_name, new_commit, OBJECT_ID_FOR_BRANCH_DELETE) - unless response.fetch( - "success", false - ) + unless response.fetch("success", false) raise Dependabot::PullRequestUpdateFailed.new(pull_request_number, response.fetch("customMessage", nil)) end From 08d13b723b5b7d4467ed7251e986aa1c994591b8 Mon Sep 17 00:00:00 2001 From: milind009 Date: Thu, 15 Apr 2021 21:37:12 +0530 Subject: [PATCH 4/4] adding PullRequestUpdateFailed error in azure pr updater class --- common/lib/dependabot/errors.rb | 9 --------- common/lib/dependabot/pull_request_updater/azure.rb | 7 +++---- .../spec/dependabot/pull_request_updater/azure_spec.rb | 2 +- 3 files changed, 4 insertions(+), 14 deletions(-) diff --git a/common/lib/dependabot/errors.rb b/common/lib/dependabot/errors.rb index ae64fe75c4c..bd8ffc360c7 100644 --- a/common/lib/dependabot/errors.rb +++ b/common/lib/dependabot/errors.rb @@ -71,15 +71,6 @@ def initialize(source, msg = nil) end end - class PullRequestUpdateFailed < DependabotError - attr_reader :pull_request_id - - def initialize(pull_request_id, msg = nil) - @pull_request_id = pull_request_id - super(msg) - end - end - ##################### # File level errors # ##################### diff --git a/common/lib/dependabot/pull_request_updater/azure.rb b/common/lib/dependabot/pull_request_updater/azure.rb index cb0f6cfd03a..e6111d1ea9a 100644 --- a/common/lib/dependabot/pull_request_updater/azure.rb +++ b/common/lib/dependabot/pull_request_updater/azure.rb @@ -6,6 +6,8 @@ module Dependabot class PullRequestUpdater class Azure + class PullRequestUpdateFailed < Dependabot::DependabotError; end + OBJECT_ID_FOR_BRANCH_DELETE = "0000000000000000000000000000000000000000" attr_reader :source, :files, :base_commit, :old_commit, :credentials, @@ -59,10 +61,7 @@ def update_source_branch # 3) Delete temp branch update_branch(temp_branch_name, new_commit, OBJECT_ID_FOR_BRANCH_DELETE) - unless response.fetch("success", false) - raise Dependabot::PullRequestUpdateFailed.new(pull_request_number, - response.fetch("customMessage", nil)) - end + raise PullRequestUpdateFailed, response.fetch("customMessage", nil) unless response.fetch("success", false) end def pull_request diff --git a/common/spec/dependabot/pull_request_updater/azure_spec.rb b/common/spec/dependabot/pull_request_updater/azure_spec.rb index e1d95291852..fe5e62d5fe0 100644 --- a/common/spec/dependabot/pull_request_updater/azure_spec.rb +++ b/common/spec/dependabot/pull_request_updater/azure_spec.rb @@ -176,7 +176,7 @@ stub_request(:post, branch_update_url). to_return(status: 200, body: fixture("azure", "update_ref_failed.json")) - expect { updater.update }.to raise_error(Dependabot::PullRequestUpdateFailed) + expect { updater.update }.to raise_error(Dependabot::PullRequestUpdater::Azure::PullRequestUpdateFailed) end end end