From 9b8415abb4085d47c85057c111149ac4c15a3c24 Mon Sep 17 00:00:00 2001 From: Andrejs Cunskis Date: Fri, 15 Oct 2021 12:31:36 +0300 Subject: [PATCH 1/3] Allow passing target_project_id to Gitlab pr creator and updater --- common/lib/dependabot/pull_request_creator.rb | 8 ++-- .../dependabot/pull_request_creator/gitlab.rb | 38 ++++++++++--------- common/lib/dependabot/pull_request_updater.rb | 9 +++-- .../dependabot/pull_request_updater/gitlab.rb | 7 ++-- 4 files changed, 35 insertions(+), 27 deletions(-) diff --git a/common/lib/dependabot/pull_request_creator.rb b/common/lib/dependabot/pull_request_creator.rb index 99ed4729170..eb7b4181238 100644 --- a/common/lib/dependabot/pull_request_creator.rb +++ b/common/lib/dependabot/pull_request_creator.rb @@ -49,7 +49,7 @@ def initialize(cause, pull_request) :commit_message_options, :vulnerabilities_fixed, :reviewers, :assignees, :milestone, :branch_name_separator, :branch_name_prefix, :github_redirection_service, - :custom_headers, :provider_metadata + :custom_headers, :provider_metadata, :target_project_id def initialize(source:, base_commit:, dependencies:, files:, credentials:, pr_message_header: nil, pr_message_footer: nil, @@ -60,7 +60,7 @@ def initialize(source:, base_commit:, dependencies:, files:, credentials:, label_language: false, automerge_candidate: false, github_redirection_service: DEFAULT_GITHUB_REDIRECTION_SERVICE, custom_headers: nil, require_up_to_date_base: false, - provider_metadata: {}, message: nil) + provider_metadata: {}, message: nil, target_project_id: nil) @dependencies = dependencies @source = source @base_commit = base_commit @@ -85,6 +85,7 @@ def initialize(source:, base_commit:, dependencies:, files:, credentials:, @require_up_to_date_base = require_up_to_date_base @provider_metadata = provider_metadata @message = message + @target_project_id = target_project_id check_dependencies_have_previous_version end @@ -157,7 +158,8 @@ def gitlab_creator labeler: labeler, approvers: reviewers, assignees: assignees, - milestone: milestone + milestone: milestone, + target_project_id: target_project_id ) end diff --git a/common/lib/dependabot/pull_request_creator/gitlab.rb b/common/lib/dependabot/pull_request_creator/gitlab.rb index 601954b7c82..26f70d147ad 100644 --- a/common/lib/dependabot/pull_request_creator/gitlab.rb +++ b/common/lib/dependabot/pull_request_creator/gitlab.rb @@ -10,25 +10,26 @@ class Gitlab attr_reader :source, :branch_name, :base_commit, :credentials, :files, :pr_description, :pr_name, :commit_message, :author_details, :labeler, :approvers, :assignees, - :milestone + :milestone, :target_project_id def initialize(source:, branch_name:, base_commit:, credentials:, files:, commit_message:, pr_description:, pr_name:, author_details:, labeler:, approvers:, assignees:, - milestone:) - @source = source - @branch_name = branch_name - @base_commit = base_commit - @credentials = credentials - @files = files - @commit_message = commit_message - @pr_description = pr_description - @pr_name = pr_name - @author_details = author_details - @labeler = labeler - @approvers = approvers - @assignees = assignees - @milestone = milestone + milestone:, target_project_id:) + @source = source + @branch_name = branch_name + @base_commit = base_commit + @credentials = credentials + @files = files + @commit_message = commit_message + @pr_description = pr_description + @pr_name = pr_name + @author_details = author_details + @labeler = labeler + @approvers = approvers + @assignees = assignees + @milestone = milestone + @target_project_id = target_project_id end def create @@ -76,7 +77,7 @@ def commit_exists? def merge_request_exists? gitlab_client_for_source.merge_requests( - source.repo, + target_project_id || source.repo, source_branch: branch_name, target_branch: source.branch || default_branch, state: "all" @@ -143,7 +144,8 @@ def create_merge_request remove_source_branch: true, assignee_ids: assignees, labels: labeler.labels_for_pr.join(","), - milestone_id: milestone + milestone_id: milestone, + target_project_id: target_project_id ) end @@ -156,7 +158,7 @@ def add_approvers_to_merge_request(merge_request) approvers.keys.map { |k| [k.to_sym, approvers[k]] }.to_h gitlab_client_for_source.edit_merge_request_approvers( - source.repo, + merge_request.project_id, merge_request.iid, approver_ids: approvers_hash[:approvers], approver_group_ids: approvers_hash[:group_approvers] diff --git a/common/lib/dependabot/pull_request_updater.rb b/common/lib/dependabot/pull_request_updater.rb index 0ad2c346136..e342d1edead 100644 --- a/common/lib/dependabot/pull_request_updater.rb +++ b/common/lib/dependabot/pull_request_updater.rb @@ -9,11 +9,12 @@ class PullRequestUpdater class BranchProtected < StandardError; end attr_reader :source, :files, :base_commit, :old_commit, :credentials, - :pull_request_number, :author_details, :signature_key + :pull_request_number, :author_details, :signature_key, :target_project_id def initialize(source:, base_commit:, old_commit:, files:, credentials:, pull_request_number:, - author_details: nil, signature_key: nil) + author_details: nil, signature_key: nil, + target_project_id: nil) @source = source @base_commit = base_commit @old_commit = old_commit @@ -22,6 +23,7 @@ def initialize(source:, base_commit:, old_commit:, files:, @pull_request_number = pull_request_number @author_details = author_details @signature_key = signature_key + @target_project_id = target_project_id end def update @@ -55,7 +57,8 @@ def gitlab_updater old_commit: old_commit, files: files, credentials: credentials, - pull_request_number: pull_request_number + pull_request_number: pull_request_number, + target_project_id: target_project_id ) end diff --git a/common/lib/dependabot/pull_request_updater/gitlab.rb b/common/lib/dependabot/pull_request_updater/gitlab.rb index fecf24add5b..32a8998372c 100644 --- a/common/lib/dependabot/pull_request_updater/gitlab.rb +++ b/common/lib/dependabot/pull_request_updater/gitlab.rb @@ -8,16 +8,17 @@ module Dependabot class PullRequestUpdater class Gitlab attr_reader :source, :files, :base_commit, :old_commit, :credentials, - :pull_request_number + :pull_request_number, :target_project_id def initialize(source:, base_commit:, old_commit:, files:, - credentials:, pull_request_number:) + credentials:, pull_request_number:, target_project_id:) @source = source @base_commit = base_commit @old_commit = old_commit @files = files @credentials = credentials @pull_request_number = pull_request_number + @target_project_id = target_project_id end def update @@ -39,7 +40,7 @@ def merge_request_exists? def merge_request @merge_request ||= gitlab_client_for_source.merge_request( - source.repo, + target_project_id || source.repo, pull_request_number ) end From 74d70cadfbf81d9dd9f1cb0d4e49c91d85d9be30 Mon Sep 17 00:00:00 2001 From: Andrejs Cunskis Date: Sun, 17 Oct 2021 13:18:39 +0300 Subject: [PATCH 2/3] Update specs Validate target_project_id passed correctly Fix request expectation --- .../dependabot/pull_request_creator/gitlab.rb | 2 +- .../pull_request_creator/gitlab_spec.rb | 37 +++++++++++++++++-- .../dependabot/pull_request_creator_spec.rb | 7 +++- .../pull_request_updater/gitlab_spec.rb | 18 ++++++++- .../dependabot/pull_request_updater_spec.rb | 7 +++- 5 files changed, 62 insertions(+), 9 deletions(-) diff --git a/common/lib/dependabot/pull_request_creator/gitlab.rb b/common/lib/dependabot/pull_request_creator/gitlab.rb index 26f70d147ad..f2e949dbfd3 100644 --- a/common/lib/dependabot/pull_request_creator/gitlab.rb +++ b/common/lib/dependabot/pull_request_creator/gitlab.rb @@ -158,7 +158,7 @@ def add_approvers_to_merge_request(merge_request) approvers.keys.map { |k| [k.to_sym, approvers[k]] }.to_h gitlab_client_for_source.edit_merge_request_approvers( - merge_request.project_id, + target_project_id || source.repo, merge_request.iid, approver_ids: approvers_hash[:approvers], approver_group_ids: approvers_hash[:group_approvers] diff --git a/common/spec/dependabot/pull_request_creator/gitlab_spec.rb b/common/spec/dependabot/pull_request_creator/gitlab_spec.rb index ca25e857e47..efd12120881 100644 --- a/common/spec/dependabot/pull_request_creator/gitlab_spec.rb +++ b/common/spec/dependabot/pull_request_creator/gitlab_spec.rb @@ -20,7 +20,8 @@ labeler: labeler, approvers: approvers, assignees: assignees, - milestone: milestone + milestone: milestone, + target_project_id: target_project_id ) end @@ -45,6 +46,7 @@ let(:approvers) { nil } let(:assignees) { nil } let(:milestone) { nil } + let(:target_project_id) { nil } let(:labeler) do Dependabot::PullRequestCreator::Labeler.new( source: source, @@ -171,6 +173,20 @@ to have_requested(:post, "#{repo_api_url}/merge_requests") end + context "with forked project" do + let(:target_project_id) { 1 } + + it "pushes a commit to GitLab and creates a merge request in upstream project" do + creator.create + + expect(WebMock). + to have_requested(:post, "#{repo_api_url}/merge_requests"). + with( + body: a_string_including("target_project_id=#{target_project_id}") + ) + end + end + context "with a submodule" do let(:files) do [ @@ -331,8 +347,12 @@ context "when a approvers has been requested" do let(:approvers) { { "approvers" => [1_394_555] } } + let(:mr_api_url) do + "https://gitlab.com/api/v4/projects/#{target_project_id || CGI.escape(source.repo)}/merge_requests" + end + before do - stub_request(:put, "#{repo_api_url}/merge_requests/5/approvers"). + stub_request(:put, "#{mr_api_url}/5/approvers"). to_return( status: 200, body: fixture("gitlab", "merge_request.json"), @@ -344,7 +364,18 @@ creator.create expect(WebMock). - to have_requested(:put, "#{repo_api_url}/merge_requests/5/approvers") + to have_requested(:put, "#{mr_api_url}/5/approvers") + end + + context "with forked project" do + let(:target_project_id) { 1 } + + it "adds the approvers to upstream project MR" do + creator.create + + expect(WebMock). + to have_requested(:put, "#{mr_api_url}/5/approvers") + end end end end diff --git a/common/spec/dependabot/pull_request_creator_spec.rb b/common/spec/dependabot/pull_request_creator_spec.rb index 82761e61736..869b9eaa280 100644 --- a/common/spec/dependabot/pull_request_creator_spec.rb +++ b/common/spec/dependabot/pull_request_creator_spec.rb @@ -21,7 +21,8 @@ milestone: milestone, author_details: author_details, signature_key: signature_key, - provider_metadata: provider_metadata + provider_metadata: provider_metadata, + target_project_id: target_project_id ) end @@ -48,6 +49,7 @@ let(:files) { [gemfile] } let(:base_commit) { "basecommitsha" } let(:provider_metadata) { nil } + let(:target_project_id) { 1 } let(:credentials) do [{ "type" => "git_source", @@ -228,7 +230,8 @@ labeler: instance_of(described_class::Labeler), approvers: reviewers, assignees: nil, - milestone: milestone + milestone: milestone, + target_project_id: target_project_id ).and_return(dummy_creator) expect(dummy_creator).to receive(:create) creator.create diff --git a/common/spec/dependabot/pull_request_updater/gitlab_spec.rb b/common/spec/dependabot/pull_request_updater/gitlab_spec.rb index 4a45ab9c0c9..66354bd9552 100644 --- a/common/spec/dependabot/pull_request_updater/gitlab_spec.rb +++ b/common/spec/dependabot/pull_request_updater/gitlab_spec.rb @@ -14,7 +14,8 @@ old_commit: old_commit, files: files, credentials: credentials, - pull_request_number: merge_request_number + pull_request_number: merge_request_number, + target_project_id: target_project_id ) end @@ -77,6 +78,7 @@ repo_api_url + "/repository/branches/" + CGI.escape(branch_name) end let(:commit_url) { repo_api_url + "/repository/commits" } + let(:target_project_id) { nil } before do stub_request(:get, merge_request_url). @@ -98,6 +100,20 @@ end describe "#update" do + context "with forked project" do + let(:target_project_id) { 1 } + let(:merge_request_url) do + "https://gitlab.com/api/v4/projects/#{target_project_id}/merge_requests/#{merge_request_number}" + end + + it "fetches mr from upstream project" do + updater.update + + expect(WebMock). + to have_requested(:get, merge_request_url) + end + end + context "when the branch doesn't exist" do before { stub_request(:get, branch_url).to_return(status: 404) } diff --git a/common/spec/dependabot/pull_request_updater_spec.rb b/common/spec/dependabot/pull_request_updater_spec.rb index 3ca266c6b59..dbe2afe7339 100644 --- a/common/spec/dependabot/pull_request_updater_spec.rb +++ b/common/spec/dependabot/pull_request_updater_spec.rb @@ -14,7 +14,8 @@ old_commit: old_commit, files: files, credentials: credentials, - pull_request_number: pull_request_number + pull_request_number: pull_request_number, + target_project_id: target_project_id ) end @@ -24,6 +25,7 @@ let(:old_commit) { "oldcommitsha" } let(:pull_request_number) { 1 } let(:credentials) { [] } + let(:target_project_id) { 1 } describe "#update" do context "with a GitHub source" do @@ -61,7 +63,8 @@ old_commit: old_commit, files: files, credentials: credentials, - pull_request_number: pull_request_number + pull_request_number: pull_request_number, + target_project_id: target_project_id ).and_return(dummy_updater) expect(dummy_updater).to receive(:update) updater.update From fe6b71f321368d080e417612889216240ea0998d Mon Sep 17 00:00:00 2001 From: Andrejs Cunskis Date: Mon, 18 Oct 2021 14:11:09 +0300 Subject: [PATCH 3/3] Move target_project_id to provider_metadata --- common/lib/dependabot/pull_request_creator.rb | 7 +++---- common/lib/dependabot/pull_request_updater.rb | 8 ++++---- common/spec/dependabot/pull_request_creator_spec.rb | 7 +++---- common/spec/dependabot/pull_request_updater_spec.rb | 6 ++++-- 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/common/lib/dependabot/pull_request_creator.rb b/common/lib/dependabot/pull_request_creator.rb index eb7b4181238..6259ca27b88 100644 --- a/common/lib/dependabot/pull_request_creator.rb +++ b/common/lib/dependabot/pull_request_creator.rb @@ -49,7 +49,7 @@ def initialize(cause, pull_request) :commit_message_options, :vulnerabilities_fixed, :reviewers, :assignees, :milestone, :branch_name_separator, :branch_name_prefix, :github_redirection_service, - :custom_headers, :provider_metadata, :target_project_id + :custom_headers, :provider_metadata def initialize(source:, base_commit:, dependencies:, files:, credentials:, pr_message_header: nil, pr_message_footer: nil, @@ -60,7 +60,7 @@ def initialize(source:, base_commit:, dependencies:, files:, credentials:, label_language: false, automerge_candidate: false, github_redirection_service: DEFAULT_GITHUB_REDIRECTION_SERVICE, custom_headers: nil, require_up_to_date_base: false, - provider_metadata: {}, message: nil, target_project_id: nil) + provider_metadata: {}, message: nil) @dependencies = dependencies @source = source @base_commit = base_commit @@ -85,7 +85,6 @@ def initialize(source:, base_commit:, dependencies:, files:, credentials:, @require_up_to_date_base = require_up_to_date_base @provider_metadata = provider_metadata @message = message - @target_project_id = target_project_id check_dependencies_have_previous_version end @@ -159,7 +158,7 @@ def gitlab_creator approvers: reviewers, assignees: assignees, milestone: milestone, - target_project_id: target_project_id + target_project_id: provider_metadata[:target_project_id] ) end diff --git a/common/lib/dependabot/pull_request_updater.rb b/common/lib/dependabot/pull_request_updater.rb index e342d1edead..ec9d1137ba9 100644 --- a/common/lib/dependabot/pull_request_updater.rb +++ b/common/lib/dependabot/pull_request_updater.rb @@ -9,12 +9,12 @@ class PullRequestUpdater class BranchProtected < StandardError; end attr_reader :source, :files, :base_commit, :old_commit, :credentials, - :pull_request_number, :author_details, :signature_key, :target_project_id + :pull_request_number, :author_details, :signature_key, :provider_metadata def initialize(source:, base_commit:, old_commit:, files:, credentials:, pull_request_number:, author_details: nil, signature_key: nil, - target_project_id: nil) + provider_metadata: {}) @source = source @base_commit = base_commit @old_commit = old_commit @@ -23,7 +23,7 @@ def initialize(source:, base_commit:, old_commit:, files:, @pull_request_number = pull_request_number @author_details = author_details @signature_key = signature_key - @target_project_id = target_project_id + @provider_metadata = provider_metadata end def update @@ -58,7 +58,7 @@ def gitlab_updater files: files, credentials: credentials, pull_request_number: pull_request_number, - target_project_id: target_project_id + target_project_id: provider_metadata[:target_project_id] ) end diff --git a/common/spec/dependabot/pull_request_creator_spec.rb b/common/spec/dependabot/pull_request_creator_spec.rb index 869b9eaa280..c68fa077099 100644 --- a/common/spec/dependabot/pull_request_creator_spec.rb +++ b/common/spec/dependabot/pull_request_creator_spec.rb @@ -21,8 +21,7 @@ milestone: milestone, author_details: author_details, signature_key: signature_key, - provider_metadata: provider_metadata, - target_project_id: target_project_id + provider_metadata: provider_metadata ) end @@ -49,7 +48,6 @@ let(:files) { [gemfile] } let(:base_commit) { "basecommitsha" } let(:provider_metadata) { nil } - let(:target_project_id) { 1 } let(:credentials) do [{ "type" => "git_source", @@ -213,6 +211,7 @@ context "with a GitLab source" do let(:source) { Dependabot::Source.new(provider: "gitlab", repo: "gc/bp") } let(:dummy_creator) { instance_double(described_class::Gitlab) } + let(:provider_metadata) { { target_project_id: 1 } } it "delegates to PullRequestCreator::Github with correct params" do expect(described_class::Gitlab). @@ -231,7 +230,7 @@ approvers: reviewers, assignees: nil, milestone: milestone, - target_project_id: target_project_id + target_project_id: provider_metadata[:target_project_id] ).and_return(dummy_creator) expect(dummy_creator).to receive(:create) creator.create diff --git a/common/spec/dependabot/pull_request_updater_spec.rb b/common/spec/dependabot/pull_request_updater_spec.rb index dbe2afe7339..e98bec4d667 100644 --- a/common/spec/dependabot/pull_request_updater_spec.rb +++ b/common/spec/dependabot/pull_request_updater_spec.rb @@ -15,7 +15,7 @@ files: files, credentials: credentials, pull_request_number: pull_request_number, - target_project_id: target_project_id + provider_metadata: provider_metadata ) end @@ -26,6 +26,7 @@ let(:pull_request_number) { 1 } let(:credentials) { [] } let(:target_project_id) { 1 } + let(:provider_metadata) { {} } describe "#update" do context "with a GitHub source" do @@ -53,6 +54,7 @@ context "with a Gitlab source" do let(:source) { Dependabot::Source.new(provider: "gitlab", repo: "gc/bp") } let(:dummy_updater) { instance_double(described_class::Gitlab) } + let(:provider_metadata) { { target_project_id: 1 } } it "delegates to PullRequestUpdater::Gitlab with correct params" do expect(described_class::Gitlab). @@ -64,7 +66,7 @@ files: files, credentials: credentials, pull_request_number: pull_request_number, - target_project_id: target_project_id + target_project_id: provider_metadata[:target_project_id] ).and_return(dummy_updater) expect(dummy_updater).to receive(:update) updater.update