Merge branch 'github-importer' into 'master'
Does not halt the GitHub import process when an error occurs ## What are the relevant issue numbers? Fixes #20385 https://gitlab.com/gitlab-org/gitlab-ce/issues/20149 ## Does this MR meet the acceptance criteria? - [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added - Tests - [x] All builds are passing - [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides) - [x] Branch has no merge conflicts with `master` (if you do - rebase it please) - [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits) See merge request !5763
This commit is contained in:
commit
291fd85880
|
@ -12,6 +12,7 @@ v 8.11.2 (unreleased)
|
||||||
- Show "Create Merge Request" widget for push events to fork projects on the source project
|
- Show "Create Merge Request" widget for push events to fork projects on the source project
|
||||||
|
|
||||||
v 8.11.1 (unreleased)
|
v 8.11.1 (unreleased)
|
||||||
|
- Does not halt the GitHub import process when an error occurs
|
||||||
- Fix file links on project page when default view is Files !5933
|
- Fix file links on project page when default view is Files !5933
|
||||||
|
|
||||||
v 8.11.0
|
v 8.11.0
|
||||||
|
|
|
@ -471,8 +471,6 @@ class Project < ActiveRecord::Base
|
||||||
end
|
end
|
||||||
|
|
||||||
def reset_cache_and_import_attrs
|
def reset_cache_and_import_attrs
|
||||||
update(import_error: nil)
|
|
||||||
|
|
||||||
ProjectCacheWorker.perform_async(self.id)
|
ProjectCacheWorker.perform_async(self.id)
|
||||||
|
|
||||||
self.import_data.destroy if self.import_data
|
self.import_data.destroy if self.import_data
|
||||||
|
|
|
@ -14,6 +14,8 @@ class RepositoryImportWorker
|
||||||
import_url: @project.import_url,
|
import_url: @project.import_url,
|
||||||
path: @project.path_with_namespace)
|
path: @project.path_with_namespace)
|
||||||
|
|
||||||
|
project.update_column(:import_error, nil)
|
||||||
|
|
||||||
result = Projects::ImportService.new(project, current_user).execute
|
result = Projects::ImportService.new(project, current_user).execute
|
||||||
|
|
||||||
if result[:status] == :error
|
if result[:status] == :error
|
||||||
|
|
|
@ -3,12 +3,13 @@ module Gitlab
|
||||||
class Importer
|
class Importer
|
||||||
include Gitlab::ShellAdapter
|
include Gitlab::ShellAdapter
|
||||||
|
|
||||||
attr_reader :client, :project, :repo, :repo_url
|
attr_reader :client, :errors, :project, :repo, :repo_url
|
||||||
|
|
||||||
def initialize(project)
|
def initialize(project)
|
||||||
@project = project
|
@project = project
|
||||||
@repo = project.import_source
|
@repo = project.import_source
|
||||||
@repo_url = project.import_url
|
@repo_url = project.import_url
|
||||||
|
@errors = []
|
||||||
|
|
||||||
if credentials
|
if credentials
|
||||||
@client = Client.new(credentials[:user])
|
@client = Client.new(credentials[:user])
|
||||||
|
@ -18,8 +19,14 @@ module Gitlab
|
||||||
end
|
end
|
||||||
|
|
||||||
def execute
|
def execute
|
||||||
import_labels && import_milestones && import_issues &&
|
import_labels
|
||||||
import_pull_requests && import_wiki
|
import_milestones
|
||||||
|
import_issues
|
||||||
|
import_pull_requests
|
||||||
|
import_wiki
|
||||||
|
handle_errors
|
||||||
|
|
||||||
|
true
|
||||||
end
|
end
|
||||||
|
|
||||||
private
|
private
|
||||||
|
@ -28,22 +35,37 @@ module Gitlab
|
||||||
@credentials ||= project.import_data.credentials if project.import_data
|
@credentials ||= project.import_data.credentials if project.import_data
|
||||||
end
|
end
|
||||||
|
|
||||||
|
def handle_errors
|
||||||
|
return unless errors.any?
|
||||||
|
|
||||||
|
project.update_column(:import_error, {
|
||||||
|
message: 'The remote data could not be fully imported.',
|
||||||
|
errors: errors
|
||||||
|
}.to_json)
|
||||||
|
end
|
||||||
|
|
||||||
def import_labels
|
def import_labels
|
||||||
labels = client.labels(repo, per_page: 100)
|
labels = client.labels(repo, per_page: 100)
|
||||||
labels.each { |raw| LabelFormatter.new(project, raw).create! }
|
|
||||||
|
|
||||||
true
|
labels.each do |raw|
|
||||||
rescue ActiveRecord::RecordInvalid => e
|
begin
|
||||||
raise Projects::ImportService::Error, e.message
|
LabelFormatter.new(project, raw).create!
|
||||||
|
rescue => e
|
||||||
|
errors << { type: :label, url: Gitlab::UrlSanitizer.sanitize(raw.url), errors: e.message }
|
||||||
|
end
|
||||||
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
def import_milestones
|
def import_milestones
|
||||||
milestones = client.milestones(repo, state: :all, per_page: 100)
|
milestones = client.milestones(repo, state: :all, per_page: 100)
|
||||||
milestones.each { |raw| MilestoneFormatter.new(project, raw).create! }
|
|
||||||
|
|
||||||
true
|
milestones.each do |raw|
|
||||||
rescue ActiveRecord::RecordInvalid => e
|
begin
|
||||||
raise Projects::ImportService::Error, e.message
|
MilestoneFormatter.new(project, raw).create!
|
||||||
|
rescue => e
|
||||||
|
errors << { type: :milestone, url: Gitlab::UrlSanitizer.sanitize(raw.url), errors: e.message }
|
||||||
|
end
|
||||||
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
def import_issues
|
def import_issues
|
||||||
|
@ -53,15 +75,15 @@ module Gitlab
|
||||||
gh_issue = IssueFormatter.new(project, raw)
|
gh_issue = IssueFormatter.new(project, raw)
|
||||||
|
|
||||||
if gh_issue.valid?
|
if gh_issue.valid?
|
||||||
issue = gh_issue.create!
|
begin
|
||||||
apply_labels(issue)
|
issue = gh_issue.create!
|
||||||
import_comments(issue) if gh_issue.has_comments?
|
apply_labels(issue)
|
||||||
|
import_comments(issue) if gh_issue.has_comments?
|
||||||
|
rescue => e
|
||||||
|
errors << { type: :issue, url: Gitlab::UrlSanitizer.sanitize(raw.url), errors: e.message }
|
||||||
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
true
|
|
||||||
rescue ActiveRecord::RecordInvalid => e
|
|
||||||
raise Projects::ImportService::Error, e.message
|
|
||||||
end
|
end
|
||||||
|
|
||||||
def import_pull_requests
|
def import_pull_requests
|
||||||
|
@ -77,14 +99,12 @@ module Gitlab
|
||||||
apply_labels(merge_request)
|
apply_labels(merge_request)
|
||||||
import_comments(merge_request)
|
import_comments(merge_request)
|
||||||
import_comments_on_diff(merge_request)
|
import_comments_on_diff(merge_request)
|
||||||
rescue ActiveRecord::RecordInvalid => e
|
rescue => e
|
||||||
raise Projects::ImportService::Error, e.message
|
errors << { type: :pull_request, url: Gitlab::UrlSanitizer.sanitize(pull_request.url), errors: e.message }
|
||||||
ensure
|
ensure
|
||||||
clean_up_restored_branches(pull_request)
|
clean_up_restored_branches(pull_request)
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
true
|
|
||||||
end
|
end
|
||||||
|
|
||||||
def restore_source_branch(pull_request)
|
def restore_source_branch(pull_request)
|
||||||
|
@ -98,7 +118,7 @@ module Gitlab
|
||||||
def remove_branch(name)
|
def remove_branch(name)
|
||||||
project.repository.delete_branch(name)
|
project.repository.delete_branch(name)
|
||||||
rescue Rugged::ReferenceError
|
rescue Rugged::ReferenceError
|
||||||
nil
|
errors << { type: :remove_branch, name: name }
|
||||||
end
|
end
|
||||||
|
|
||||||
def clean_up_restored_branches(pull_request)
|
def clean_up_restored_branches(pull_request)
|
||||||
|
@ -112,9 +132,10 @@ module Gitlab
|
||||||
issue = client.issue(repo, issuable.iid)
|
issue = client.issue(repo, issuable.iid)
|
||||||
|
|
||||||
if issue.labels.count > 0
|
if issue.labels.count > 0
|
||||||
label_ids = issue.labels.map do |raw|
|
label_ids = issue.labels
|
||||||
Label.find_by(LabelFormatter.new(project, raw).attributes).try(:id)
|
.map { |raw| LabelFormatter.new(project, raw).attributes }
|
||||||
end
|
.map { |attrs| Label.find_by(attrs).try(:id) }
|
||||||
|
.compact
|
||||||
|
|
||||||
issuable.update_attribute(:label_ids, label_ids)
|
issuable.update_attribute(:label_ids, label_ids)
|
||||||
end
|
end
|
||||||
|
@ -132,8 +153,12 @@ module Gitlab
|
||||||
|
|
||||||
def create_comments(issuable, comments)
|
def create_comments(issuable, comments)
|
||||||
comments.each do |raw|
|
comments.each do |raw|
|
||||||
comment = CommentFormatter.new(project, raw)
|
begin
|
||||||
issuable.notes.create!(comment.attributes)
|
comment = CommentFormatter.new(project, raw)
|
||||||
|
issuable.notes.create!(comment.attributes)
|
||||||
|
rescue => e
|
||||||
|
errors << { type: :comment, url: Gitlab::UrlSanitizer.sanitize(raw.url), errors: e.message }
|
||||||
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
@ -143,16 +168,12 @@ module Gitlab
|
||||||
gitlab_shell.import_repository(project.repository_storage_path, wiki.path_with_namespace, wiki.import_url)
|
gitlab_shell.import_repository(project.repository_storage_path, wiki.path_with_namespace, wiki.import_url)
|
||||||
project.update_attribute(:wiki_enabled, true)
|
project.update_attribute(:wiki_enabled, true)
|
||||||
end
|
end
|
||||||
|
|
||||||
true
|
|
||||||
rescue Gitlab::Shell::Error => e
|
rescue Gitlab::Shell::Error => e
|
||||||
# GitHub error message when the wiki repo has not been created,
|
# GitHub error message when the wiki repo has not been created,
|
||||||
# this means that repo has wiki enabled, but have no pages. So,
|
# this means that repo has wiki enabled, but have no pages. So,
|
||||||
# we can skip the import.
|
# we can skip the import.
|
||||||
if e.message !~ /repository not exported/
|
if e.message !~ /repository not exported/
|
||||||
raise Projects::ImportService::Error, e.message
|
errors << { type: :wiki, errors: e.message }
|
||||||
else
|
|
||||||
true
|
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
|
@ -56,6 +56,10 @@ module Gitlab
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
def url
|
||||||
|
raw_data.url
|
||||||
|
end
|
||||||
|
|
||||||
private
|
private
|
||||||
|
|
||||||
def assigned?
|
def assigned?
|
||||||
|
|
|
@ -0,0 +1,132 @@
|
||||||
|
require 'spec_helper'
|
||||||
|
|
||||||
|
describe Gitlab::GithubImport::Importer, lib: true do
|
||||||
|
describe '#execute' do
|
||||||
|
context 'when an error occurs' do
|
||||||
|
let(:project) { create(:project, import_url: 'https://github.com/octocat/Hello-World.git', wiki_enabled: false) }
|
||||||
|
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') }
|
||||||
|
let(:repository) { double(id: 1, fork: false) }
|
||||||
|
let(:source_sha) { create(:commit, project: project).id }
|
||||||
|
let(:source_branch) { double(ref: 'feature', repo: repository, sha: source_sha) }
|
||||||
|
let(:target_sha) { create(:commit, project: project, git_commit: RepoHelpers.another_sample_commit).id }
|
||||||
|
let(:target_branch) { double(ref: 'master', repo: repository, sha: target_sha) }
|
||||||
|
|
||||||
|
let(:label) do
|
||||||
|
double(
|
||||||
|
name: 'Bug',
|
||||||
|
color: 'ff0000',
|
||||||
|
url: 'https://api.github.com/repos/octocat/Hello-World/labels/bug'
|
||||||
|
)
|
||||||
|
end
|
||||||
|
|
||||||
|
let(:milestone) do
|
||||||
|
double(
|
||||||
|
number: 1347,
|
||||||
|
state: 'open',
|
||||||
|
title: '1.0',
|
||||||
|
description: 'Version 1.0',
|
||||||
|
due_on: nil,
|
||||||
|
created_at: created_at,
|
||||||
|
updated_at: updated_at,
|
||||||
|
closed_at: nil,
|
||||||
|
url: 'https://api.github.com/repos/octocat/Hello-World/milestones/1'
|
||||||
|
)
|
||||||
|
end
|
||||||
|
|
||||||
|
let(:issue1) do
|
||||||
|
double(
|
||||||
|
number: 1347,
|
||||||
|
milestone: nil,
|
||||||
|
state: 'open',
|
||||||
|
title: 'Found a bug',
|
||||||
|
body: "I'm having a problem with this.",
|
||||||
|
assignee: nil,
|
||||||
|
user: octocat,
|
||||||
|
comments: 0,
|
||||||
|
pull_request: nil,
|
||||||
|
created_at: created_at,
|
||||||
|
updated_at: updated_at,
|
||||||
|
closed_at: nil,
|
||||||
|
url: 'https://api.github.com/repos/octocat/Hello-World/issues/1347'
|
||||||
|
)
|
||||||
|
end
|
||||||
|
|
||||||
|
let(:issue2) do
|
||||||
|
double(
|
||||||
|
number: 1348,
|
||||||
|
milestone: nil,
|
||||||
|
state: 'open',
|
||||||
|
title: nil,
|
||||||
|
body: "I'm having a problem with this.",
|
||||||
|
assignee: nil,
|
||||||
|
user: octocat,
|
||||||
|
comments: 0,
|
||||||
|
pull_request: nil,
|
||||||
|
created_at: created_at,
|
||||||
|
updated_at: updated_at,
|
||||||
|
closed_at: nil,
|
||||||
|
url: 'https://api.github.com/repos/octocat/Hello-World/issues/1348'
|
||||||
|
)
|
||||||
|
end
|
||||||
|
|
||||||
|
let(:pull_request) do
|
||||||
|
double(
|
||||||
|
number: 1347,
|
||||||
|
milestone: nil,
|
||||||
|
state: 'open',
|
||||||
|
title: 'New feature',
|
||||||
|
body: 'Please pull these awesome changes',
|
||||||
|
head: source_branch,
|
||||||
|
base: target_branch,
|
||||||
|
assignee: nil,
|
||||||
|
user: octocat,
|
||||||
|
created_at: created_at,
|
||||||
|
updated_at: updated_at,
|
||||||
|
closed_at: nil,
|
||||||
|
merged_at: nil,
|
||||||
|
url: 'https://api.github.com/repos/octocat/Hello-World/pulls/1347'
|
||||||
|
)
|
||||||
|
end
|
||||||
|
|
||||||
|
before do
|
||||||
|
allow(project).to receive(:import_data).and_return(double.as_null_object)
|
||||||
|
allow_any_instance_of(Octokit::Client).to receive(:rate_limit!).and_raise(Octokit::NotFound)
|
||||||
|
allow_any_instance_of(Octokit::Client).to receive(:labels).and_return([label, label])
|
||||||
|
allow_any_instance_of(Octokit::Client).to receive(:milestones).and_return([milestone, milestone])
|
||||||
|
allow_any_instance_of(Octokit::Client).to receive(:issues).and_return([issue1, issue2])
|
||||||
|
allow_any_instance_of(Octokit::Client).to receive(:pull_requests).and_return([pull_request, pull_request])
|
||||||
|
allow_any_instance_of(Octokit::Client).to receive(:last_response).and_return(double(rels: { next: nil }))
|
||||||
|
allow_any_instance_of(Gitlab::Shell).to receive(:import_repository).and_raise(Gitlab::Shell::Error)
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'returns true' do
|
||||||
|
expect(described_class.new(project).execute).to eq true
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'does not raise an error' do
|
||||||
|
expect { described_class.new(project).execute }.not_to raise_error
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'stores error messages' do
|
||||||
|
error = {
|
||||||
|
message: 'The remote data could not be fully imported.',
|
||||||
|
errors: [
|
||||||
|
{ type: :label, url: "https://api.github.com/repos/octocat/Hello-World/labels/bug", errors: "Validation failed: Title has already been taken" },
|
||||||
|
{ type: :milestone, url: "https://api.github.com/repos/octocat/Hello-World/milestones/1", errors: "Validation failed: Title has already been taken" },
|
||||||
|
{ type: :issue, url: "https://api.github.com/repos/octocat/Hello-World/issues/1347", errors: "Invalid Repository. Use user/repo format." },
|
||||||
|
{ 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: "Validation failed: Validate branches Cannot Create: This merge request already exists: [\"New feature\"]" },
|
||||||
|
{ type: :wiki, errors: "Gitlab::Shell::Error" }
|
||||||
|
]
|
||||||
|
}
|
||||||
|
|
||||||
|
described_class.new(project).execute
|
||||||
|
|
||||||
|
expect(project.import_error).to eq error.to_json
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
|
@ -27,7 +27,8 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
|
||||||
created_at: created_at,
|
created_at: created_at,
|
||||||
updated_at: updated_at,
|
updated_at: updated_at,
|
||||||
closed_at: nil,
|
closed_at: nil,
|
||||||
merged_at: nil
|
merged_at: nil,
|
||||||
|
url: 'https://api.github.com/repos/octocat/Hello-World/pulls/1347'
|
||||||
}
|
}
|
||||||
end
|
end
|
||||||
|
|
||||||
|
@ -229,4 +230,12 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
describe '#url' do
|
||||||
|
let(:raw_data) { double(base_data) }
|
||||||
|
|
||||||
|
it 'return raw url' do
|
||||||
|
expect(pull_request.url).to eq 'https://api.github.com/repos/octocat/Hello-World/pulls/1347'
|
||||||
|
end
|
||||||
|
end
|
||||||
end
|
end
|
||||||
|
|
Loading…
Reference in New Issue