Merge branch 'kassio/github-importer-more-details-to-logs' into 'master'

Github Importer: Add more details to logs

See merge request gitlab-org/gitlab!61188

app/workers/concerns/gitlab/github_import/object_importer.rb

@@ -27,15 +27,19 @@ module Gitlab
       # client - An instance of `Gitlab::GithubImport::Client`
       # hash - A Hash containing the details of the object to import.
       def import(project, client, hash)
+        object = representation_class.from_json_hash(hash)
+
+        # To better express in the logs what object is being imported.
+        self.github_id = object.attributes.fetch(:github_id)
+
         info(project.id, message: 'starting importer')
 
-        object = representation_class.from_json_hash(hash)
         importer_class.new(object, project, client).execute
 
         counter.increment
         info(project.id, message: 'importer finished')
       rescue StandardError => e
-        error(project.id, e)
+        error(project.id, e, hash)
       end
 
       def counter
@@ -65,16 +69,19 @@ module Gitlab
 
       private
 
+      attr_accessor :github_id
+
       def info(project_id, extra = {})
         logger.info(log_attributes(project_id, extra))
       end
 
-      def error(project_id, exception)
+      def error(project_id, exception, data = {})
         logger.error(
           log_attributes(
             project_id,
             message: 'importer failed',
-            'error.message': exception.message
+            'error.message': exception.message,
+            'github.data': data
           )
         )
 
@@ -88,7 +95,8 @@ module Gitlab
         extra.merge(
           import_source: :github,
           project_id: project_id,
-          importer: importer_class.name
+          importer: importer_class.name,
+          github_id: github_id
         )
       end
     end

lib/gitlab/github_import/representation/issue.rb

