Does not halt the GitHub import process when an error occurs

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

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

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?
+            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|
+          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

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?

spec/lib/gitlab/github_import/importer_spec.rb

+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

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