Merge branch 'kassio/expose-failed-import-errors' into 'master'

Expose failed import error through API

See merge request gitlab-org/gitlab!75440

lib/gitlab/import/import_failure_service.rb

@@ -15,11 +15,21 @@ module Gitlab
           exception: exception,
           import_state: import_state,
           project_id: project_id,
-          error_source: error_source
-        ).execute(fail_import: fail_import, metrics: metrics)
+          error_source: error_source,
+          fail_import: fail_import,
+          metrics: metrics
+        ).execute
       end
 
-      def initialize(exception:, import_state: nil, project_id: nil, error_source: nil)
+      def initialize(
+        exception:,
+        import_state: nil,
+        project_id: nil,
+        error_source: nil,
+        fail_import: false,
+        metrics: false
+      )
+
         if import_state.blank? && project_id.blank?
           raise ArgumentError, 'import_state OR project_id must be provided'
         end
@@ -34,9 +44,11 @@ module Gitlab
 
         @exception = exception
         @error_source = error_source
+        @fail_import = fail_import
+        @metrics = metrics
       end
 
-      def execute(fail_import:, metrics:)
+      def execute
         track_exception
         persist_failure
 
@@ -46,7 +58,7 @@ module Gitlab
 
       private
 
-      attr_reader :exception, :import_state, :project, :error_source
+      attr_reader :exception, :import_state, :project, :error_source, :fail_import, :metrics
 
       def track_exception
         attributes = {
@@ -65,12 +77,15 @@ module Gitlab
         Gitlab::ErrorTracking.track_exception(exception, attributes)
       end
 
+      # Failures with `retry_count: 0` are considered "hard_failures" and those
+      # are exposed on the REST API projects/:id/import
       def persist_failure
         project.import_failures.create(
           source: error_source,
           exception_class: exception.class.to_s,
           exception_message: exception.message.truncate(255),
-          correlation_id_value: Labkit::Correlation::CorrelationId.current_or_new_id
+          correlation_id_value: Labkit::Correlation::CorrelationId.current_or_new_id,
+          retry_count: fail_import ? 0 : nil
         )
       end
 

spec/lib/gitlab/import/import_failure_service_spec.rb

@@ -7,58 +7,48 @@ RSpec.describe Gitlab::Import::ImportFailureService, :aggregate_failures do
   let_it_be(:project) { create(:project, :import_started, import_type: import_type) }
 
   let(:exception) { StandardError.new('some error') }
-  let(:arguments) { { project_id: project.id } }
-  let(:base_arguments) { { error_source: 'SomeImporter', exception: exception }.merge(arguments) }
-  let(:exe_arguments) { { fail_import: false, metrics: false } }
+  let(:import_state) { nil }
+  let(:fail_import) { false }
+  let(:metrics) { false }
 
-  describe '.track' do
-    context 'with all arguments provided' do
-      let(:instance) { double(:failure_service) }
-      let(:instance_arguments) do
+  let(:arguments) do
     {
+      project_id: project.id,
+      error_source: 'SomeImporter',
       exception: exception,
-          import_state: '_import_state_',
-          project_id: '_project_id_',
-          error_source: '_error_source_'
+      fail_import: fail_import,
+      metrics: metrics,
+      import_state: import_state
     }
   end
 
-      let(:exe_arguments) do
+  describe '.track' do
+    let(:instance) { double(:failure_service) }
+
+    context 'with all arguments provided' do
+      let(:arguments) do
         {
+          exception: exception,
+          import_state: '_import_state_',
+          project_id: '_project_id_',
+          error_source: '_error_source_',
           fail_import: '_fail_import_',
           metrics: '_metrics_'
         }
       end
 
       it 'invokes a new instance and executes' do
-        expect(described_class).to receive(:new).with(**instance_arguments).and_return(instance)
-        expect(instance).to receive(:execute).with(**exe_arguments)
+        expect(described_class).to receive(:new).with(**arguments).and_return(instance)
+        expect(instance).to receive(:execute)
 
-        described_class.track(**instance_arguments.merge(exe_arguments))
+        described_class.track(**arguments)
       end
     end
 
     context 'with only necessary arguments utilizing defaults' do
-      let(:instance) { double(:failure_service) }
-      let(:instance_arguments) do
-        {
-          exception: exception,
-          import_state: nil,
-          project_id: nil,
-          error_source: nil
-        }
-      end
-
-      let(:exe_arguments) do
-        {
-          fail_import: false,
-          metrics: false
-        }
-      end
-
       it 'invokes a new instance and executes' do
-        expect(described_class).to receive(:new).with(**instance_arguments).and_return(instance)
-        expect(instance).to receive(:execute).with(**exe_arguments)
+        expect(described_class).to receive(:new).with(a_hash_including(exception: exception)).and_return(instance)
+        expect(instance).to receive(:execute)
 
         described_class.track(exception: exception)
       end
@@ -66,7 +56,7 @@ RSpec.describe Gitlab::Import::ImportFailureService, :aggregate_failures do
   end
 
   describe '#execute' do
-    subject(:service) { described_class.new(**base_arguments) }
+    subject(:service) { described_class.new(**arguments) }
 
     shared_examples 'logs the exception and fails the import' do
       it 'when the failure does not abort the import' do
@@ -89,13 +79,14 @@ RSpec.describe Gitlab::Import::ImportFailureService, :aggregate_failures do
             source: 'SomeImporter'
           )
 