@@ -25,6 +25,7 @@ module Gitlab
 
           hash = {
             iid: issue.number,
+            github_id: issue.number,
             title: issue.title,
             description: issue.body,
             milestone_number: issue.milestone&.number,

lib/gitlab/github_import/representation/lfs_object.rb

@@ -13,7 +13,12 @@ module Gitlab
 
         # Builds a lfs_object
         def self.from_api_response(lfs_object)
-          new({ oid: lfs_object.oid, link: lfs_object.link, size: lfs_object.size })
+          new(
+            oid: lfs_object.oid,
+            link: lfs_object.link,
+            size: lfs_object.size,
+            github_id: lfs_object.oid
+          )
         end
 
         # Builds a new lfs_object using a Hash that was built from a JSON payload.

lib/gitlab/github_import/representation/pull_request.rb

@@ -25,6 +25,7 @@ module Gitlab
 
           hash = {
             iid: pr.number,
+            github_id: pr.number,
             title: pr.title,
             description: pr.body,
             source_branch: pr.head.ref,

lib/gitlab/github_import/representation/user.rb

@@ -15,7 +15,11 @@ module Gitlab
         #
         # user - An instance of `Sawyer::Resource` containing the user details.
         def self.from_api_response(user)
-          new(id: user.id, login: user.login)
+          new(
+            id: user.id,
+            github_id: user.id,
+            login: user.login
+          )
         end
 
         # Builds a user using a Hash that was built from a JSON payload.

spec/workers/concerns/gitlab/github_import/object_importer_spec.rb

@@ -18,39 +18,49 @@ RSpec.describe Gitlab::GithubImport::ObjectImporter do
       def counter_description
         'This is a counter'
       end
+
+      def representation_class
+        MockRepresantation
+      end
     end.new
   end
 
+  before do
+    stub_const('MockRepresantation', Class.new do
+      include Gitlab::GithubImport::Representation::ToHash
+      include Gitlab::GithubImport::Representation::ExposeAttribute
+
+      def self.from_json_hash(raw_hash)
+        new(Gitlab::GithubImport::Representation.symbolize_hash(raw_hash))
+      end
+
+      attr_reader :attributes
+
+      def initialize(attributes)
+        @attributes = attributes
+      end
+    end)
+  end
+
   describe '#import' do
-    let(:representation_class) { double(:representation_class) }
     let(:importer_class) { double(:importer_class, name: 'klass_name') }
     let(:importer_instance) { double(:importer_instance) }
-    let(:representation) { double(:representation) }
     let(:project) { double(:project, full_path: 'foo/bar', id: 1) }
     let(:client) { double(:client) }
 
     before do
-      expect(worker)
-        .to receive(:representation_class)
-        .and_return(representation_class)
-
       expect(worker)
         .to receive(:importer_class)
         .at_least(:once)
         .and_return(importer_class)
+    end
 
-      expect(representation_class)
-        .to receive(:from_json_hash)
-        .with(an_instance_of(Hash))
-        .and_return(representation)
-
+    it 'imports the object' do
       expect(importer_class)
         .to receive(:new)
-        .with(representation, project, client)
+        .with(instance_of(MockRepresantation), project, client)
         .and_return(importer_instance)
-    end
 
-    it 'imports the object' do
       expect(importer_instance)
         .to receive(:execute)
 
@@ -62,6 +72,7 @@ RSpec.describe Gitlab::GithubImport::ObjectImporter do
         expect(logger)
           .to receive(:info)
           .with(
+            github_id: 1,
             message: 'starting importer',
             import_source: :github,
             project_id: 1,
@@ -70,6 +81,7 @@ RSpec.describe Gitlab::GithubImport::ObjectImporter do
         expect(logger)
           .to receive(:info)
           .with(
+            github_id: 1,
             message: 'importer finished',
             import_source: :github,
             project_id: 1,
@@ -77,10 +89,15 @@ RSpec.describe Gitlab::GithubImport::ObjectImporter do
           )
       end
 
-      worker.import(project, client, { 'number' => 10 })
+      worker.import(project, client, { 'number' => 10, 'github_id' => 1 })
     end
 
     it 'logs error when the import fails' do
+      expect(importer_class)
+        .to receive(:new)
+        .with(instance_of(MockRepresantation), project, client)
+        .and_return(importer_instance)
+
       exception = StandardError.new('some error')
       expect(importer_instance)
         .to receive(:execute)
@@ -90,6 +107,7 @@ RSpec.describe Gitlab::GithubImport::ObjectImporter do
         expect(logger)
           .to receive(:info)
           .with(
+            github_id: 1,
             message: 'starting importer',
             import_source: :github,
             project_id: project.id,
@@ -98,20 +116,64 @@ RSpec.describe Gitlab::GithubImport::ObjectImporter do
         expect(logger)
           .to receive(:error)
           .with(
+            github_id:  1,
             message: 'importer failed',
             import_source: :github,
             project_id: project.id,
             importer: 'klass_name',
-            'error.message': 'some error'
+            'error.message': 'some error',
+            'github.data': {
+              'github_id' => 1,
+              'number' => 10
+            }
           )
       end
 
       expect(Gitlab::ErrorTracking)
         .to receive(:track_and_raise_exception)
-        .with(exception, import_source: :github, project_id: 1, importer: 'klass_name')
-        .and_call_original
+        .with(
+          exception,
+          import_source: :github,
+          github_id: 1,
+          project_id: 1,
+          importer: 'klass_name'
+        ).and_call_original
+
+      expect { worker.import(project, client, { 'number' => 10, 'github_id' => 1 }) }
+        .to raise_error(exception)
+    end
+
+    it 'logs error when representation does not have a github_id' do
+      expect(importer_class).not_to receive(:new)
+
+      expect_next_instance_of(Gitlab::Import::Logger) do |logger|
+        expect(logger)
+          .to receive(:error)
+          .with(
+            github_id:  nil,
+            message: 'importer failed',
+            import_source: :github,
+            project_id: project.id,
+            importer: 'klass_name',
+            'error.message': 'key not found: :github_id',
+            'github.data': {
+              'number' => 10
+            }
+          )
+      end
+
+      expect(Gitlab::ErrorTracking)
+        .to receive(:track_and_raise_exception)
+        .with(
+          an_instance_of(KeyError),
+          import_source: :github,
+          github_id: nil,
+          project_id: 1,
+          importer: 'klass_name'
+        ).and_call_original
 
-      expect { worker.import(project, client, { 'number' => 10 }) }.to raise_error(exception)
+      expect { worker.import(project, client, { 'number' => 10 }) }
+        .to raise_error(KeyError, 'key not found: :github_id')
     end
   end
 

spec/workers/gitlab/github_import/import_diff_note_worker_spec.rb

@@ -12,6 +12,7 @@ RSpec.describe Gitlab::GithubImport::ImportDiffNoteWorker do
       importer = double(:importer)
       hash = {
         'noteable_id' => 42,
+        'github_id' => 42,
         'path' => 'README.md',
         'commit_id' => '123abc',
         'diff_hunk' => "@@ -1 +1 @@\n-Hello\n+Hello world",

spec/workers/gitlab/github_import/import_issue_worker_spec.rb

@@ -12,6 +12,7 @@ RSpec.describe Gitlab::GithubImport::ImportIssueWorker do
       importer = double(:importer)
       hash = {
         'iid' => 42,
+        'github_id' => 42,
         'title' => 'My Issue',
         'description' => 'This is my issue',
         'milestone_number' => 4,

spec/workers/gitlab/github_import/import_note_worker_spec.rb

@@ -12,6 +12,7 @@ RSpec.describe Gitlab::GithubImport::ImportNoteWorker do
       importer = double(:importer)
       hash = {
         'noteable_id' => 42,
+        'github_id' => 42,
         'noteable_type' => 'issues',
         'user' => { 'id' => 4, 'login' => 'alice' },
         'note' => 'Hello world',

spec/workers/gitlab/github_import/import_pull_request_worker_spec.rb

@@ -12,6 +12,7 @@ RSpec.describe Gitlab::GithubImport::ImportPullRequestWorker do
       importer = double(:importer)
       hash = {
         'iid' => 42,
+        'github_id' => 42,
         'title' => 'My Pull Request',
         'description' => 'This is my pull request',
         'source_branch' => 'my-feature',