Merge branch 'track-project-import-failure' into 'master'

Track project import failure

Fixes #12512 

This also help us on identify why the import process is returning the wrong status when the project was successfully imported. Like mentioned in the issue #12450 

See merge request !2538

CHANGELOG

@@ -8,6 +8,7 @@ v 8.5.0 (unreleased)
   - Fix diff comments loaded by AJAX to load comment with diff in discussion tab
   - Whitelist raw "abbr" elements when parsing Markdown (Benedict Etzel)
   - Don't vendor minified JS
+  - Track project import failure
 
 v 8.4.1
   - Apply security updates for Rails (4.2.5.1), rails-html-sanitizer (1.0.3),

app/services/projects/import_service.rb

+module Projects
+  class ImportService < BaseService
+    include Gitlab::ShellAdapter
+
+    class Error < StandardError; end
+
+    ALLOWED_TYPES = [
+      'bitbucket',
+      'fogbugz',
+      'gitlab',
+      'github',
+      'google_code'
+    ]
+
+    def execute
+      if unknown_url?
+        # In this case, we only want to import issues, not a repository.
+        create_repository
+      else
+        import_repository
+      end
+
+      import_data
+
+      success
+    rescue Error => e
+      error(e.message)
+    end
+
+    private
+
+    def create_repository
+      unless project.create_repository
+        raise Error, 'The repository could not be created.'
+      end
+    end
+
+    def import_repository
+      begin
+        gitlab_shell.import_repository(project.path_with_namespace, project.import_url)
+      rescue Gitlab::Shell::Error => e
+        raise Error, e.message
+      end
+    end
+
+    def import_data
+      return unless has_importer?
+
+      unless importer.execute
+        raise Error, 'The remote data could not be imported.'
+      end
+    end
+
+    def has_importer?
+      ALLOWED_TYPES.include?(project.import_type)
+    end
+
+    def importer
+      class_name = "Gitlab::#{project.import_type.camelize}Import::Importer"
+      class_name.constantize.new(project)
+    end
+
+    def unknown_url?
+      project.import_url == Project::UNKNOWN_IMPORT_URL
+    end
+  end
+end

app/workers/repository_import_worker.rb

@@ -4,52 +4,20 @@ class RepositoryImportWorker
 
   sidekiq_options queue: :gitlab_shell
 
-  def perform(project_id)
-    project = Project.find(project_id)
+  attr_accessor :project, :current_user
 
-    if project.import_url == Project::UNKNOWN_IMPORT_URL
-      # In this case, we only want to import issues, not a repository.
-      unless project.create_repository
-        project.update(import_error: "The repository could not be created.")
-        project.import_fail
-        return
-      end
-    else
-      begin
-        gitlab_shell.import_repository(project.path_with_namespace, project.import_url)
-      rescue Gitlab::Shell::Error => e
-        project.update(import_error: e.message)
-        project.import_fail
-        return
-      end
-    end
+  def perform(project_id)
+    @project = Project.find(project_id)
+    @current_user = @project.creator
 
-    data_import_result =
-      case project.import_type
-      when 'github'
-        Gitlab::GithubImport::Importer.new(project).execute
-      when 'gitlab'
-        Gitlab::GitlabImport::Importer.new(project).execute
-      when 'bitbucket'
-        Gitlab::BitbucketImport::Importer.new(project).execute
-      when 'google_code'
-        Gitlab::GoogleCodeImport::Importer.new(project).execute
-      when 'fogbugz'
-        Gitlab::FogbugzImport::Importer.new(project).execute
-      else
-        true
-      end
+    result = Projects::ImportService.new(project, current_user).execute
 
-    unless data_import_result
-      project.update(import_error: "The remote issue data could not be imported.")
+    if result[:status] == :error
+      project.update(import_error: result[:message])
       project.import_fail
       return
     end
 
-    if project.import_type == 'bitbucket'
-      Gitlab::BitbucketImport::KeyDeleter.new(project).execute
-    end
-
     project.import_finish
   end
 end

lib/gitlab/bitbucket_import/importer.rb

@@ -13,12 +13,36 @@ module Gitlab
       end
 
       def execute
-        project_identifier = project.import_source
+        import_issues if has_issues?
 
-        return true unless client.project(project_identifier)["has_issues"]
+        true
+      rescue ActiveRecord::RecordInvalid => e
+        raise Projects::ImportService::Error.new, e.message
+      ensure
+        Gitlab::BitbucketImport::KeyDeleter.new(project).execute
+      end
 
-        #Issues && Comments
-        issues = client.issues(project_identifier)
+      private
+
+      def gl_user_id(project, bitbucket_id)
+        if bitbucket_id
+          user = User.joins(:identities).find_by("identities.extern_uid = ? AND identities.provider = 'bitbucket'", bitbucket_id.to_s)
+          (user && user.id) || project.creator_id
+        else
+          project.creator_id
+        end
+      end
+
+      def identifier
+        project.import_source
+      end
+
+      def has_issues?
+        client.project(identifier)["has_issues"]
+      end
+
+      def import_issues
+        issues = client.issues(identifier)
 
         issues.each do |issue|
           body = ''
@@ -33,7 +57,7 @@ module Gitlab
           body = @formatter.author_line(author)
           body += issue["content"]
 
-          comments = client.issue_comments(project_identifier, issue["local_id"])
+          comments = client.issue_comments(identifier, issue["local_id"])
 
           if comments.any?
             body += @formatter.comments_header
@@ -56,20 +80,9 @@ module Gitlab
             author_id: gl_user_id(project, reporter)
           )
         end
-
-        true
+      rescue ActiveRecord::RecordInvalid => e
+        raise Projects::ImportService::Error, e.message
       end
-
-      private
-
-      def gl_user_id(project, bitbucket_id)
-        if bitbucket_id
-          user = User.joins(:identities).find_by("identities.extern_uid = ? AND identities.provider = 'bitbucket'", bitbucket_id.to_s)
-          (user && user.id) || project.creator_id
-        else
-          project.creator_id
-        end
-       end
     end
   end
 end

lib/gitlab/github_import/importer.rb

@@ -35,8 +35,8 @@ module Gitlab
         end
 
         true
-      rescue ActiveRecord::RecordInvalid
-        false
+      rescue ActiveRecord::RecordInvalid => e
+        raise Projects::ImportService::Error, e.message
       end
 
       def import_pull_requests
@@ -53,8 +53,8 @@ module Gitlab
         end
 
         true
-      rescue ActiveRecord::RecordInvalid
-        false
+      rescue ActiveRecord::RecordInvalid => e
+        raise Projects::ImportService::Error, e.message
       end
 
       def import_comments(issue_number, noteable)
@@ -83,10 +83,13 @@ module Gitlab
 
         true
       rescue Gitlab::Shell::Error => e
-        if e.message =~ /repository not exported/
-          true
+        # 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
-          false
+          true
         end
       end
     end

spec/services/projects/import_service_spec.rb

+require 'spec_helper'
+
+describe Projects::ImportService, services: true do
+  let!(:project) { create(:empty_project) }
+  let(:user) { project.creator }
+
+  subject { described_class.new(project, user) }
+
+  describe '#execute' do
+    context 'with unknown url' do
+      before do
+        project.import_url = Project::UNKNOWN_IMPORT_URL
+      end
+
+      it 'succeeds if repository is created successfully' do
+        expect(project).to receive(:create_repository).and_return(true)
+
+        result = subject.execute
+
+        expect(result[:status]).to eq :success
+      end
+
+      it 'fails if repository creation fails' do
+        expect(project).to receive(:create_repository).and_return(false)
+
+        result = subject.execute
+
+        expect(result[:status]).to eq :error
+        expect(result[:message]).to eq 'The repository could not be created.'
+      end
+    end
+
+    context 'with known url' do
+      before do
+        project.import_url = 'https://github.com/vim/vim.git'
+      end
+
+      it 'succeeds if repository import is successfully' do
+        expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).with(project.path_with_namespace, project.import_url).and_return(true)
+
+        result = subject.execute
+
+        expect(result[:status]).to eq :success
+      end
+
+      it 'fails if repository import fails' do
+        expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).with(project.path_with_namespace, project.import_url).and_raise(Gitlab::Shell::Error.new('Failed to import the repository'))
+
+        result = subject.execute
+
+        expect(result[:status]).to eq :error
+        expect(result[:message]).to eq 'Failed to import the repository'
+      end
+    end
+
+    context 'with valid importer' do
+      before do
+        stub_github_omniauth_provider
+
+        project.import_url = 'https://github.com/vim/vim.git'
+        project.import_type = 'github'
+
+        allow(project).to receive(:import_data).and_return(double.as_null_object)
+      end
+
+      it 'succeeds if importer succeeds' do
+        expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).with(project.path_with_namespace, project.import_url).and_return(true)
+        expect_any_instance_of(Gitlab::GithubImport::Importer).to receive(:execute).and_return(true)
+
+        result = subject.execute
+
+        expect(result[:status]).to eq :success
+      end
+
+      it 'fails if importer fails' do
+        expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).with(project.path_with_namespace, project.import_url).and_return(true)
+        expect_any_instance_of(Gitlab::GithubImport::Importer).to receive(:execute).and_return(false)
+
+        result = subject.execute
+
+        expect(result[:status]).to eq :error
+        expect(result[:message]).to eq 'The remote data could not be imported.'
+      end
+
+      it 'fails if importer raise an error' do
+        expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).with(project.path_with_namespace, project.import_url).and_return(true)
+        expect_any_instance_of(Gitlab::GithubImport::Importer).to receive(:execute).and_raise(Projects::ImportService::Error.new('Github: failed to connect API'))
+
+        result = subject.execute
+
+        expect(result[:status]).to eq :error
+        expect(result[:message]).to eq 'Github: failed to connect API'
+      end
+    end
+
+    def stub_github_omniauth_provider
+      provider = OpenStruct.new(
+        name: 'github',
+        app_id: 'asd123',
+        app_secret: 'asd123'
+      )
+
+      Gitlab.config.omniauth.providers << provider
+    end
+  end
+end