-        service.execute(**exe_arguments)
+        service.execute
 
         expect(project.import_state.reload.status).to eq('failed')
 
         expect(project.import_failures).not_to be_empty
         expect(project.import_failures.last.exception_class).to eq('StandardError')
         expect(project.import_failures.last.exception_message).to eq('some error')
+        expect(project.import_failures.last.retry_count).to eq(0)
       end
     end
 
@@ -120,32 +111,36 @@ RSpec.describe Gitlab::Import::ImportFailureService, :aggregate_failures do
             source: 'SomeImporter'
           )
 
-        service.execute(**exe_arguments)
+        service.execute
 
         expect(project.import_state.reload.status).to eq('started')
 
         expect(project.import_failures).not_to be_empty
         expect(project.import_failures.last.exception_class).to eq('StandardError')
         expect(project.import_failures.last.exception_message).to eq('some error')
+        expect(project.import_failures.last.retry_count).to eq(nil)
       end
     end
 
     context 'when tracking metrics' do
-      let(:exe_arguments) { { fail_import: false, metrics: true } }
+      let(:metrics) { true }
 
       it 'tracks the failed import' do
-        metrics = double(:metrics)
+        metrics_double = double(:metrics)
 
-        expect(Gitlab::Import::Metrics).to receive(:new).with("#{project.import_type}_importer", project).and_return(metrics)
-        expect(metrics).to receive(:track_failed_import)
+        expect(Gitlab::Import::Metrics)
+          .to receive(:new)
+          .with("#{project.import_type}_importer", project)
+          .and_return(metrics_double)
+        expect(metrics_double).to receive(:track_failed_import)
 
-        service.execute(**exe_arguments)
+        service.execute
       end
     end
 
     context 'when using the project as reference' do
       context 'when it fails the import' do
-        let(:exe_arguments) { { fail_import: true, metrics: false } }
+        let(:fail_import) { true }
 
         it_behaves_like 'logs the exception and fails the import'
       end
@@ -156,10 +151,10 @@ RSpec.describe Gitlab::Import::ImportFailureService, :aggregate_failures do
     end
 
     context 'when using the import_state as reference' do
-      let(:arguments) { { import_state: project.import_state } }
+      let(:import_state) { project.import_state }
 
       context 'when it fails the import' do
-        let(:exe_arguments) { { fail_import: true, metrics: false } }
+        let(:fail_import) { true }
 
         it_behaves_like 'logs the exception and fails the import'
       end