From ceff0c91700b15ecf26931c23f40637dcdf7204d Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Tue, 2 Aug 2016 13:41:22 -0300 Subject: [PATCH 1/6] Check out locally PRs where the source/target branch were removed --- lib/gitlab/github_import/importer.rb | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/lib/gitlab/github_import/importer.rb b/lib/gitlab/github_import/importer.rb index 3932fcb1eda..3b501533ffb 100644 --- a/lib/gitlab/github_import/importer.rb +++ b/lib/gitlab/github_import/importer.rb @@ -71,11 +71,12 @@ module Gitlab pull_requests = client.pull_requests(repo, state: :all, sort: :created, direction: :asc, per_page: 100) pull_requests = pull_requests.map { |raw| PullRequestFormatter.new(project, raw) }.select(&:valid?) - source_branches_removed = pull_requests.reject(&:source_branch_exists?).map { |pr| [pr.source_branch_name, pr.source_branch_sha] } + source_branches_removed = pull_requests.reject(&:source_branch_exists?).map { |pr| [pr.source_branch_name, pr.number] } target_branches_removed = pull_requests.reject(&:target_branch_exists?).map { |pr| [pr.target_branch_name, pr.target_branch_sha] } branches_removed = source_branches_removed | target_branches_removed - restore_branches(branches_removed) + restore_source_branches(source_branches_removed) + restore_target_branches(target_branches_removed) pull_requests.each do |pull_request| merge_request = pull_request.create! @@ -120,17 +121,20 @@ module Gitlab end end - def restore_branches(branches) - branches.each do |name, sha| - client.create_ref(repo, "refs/heads/#{name}", sha) + def restore_source_branches(branches) + branches.each do |name, number| + project.repository.fetch_ref(repo_url, "pull/#{number}/head", name) end + end - project.repository.fetch_ref(repo_url, '+refs/heads/*', 'refs/heads/*') + def restore_target_branches(branches) + branches.each do |name, sha| + project.repository.create_branch(name, sha) + end end def clean_up_restored_branches(branches) branches.each do |name, _| - client.delete_ref(repo, "heads/#{name}") project.repository.delete_branch(name) rescue Rugged::ReferenceError end From 51cf14b37c64b1afab25b10dfe94e281ff58d288 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Tue, 2 Aug 2016 13:46:02 -0300 Subject: [PATCH 2/6] Does not need to disable GitHub webhooks since PRs are check out locally --- app/views/import/github/status.html.haml | 4 -- lib/gitlab/github_import/hook_formatter.rb | 23 ------- lib/gitlab/github_import/importer.rb | 31 --------- .../github_import/hook_formatter_spec.rb | 65 ------------------- 4 files changed, 123 deletions(-) delete mode 100644 lib/gitlab/github_import/hook_formatter.rb delete mode 100644 spec/lib/gitlab/github_import/hook_formatter_spec.rb diff --git a/app/views/import/github/status.html.haml b/app/views/import/github/status.html.haml index deaaf9af875..54ff1d27c67 100644 --- a/app/views/import/github/status.html.haml +++ b/app/views/import/github/status.html.haml @@ -4,10 +4,6 @@ %i.fa.fa-github Import projects from GitHub -%p - %i.fa.fa-warning - To import GitHub pull requests, any pull request source branches that had been deleted are temporarily restored on GitHub. To prevent any connected CI services from being overloaded with dozens of irrelevant branches being created and deleted again, GitHub webhooks are temporarily disabled during the import process, but only if you have admin access to the GitHub repository. - %p.light Select projects you want to import. %hr diff --git a/lib/gitlab/github_import/hook_formatter.rb b/lib/gitlab/github_import/hook_formatter.rb deleted file mode 100644 index db1fabaa18a..00000000000 --- a/lib/gitlab/github_import/hook_formatter.rb +++ /dev/null @@ -1,23 +0,0 @@ -module Gitlab - module GithubImport - class HookFormatter - EVENTS = %w[* create delete pull_request push].freeze - - attr_reader :raw - - delegate :id, :name, :active, to: :raw - - def initialize(raw) - @raw = raw - end - - def config - raw.config.attrs - end - - def valid? - (EVENTS & raw.events).any? && active - end - end - end -end diff --git a/lib/gitlab/github_import/importer.rb b/lib/gitlab/github_import/importer.rb index 3b501533ffb..294e44b7124 100644 --- a/lib/gitlab/github_import/importer.rb +++ b/lib/gitlab/github_import/importer.rb @@ -66,8 +66,6 @@ module Gitlab end def import_pull_requests - disable_webhooks - pull_requests = client.pull_requests(repo, state: :all, sort: :created, direction: :asc, per_page: 100) pull_requests = pull_requests.map { |raw| PullRequestFormatter.new(project, raw) }.select(&:valid?) @@ -90,35 +88,6 @@ module Gitlab raise Projects::ImportService::Error, e.message ensure clean_up_restored_branches(branches_removed) - clean_up_disabled_webhooks - end - - def disable_webhooks - update_webhooks(hooks, active: false) - end - - def clean_up_disabled_webhooks - update_webhooks(hooks, active: true) - end - - def update_webhooks(hooks, options) - hooks.each do |hook| - client.edit_hook(repo, hook.id, hook.name, hook.config, options) - end - end - - def hooks - @hooks ||= - begin - client.hooks(repo).map { |raw| HookFormatter.new(raw) }.select(&:valid?) - - # The GitHub Repository Webhooks API returns 404 for users - # without admin access to the repository when listing hooks. - # In this case we just want to return gracefully instead of - # spitting out an error and stop the import process. - rescue Octokit::NotFound - [] - end end def restore_source_branches(branches) diff --git a/spec/lib/gitlab/github_import/hook_formatter_spec.rb b/spec/lib/gitlab/github_import/hook_formatter_spec.rb deleted file mode 100644 index 110ba428258..00000000000 --- a/spec/lib/gitlab/github_import/hook_formatter_spec.rb +++ /dev/null @@ -1,65 +0,0 @@ -require 'spec_helper' - -describe Gitlab::GithubImport::HookFormatter, lib: true do - describe '#id' do - it 'returns raw id' do - raw = double(id: 100000) - formatter = described_class.new(raw) - expect(formatter.id).to eq 100000 - end - end - - describe '#name' do - it 'returns raw id' do - raw = double(name: 'web') - formatter = described_class.new(raw) - expect(formatter.name).to eq 'web' - end - end - - describe '#config' do - it 'returns raw config.attrs' do - raw = double(config: double(attrs: { url: 'http://something.com/webhook' })) - formatter = described_class.new(raw) - expect(formatter.config).to eq({ url: 'http://something.com/webhook' }) - end - end - - describe '#valid?' do - it 'returns true when events contains the wildcard event' do - raw = double(events: ['*', 'commit_comment'], active: true) - formatter = described_class.new(raw) - expect(formatter.valid?).to eq true - end - - it 'returns true when events contains the create event' do - raw = double(events: ['create', 'commit_comment'], active: true) - formatter = described_class.new(raw) - expect(formatter.valid?).to eq true - end - - it 'returns true when events contains delete event' do - raw = double(events: ['delete', 'commit_comment'], active: true) - formatter = described_class.new(raw) - expect(formatter.valid?).to eq true - end - - it 'returns true when events contains pull_request event' do - raw = double(events: ['pull_request', 'commit_comment'], active: true) - formatter = described_class.new(raw) - expect(formatter.valid?).to eq true - end - - it 'returns false when events does not contains branch related events' do - raw = double(events: ['member', 'commit_comment'], active: true) - formatter = described_class.new(raw) - expect(formatter.valid?).to eq false - end - - it 'returns false when hook is not active' do - raw = double(events: ['pull_request', 'commit_comment'], active: false) - formatter = described_class.new(raw) - expect(formatter.valid?).to eq false - end - end -end From 1e736fb5272b75eb363a1ef766e29d35d53babb6 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Tue, 2 Aug 2016 23:25:45 -0300 Subject: [PATCH 3/6] Allow users to import cross-repository pull requests from GitHub --- doc/workflow/importing/import_projects_from_github.md | 3 --- lib/gitlab/github_import/pull_request_formatter.rb | 6 +----- .../gitlab/github_import/pull_request_formatter_spec.rb | 8 ++++---- 3 files changed, 5 insertions(+), 12 deletions(-) diff --git a/doc/workflow/importing/import_projects_from_github.md b/doc/workflow/importing/import_projects_from_github.md index a2b2a4b88f9..306caabf6e6 100644 --- a/doc/workflow/importing/import_projects_from_github.md +++ b/doc/workflow/importing/import_projects_from_github.md @@ -18,9 +18,6 @@ At its current state, GitHub importer can import: With GitLab 8.7+, references to pull requests and issues are preserved. -It is not yet possible to import your cross-repository pull requests (those from -forks). We are working on improving this in the near future. - The importer page is visible when you [create a new project][new-project]. Click on the **GitHub** link and, if you are logged in via the GitHub integration, you will be redirected to GitHub for permission to access your diff --git a/lib/gitlab/github_import/pull_request_formatter.rb b/lib/gitlab/github_import/pull_request_formatter.rb index a4ea2210abd..f7f8a4ce984 100644 --- a/lib/gitlab/github_import/pull_request_formatter.rb +++ b/lib/gitlab/github_import/pull_request_formatter.rb @@ -33,7 +33,7 @@ module Gitlab end def valid? - source_branch.valid? && target_branch.valid? && !cross_project? + source_branch.valid? && target_branch.valid? end def source_branch @@ -68,10 +68,6 @@ module Gitlab raw_data.body || "" end - def cross_project? - source_branch_repo.id != target_branch_repo.id - end - def description formatter.author_line(author) + body end diff --git a/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb b/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb index 79931ecd134..ad79715a2d4 100644 --- a/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb +++ b/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb @@ -178,8 +178,8 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do let(:source_repo) { double(id: 2) } let(:raw_data) { double(base_data) } - it 'returns false' do - expect(pull_request.valid?).to eq false + it 'returns true' do + expect(pull_request.valid?).to eq true end end @@ -187,8 +187,8 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do let(:target_repo) { double(id: 2) } let(:raw_data) { double(base_data) } - it 'returns false' do - expect(pull_request.valid?).to eq false + it 'returns true' do + expect(pull_request.valid?).to eq true end end end From 167a0c7f0bd88e565f580e144b59d9eebe7e24f6 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Thu, 4 Aug 2016 17:15:00 -0300 Subject: [PATCH 4/6] Remove SHA suffix for removed branches name when importing PR from GH --- lib/gitlab/github_import/branch_formatter.rb | 2 +- lib/gitlab/github_import/importer.rb | 54 +++++++++---------- .../github_import/branch_formatter_spec.rb | 8 +-- 3 files changed, 28 insertions(+), 36 deletions(-) diff --git a/lib/gitlab/github_import/branch_formatter.rb b/lib/gitlab/github_import/branch_formatter.rb index 7d2d545b84e..4be4fc8fe37 100644 --- a/lib/gitlab/github_import/branch_formatter.rb +++ b/lib/gitlab/github_import/branch_formatter.rb @@ -8,7 +8,7 @@ module Gitlab end def name - @name ||= exists? ? ref : "#{ref}-#{short_id}" + ref end def valid? diff --git a/lib/gitlab/github_import/importer.rb b/lib/gitlab/github_import/importer.rb index 294e44b7124..9ddc8905bd6 100644 --- a/lib/gitlab/github_import/importer.rb +++ b/lib/gitlab/github_import/importer.rb @@ -12,7 +12,6 @@ module Gitlab if credentials @client = Client.new(credentials[:user]) - @formatter = Gitlab::ImportFormatter.new else raise Projects::ImportService::Error, "Unable to find project import data credentials for project ID: #{@project.id}" end @@ -69,43 +68,42 @@ module Gitlab pull_requests = client.pull_requests(repo, state: :all, sort: :created, direction: :asc, per_page: 100) pull_requests = pull_requests.map { |raw| PullRequestFormatter.new(project, raw) }.select(&:valid?) - source_branches_removed = pull_requests.reject(&:source_branch_exists?).map { |pr| [pr.source_branch_name, pr.number] } - target_branches_removed = pull_requests.reject(&:target_branch_exists?).map { |pr| [pr.target_branch_name, pr.target_branch_sha] } - branches_removed = source_branches_removed | target_branches_removed - - restore_source_branches(source_branches_removed) - restore_target_branches(target_branches_removed) - pull_requests.each do |pull_request| - merge_request = pull_request.create! - apply_labels(merge_request) - import_comments(merge_request) - import_comments_on_diff(merge_request) + begin + restore_source_branch(pull_request) unless pull_request.source_branch_exists? + restore_target_branch(pull_request) unless pull_request.target_branch_exists? + + merge_request = pull_request.create! + apply_labels(merge_request) + import_comments(merge_request) + import_comments_on_diff(merge_request) + rescue ActiveRecord::RecordInvalid => e + raise Projects::ImportService::Error, e.message + ensure + clean_up_restored_branches(pull_request) + end end true - rescue ActiveRecord::RecordInvalid => e - raise Projects::ImportService::Error, e.message - ensure - clean_up_restored_branches(branches_removed) end - def restore_source_branches(branches) - branches.each do |name, number| - project.repository.fetch_ref(repo_url, "pull/#{number}/head", name) - end + def restore_source_branch(pull_request) + project.repository.fetch_ref(repo_url, "pull/#{pull_request.number}/head", pull_request.source_branch_name) end - def restore_target_branches(branches) - branches.each do |name, sha| - project.repository.create_branch(name, sha) - end + def restore_target_branch(pull_request) + project.repository.create_branch(pull_request.target_branch_name, pull_request.target_branch_sha) end - def clean_up_restored_branches(branches) - branches.each do |name, _| - project.repository.delete_branch(name) rescue Rugged::ReferenceError - end + def remove_branch(name) + project.repository.delete_branch(name) + rescue Rugged::ReferenceError + nil + end + + def clean_up_restored_branches(pull_request) + remove_branch(pull_request.source_branch_name) unless pull_request.source_branch_exists? + remove_branch(pull_request.target_branch_name) unless pull_request.target_branch_exists? project.repository.after_remove_branch end diff --git a/spec/lib/gitlab/github_import/branch_formatter_spec.rb b/spec/lib/gitlab/github_import/branch_formatter_spec.rb index fc9d5204148..e01a80054a3 100644 --- a/spec/lib/gitlab/github_import/branch_formatter_spec.rb +++ b/spec/lib/gitlab/github_import/branch_formatter_spec.rb @@ -33,17 +33,11 @@ describe Gitlab::GithubImport::BranchFormatter, lib: true do end describe '#name' do - it 'returns raw ref when branch exists' do + it 'returns raw ref' do branch = described_class.new(project, double(raw)) expect(branch.name).to eq 'feature' end - - it 'returns formatted ref when branch does not exist' do - branch = described_class.new(project, double(raw.merge(ref: 'removed-branch', sha: '2e5d3239642f9161dcbbc4b70a211a68e5e45e2b'))) - - expect(branch.name).to eq 'removed-branch-2e5d3239' - end end describe '#repo' do From 44a196f8bc978ec09256abdc67c7e331b07cb7c3 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Tue, 2 Aug 2016 23:27:43 -0300 Subject: [PATCH 5/6] Update CHANGELOG --- CHANGELOG | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG b/CHANGELOG index 77bcea54cf9..d08d7790568 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -39,6 +39,7 @@ v 8.11.0 (unreleased) - Make fork counter always clickable. !5463 (winniehell) - Gitlab::Highlight is now instrumented - All created issues, API or WebUI, can be submitted to Akismet for spam check !5333 + - Allow users to import cross-repository pull requests from GitHub - The overhead of instrumented method calls has been reduced - Remove `search_id` of labels dropdown filter to fix 'Missleading URI for labels in Merge Requests and Issues view'. !5368 (Scott Le) - Load project invited groups and members eagerly in `ProjectTeam#fetch_members` From 0a1535a9f44b12accdf3d6585ff7fee53737da51 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Mon, 8 Aug 2016 20:24:40 -0300 Subject: [PATCH 6/6] Prefixes removed branches name with PR number when importing PR from GH --- lib/gitlab/github_import/branch_formatter.rb | 4 -- .../github_import/pull_request_formatter.rb | 16 +++++++- .../github_import/branch_formatter_spec.rb | 8 ---- .../pull_request_formatter_spec.rb | 37 +++++++++++++++++++ 4 files changed, 51 insertions(+), 14 deletions(-) diff --git a/lib/gitlab/github_import/branch_formatter.rb b/lib/gitlab/github_import/branch_formatter.rb index 4be4fc8fe37..4750675ae9d 100644 --- a/lib/gitlab/github_import/branch_formatter.rb +++ b/lib/gitlab/github_import/branch_formatter.rb @@ -7,10 +7,6 @@ module Gitlab branch_exists? && commit_exists? end - def name - ref - end - def valid? repo.present? end diff --git a/lib/gitlab/github_import/pull_request_formatter.rb b/lib/gitlab/github_import/pull_request_formatter.rb index f7f8a4ce984..b84538a090a 100644 --- a/lib/gitlab/github_import/pull_request_formatter.rb +++ b/lib/gitlab/github_import/pull_request_formatter.rb @@ -1,8 +1,8 @@ module Gitlab module GithubImport class PullRequestFormatter < BaseFormatter - delegate :exists?, :name, :project, :repo, :sha, to: :source_branch, prefix: true - delegate :exists?, :name, :project, :repo, :sha, to: :target_branch, prefix: true + delegate :exists?, :project, :ref, :repo, :sha, to: :source_branch, prefix: true + delegate :exists?, :project, :ref, :repo, :sha, to: :target_branch, prefix: true def attributes { @@ -40,10 +40,22 @@ module Gitlab @source_branch ||= BranchFormatter.new(project, raw_data.head) end + def source_branch_name + @source_branch_name ||= begin + source_branch_exists? ? source_branch_ref : "pull/#{number}/#{source_branch_ref}" + end + end + def target_branch @target_branch ||= BranchFormatter.new(project, raw_data.base) end + def target_branch_name + @target_branch_name ||= begin + target_branch_exists? ? target_branch_ref : "pull/#{number}/#{target_branch_ref}" + end + end + private def assigned? diff --git a/spec/lib/gitlab/github_import/branch_formatter_spec.rb b/spec/lib/gitlab/github_import/branch_formatter_spec.rb index e01a80054a3..e5300dbba1e 100644 --- a/spec/lib/gitlab/github_import/branch_formatter_spec.rb +++ b/spec/lib/gitlab/github_import/branch_formatter_spec.rb @@ -32,14 +32,6 @@ describe Gitlab::GithubImport::BranchFormatter, lib: true do end end - describe '#name' do - it 'returns raw ref' do - branch = described_class.new(project, double(raw)) - - expect(branch.name).to eq 'feature' - end - end - describe '#repo' do it 'returns raw repo' do branch = described_class.new(project, double(raw)) diff --git a/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb b/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb index ad79715a2d4..aa28e360993 100644 --- a/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb +++ b/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb @@ -9,6 +9,7 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do let(:source_branch) { double(ref: 'feature', repo: source_repo, sha: source_sha) } let(:target_repo) { repository } let(:target_branch) { double(ref: 'master', repo: target_repo, sha: target_sha) } + let(:removed_branch) { double(ref: 'removed-branch', repo: source_repo, sha: '2e5d3239642f9161dcbbc4b70a211a68e5e45e2b') } let(:octocat) { double(id: 123456, login: 'octocat') } let(:created_at) { DateTime.strptime('2011-01-26T19:01:12Z') } let(:updated_at) { DateTime.strptime('2011-01-27T19:01:12Z') } @@ -165,6 +166,42 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do end end + describe '#source_branch_name' do + context 'when source branch exists' do + let(:raw_data) { double(base_data) } + + it 'returns branch ref' do + expect(pull_request.source_branch_name).to eq 'feature' + end + end + + context 'when source branch does not exist' do + let(:raw_data) { double(base_data.merge(head: removed_branch)) } + + it 'prefixes branch name with pull request number' do + expect(pull_request.source_branch_name).to eq 'pull/1347/removed-branch' + end + end + end + + describe '#target_branch_name' do + context 'when source branch exists' do + let(:raw_data) { double(base_data) } + + it 'returns branch ref' do + expect(pull_request.target_branch_name).to eq 'master' + end + end + + context 'when target branch does not exist' do + let(:raw_data) { double(base_data.merge(base: removed_branch)) } + + it 'prefixes branch name with pull request number' do + expect(pull_request.target_branch_name).to eq 'pull/1347/removed-branch' + end + end + end + describe '#valid?' do context 'when source, and target repos are not a fork' do let(:raw_data) { double(base_data) }