Commit dd9cd24e authored by Sean McGivern's avatar Sean McGivern

Merge branch 'feature/24072-improve-importing-of-github-pull-requests' into 'master'

Improve importing of github pull requests

Closes #24072

See merge request !7241
parents da9b6432 3228798b
---
title: Reduce API calls needed when importing issues and pull requests from GitHub
merge_request: 7241
author: Andrew Smith (EspadaV8)
...@@ -20,10 +20,18 @@ module Gitlab ...@@ -20,10 +20,18 @@ module Gitlab
end end
def execute def execute
# The ordering of importing is important here due to the way GitHub structures their data
# 1. Labels are required by other items while not having a dependency on anything else
# so need to be first
# 2. Pull requests must come before issues. Every pull request is also an issue but not
# all issues are pull requests. Only the issue entity has labels defined in GitHub. GitLab
# doesn't structure data like this so we need to make sure that we've created the MRs
# before we attempt to add the labels defined in the GitHub issue for the related, already
# imported, pull request
import_labels import_labels
import_milestones import_milestones
import_issues
import_pull_requests import_pull_requests
import_issues
import_comments(:issues) import_comments(:issues)
import_comments(:pull_requests) import_comments(:pull_requests)
import_wiki import_wiki
...@@ -79,13 +87,17 @@ module Gitlab ...@@ -79,13 +87,17 @@ module Gitlab
issues.each do |raw| issues.each do |raw|
gh_issue = IssueFormatter.new(project, raw) gh_issue = IssueFormatter.new(project, raw)
if gh_issue.valid? begin
begin issuable =
issue = gh_issue.create! if gh_issue.pull_request?
apply_labels(issue, raw) MergeRequest.find_by_iid(gh_issue.number)
rescue => e else
errors << { type: :issue, url: Gitlab::UrlSanitizer.sanitize(raw.url), errors: e.message } gh_issue.create!
end end
apply_labels(issuable, raw)
rescue => e
errors << { type: :issue, url: Gitlab::UrlSanitizer.sanitize(raw.url), errors: e.message }
end end
end end
end end
...@@ -101,8 +113,7 @@ module Gitlab ...@@ -101,8 +113,7 @@ module Gitlab
restore_source_branch(pull_request) unless pull_request.source_branch_exists? restore_source_branch(pull_request) unless pull_request.source_branch_exists?
restore_target_branch(pull_request) unless pull_request.target_branch_exists? restore_target_branch(pull_request) unless pull_request.target_branch_exists?
merge_request = pull_request.create! pull_request.create!
apply_labels(merge_request, raw)
rescue => e rescue => e
errors << { type: :pull_request, url: Gitlab::UrlSanitizer.sanitize(pull_request.url), errors: e.message } errors << { type: :pull_request, url: Gitlab::UrlSanitizer.sanitize(pull_request.url), errors: e.message }
ensure ensure
...@@ -133,21 +144,14 @@ module Gitlab ...@@ -133,21 +144,14 @@ module Gitlab
remove_branch(pull_request.target_branch_name) unless pull_request.target_branch_exists? remove_branch(pull_request.target_branch_name) unless pull_request.target_branch_exists?
end end
def apply_labels(issuable, raw_issuable) def apply_labels(issuable, raw)
# GH returns labels for issues but not for pull requests! return unless raw.labels.count > 0
labels = if issuable.is_a?(MergeRequest)
client.labels_for_issue(repo, raw_issuable.number)
else
raw_issuable.labels
end
if labels.count > 0 label_ids = raw.labels
label_ids = labels .map { |attrs| @labels[attrs.name] }
.map { |attrs| @labels[attrs.name] } .compact
.compact
issuable.update_attribute(:label_ids, label_ids) issuable.update_attribute(:label_ids, label_ids)
end
end end
def import_comments(issuable_type) def import_comments(issuable_type)
......
...@@ -32,8 +32,8 @@ module Gitlab ...@@ -32,8 +32,8 @@ module Gitlab
raw_data.number raw_data.number
end end
def valid? def pull_request?
raw_data.pull_request.nil? raw_data.pull_request.present?
end end
private private
......
...@@ -101,7 +101,6 @@ describe Gitlab::GithubImport::Importer, lib: true do ...@@ -101,7 +101,6 @@ describe Gitlab::GithubImport::Importer, lib: true do
closed_at: nil, closed_at: nil,
merged_at: nil, merged_at: nil,
url: 'https://api.github.com/repos/octocat/Hello-World/pulls/1347', url: 'https://api.github.com/repos/octocat/Hello-World/pulls/1347',
labels: [double(name: 'Label #3')],
) )
end end
...@@ -157,8 +156,6 @@ describe Gitlab::GithubImport::Importer, lib: true do ...@@ -157,8 +156,6 @@ describe Gitlab::GithubImport::Importer, lib: true do
errors: [ errors: [
{ type: :label, url: "https://api.github.com/repos/octocat/Hello-World/labels/bug", errors: "Validation failed: Title can't be blank, Title is invalid" }, { type: :label, url: "https://api.github.com/repos/octocat/Hello-World/labels/bug", errors: "Validation failed: Title can't be blank, Title is invalid" },
{ type: :issue, url: "https://api.github.com/repos/octocat/Hello-World/issues/1348", errors: "Validation failed: Title can't be blank, Title is too short (minimum is 0 characters)" }, { type: :issue, url: "https://api.github.com/repos/octocat/Hello-World/issues/1348", errors: "Validation failed: Title can't be blank, Title is too short (minimum is 0 characters)" },
{ type: :pull_request, url: "https://api.github.com/repos/octocat/Hello-World/pulls/1347", errors: "Invalid Repository. Use user/repo format." },
{ type: :pull_request, url: "https://api.github.com/repos/octocat/Hello-World/pulls/1347", errors: "Invalid Repository. Use user/repo format." },
{ type: :wiki, errors: "Gitlab::Shell::Error" }, { type: :wiki, errors: "Gitlab::Shell::Error" },
{ type: :release, url: 'https://api.github.com/repos/octocat/Hello-World/releases/2', errors: "Validation failed: Description can't be blank" } { type: :release, url: 'https://api.github.com/repos/octocat/Hello-World/releases/2', errors: "Validation failed: Description can't be blank" }
] ]
......
...@@ -144,20 +144,20 @@ describe Gitlab::GithubImport::IssueFormatter, lib: true do ...@@ -144,20 +144,20 @@ describe Gitlab::GithubImport::IssueFormatter, lib: true do
end end
end end
describe '#valid?' do describe '#pull_request?' do
context 'when mention a pull request' do context 'when mention a pull request' do
let(:raw_data) { double(base_data.merge(pull_request: double)) } let(:raw_data) { double(base_data.merge(pull_request: double)) }
it 'returns false' do it 'returns true' do
expect(issue.valid?).to eq false expect(issue.pull_request?).to eq true
end end
end end
context 'when does not mention a pull request' do context 'when does not mention a pull request' do
let(:raw_data) { double(base_data.merge(pull_request: nil)) } let(:raw_data) { double(base_data.merge(pull_request: nil)) }
it 'returns true' do it 'returns false' do
expect(issue.valid?).to eq true expect(issue.pull_request?).to eq false
end end
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment