From 3c09000e18dcbf6a74ed1f749db3184e309cf081 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Wed, 10 Aug 2016 18:22:21 -0300 Subject: [PATCH 1/3] Does not halt the GitHub import process when an error occurs --- app/models/project.rb | 2 - app/workers/repository_import_worker.rb | 2 + lib/gitlab/github_import/importer.rb | 82 ++++++----- .../github_import/pull_request_formatter.rb | 4 + .../lib/gitlab/github_import/importer_spec.rb | 129 ++++++++++++++++++ .../pull_request_formatter_spec.rb | 11 +- 6 files changed, 194 insertions(+), 36 deletions(-) create mode 100644 spec/lib/gitlab/github_import/importer_spec.rb diff --git a/app/models/project.rb b/app/models/project.rb index 1855760e694..8cf093be4c3 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -471,8 +471,6 @@ class Project < ActiveRecord::Base end def reset_cache_and_import_attrs - update(import_error: nil) - ProjectCacheWorker.perform_async(self.id) self.import_data.destroy if self.import_data diff --git a/app/workers/repository_import_worker.rb b/app/workers/repository_import_worker.rb index e6701078f71..d2ca8813ab9 100644 --- a/app/workers/repository_import_worker.rb +++ b/app/workers/repository_import_worker.rb @@ -14,6 +14,8 @@ class RepositoryImportWorker import_url: @project.import_url, path: @project.path_with_namespace) + project.update_column(:import_error, nil) + result = Projects::ImportService.new(project, current_user).execute if result[:status] == :error diff --git a/lib/gitlab/github_import/importer.rb b/lib/gitlab/github_import/importer.rb index 9ddc8905bd6..86b49a5021a 100644 --- a/lib/gitlab/github_import/importer.rb +++ b/lib/gitlab/github_import/importer.rb @@ -3,12 +3,13 @@ module Gitlab class Importer include Gitlab::ShellAdapter - attr_reader :client, :project, :repo, :repo_url + attr_reader :client, :errors, :project, :repo, :repo_url def initialize(project) @project = project @repo = project.import_source @repo_url = project.import_url + @errors = [] if credentials @client = Client.new(credentials[:user]) @@ -18,8 +19,14 @@ module Gitlab end def execute - import_labels && import_milestones && import_issues && - import_pull_requests && import_wiki + import_labels + import_milestones + import_issues + import_pull_requests + import_wiki + handle_errors + + true end private @@ -28,22 +35,32 @@ module Gitlab @credentials ||= project.import_data.credentials if project.import_data end + def handle_errors + project.update_column(:import_error, errors.to_json) unless errors.empty? + end + def import_labels labels = client.labels(repo, per_page: 100) - labels.each { |raw| LabelFormatter.new(project, raw).create! } - true - rescue ActiveRecord::RecordInvalid => e - raise Projects::ImportService::Error, e.message + labels.each do |raw| + begin + LabelFormatter.new(project, raw).create! + rescue => e + errors << { type: :label, url: Gitlab::UrlSanitizer.sanitize(raw.url), errors: e.message } + end + end end def import_milestones milestones = client.milestones(repo, state: :all, per_page: 100) - milestones.each { |raw| MilestoneFormatter.new(project, raw).create! } - true - rescue ActiveRecord::RecordInvalid => e - raise Projects::ImportService::Error, e.message + milestones.each do |raw| + begin + MilestoneFormatter.new(project, raw).create! + rescue => e + errors << { type: :milestone, url: Gitlab::UrlSanitizer.sanitize(raw.url), errors: e.message } + end + end end def import_issues @@ -53,15 +70,15 @@ module Gitlab gh_issue = IssueFormatter.new(project, raw) if gh_issue.valid? - issue = gh_issue.create! - apply_labels(issue) - import_comments(issue) if gh_issue.has_comments? + begin + issue = gh_issue.create! + 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 - - true - rescue ActiveRecord::RecordInvalid => e - raise Projects::ImportService::Error, e.message end def import_pull_requests @@ -77,14 +94,12 @@ module Gitlab apply_labels(merge_request) import_comments(merge_request) import_comments_on_diff(merge_request) - rescue ActiveRecord::RecordInvalid => e - raise Projects::ImportService::Error, e.message + rescue => e + errors << { type: :pull_request, url: Gitlab::UrlSanitizer.sanitize(pull_request.url), errors: e.message } ensure clean_up_restored_branches(pull_request) end end - - true end def restore_source_branch(pull_request) @@ -98,7 +113,7 @@ module Gitlab def remove_branch(name) project.repository.delete_branch(name) rescue Rugged::ReferenceError - nil + errors << { type: :remove_branch, name: name } end def clean_up_restored_branches(pull_request) @@ -112,9 +127,10 @@ module Gitlab issue = client.issue(repo, issuable.iid) if issue.labels.count > 0 - label_ids = issue.labels.map do |raw| - Label.find_by(LabelFormatter.new(project, raw).attributes).try(:id) - end + label_ids = issue.labels + .map { |raw| LabelFormatter.new(project, raw).attributes } + .map { |attrs| Label.find_by(attrs).try(:id) } + .compact issuable.update_attribute(:label_ids, label_ids) end @@ -132,8 +148,12 @@ module Gitlab def create_comments(issuable, comments) comments.each do |raw| - comment = CommentFormatter.new(project, raw) - issuable.notes.create!(comment.attributes) + begin + 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 @@ -143,16 +163,12 @@ module Gitlab gitlab_shell.import_repository(project.repository_storage_path, wiki.path_with_namespace, wiki.import_url) project.update_attribute(:wiki_enabled, true) end - - true rescue Gitlab::Shell::Error => e # GitHub error message when the wiki repo has not been created, # this means that repo has wiki enabled, but have no pages. So, # we can skip the import. if e.message !~ /repository not exported/ - raise Projects::ImportService::Error, e.message - else - true + errors << { type: :wiki, errors: e.message } end end end diff --git a/lib/gitlab/github_import/pull_request_formatter.rb b/lib/gitlab/github_import/pull_request_formatter.rb index b84538a090a..04aa3664f64 100644 --- a/lib/gitlab/github_import/pull_request_formatter.rb +++ b/lib/gitlab/github_import/pull_request_formatter.rb @@ -56,6 +56,10 @@ module Gitlab end end + def url + raw_data.url + end + private def assigned? diff --git a/spec/lib/gitlab/github_import/importer_spec.rb b/spec/lib/gitlab/github_import/importer_spec.rb new file mode 100644 index 00000000000..2c8d86ef9b6 --- /dev/null +++ b/spec/lib/gitlab/github_import/importer_spec.rb @@ -0,0 +1,129 @@ +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 + 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 errors.to_json + end + end + end +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 aa28e360993..b667abf063d 100644 --- a/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb +++ b/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb @@ -27,7 +27,8 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do created_at: created_at, updated_at: updated_at, closed_at: nil, - merged_at: nil + merged_at: nil, + url: 'https://api.github.com/repos/octocat/Hello-World/pulls/1347' } end @@ -229,4 +230,12 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do 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 From 2986de7c8c63c0a90f750adbfd843d3b2d50e25f Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Wed, 24 Aug 2016 12:14:06 -0300 Subject: [PATCH 2/3] Add readable error message when remote data could not be fully imported --- lib/gitlab/github_import/importer.rb | 7 +++++- .../lib/gitlab/github_import/importer_spec.rb | 23 +++++++++++-------- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/lib/gitlab/github_import/importer.rb b/lib/gitlab/github_import/importer.rb index 86b49a5021a..02ffb43d89b 100644 --- a/lib/gitlab/github_import/importer.rb +++ b/lib/gitlab/github_import/importer.rb @@ -36,7 +36,12 @@ module Gitlab end def handle_errors - project.update_column(:import_error, errors.to_json) unless errors.empty? + 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 diff --git a/spec/lib/gitlab/github_import/importer_spec.rb b/spec/lib/gitlab/github_import/importer_spec.rb index 2c8d86ef9b6..b7c3bc4e1a7 100644 --- a/spec/lib/gitlab/github_import/importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer_spec.rb @@ -110,19 +110,22 @@ describe Gitlab::GithubImport::Importer, lib: true do end it 'stores error messages' do - 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" } - ] + 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 errors.to_json + expect(project.import_error).to eq error.to_json end end end From 8e52705776dce7021ac1a150253037f085638a46 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Wed, 24 Aug 2016 13:04:26 -0300 Subject: [PATCH 3/3] Update CHANGELOG --- CHANGELOG | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG b/CHANGELOG index ef38d3e29f5..b3cb9d4b3e1 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -6,6 +6,7 @@ v 8.12.0 (unreleased) - Added tests for diff notes 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 v 8.11.0