Merge branch '335203-github-importer-improve-logging' into 'master'

Github Importer: Improve logging with more identifying data

See merge request gitlab-org/gitlab!69509

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

@@ -26,8 +26,7 @@ module Gitlab
         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)
-
+        self.github_identifiers = object.github_identifiers
         info(project.id, message: 'starting importer')
 
         importer_class.new(object, project, client).execute
@@ -35,10 +34,10 @@ module Gitlab
         Gitlab::GithubImport::ObjectCounter.increment(project, object_type, :imported)
 
         info(project.id, message: 'importer finished')
-      rescue KeyError => e
+      rescue NoMethodError => e
         # This exception will be more useful in development when a new
         # Representation is created but the developer forgot to add a
-        # `:github_id` field.
+        # `:github_identifiers` field.
         Gitlab::Import::ImportFailureService.track(
           project_id: project.id,
           error_source: importer_class.name,
@@ -72,7 +71,7 @@ module Gitlab
 
       private
 
-      attr_accessor :github_id
+      attr_accessor :github_identifiers
 
       def info(project_id, extra = {})
         Logger.info(log_attributes(project_id, extra))
@@ -82,7 +81,7 @@ module Gitlab
         extra.merge(
           project_id: project_id,
           importer: importer_class.name,
-          github_id: github_id
+          github_identifiers: github_identifiers
         )
       end
     end

lib/gitlab/github_import/representation/diff_note.rb

@@ -11,7 +11,7 @@ module Gitlab
 
         expose_attribute :noteable_type, :noteable_id, :commit_id, :file_path,
                          :diff_hunk, :author, :note, :created_at, :updated_at,
-                         :github_id, :original_commit_id
+                         :original_commit_id, :note_id
 
         NOTEABLE_ID_REGEX = %r{/pull/(?<iid>\d+)}i.freeze
 
@@ -40,7 +40,7 @@ module Gitlab
             note: note.body,
             created_at: note.created_at,
             updated_at: note.updated_at,
-            github_id: note.id
+            note_id: note.id
           }
 
           new(hash)
@@ -82,6 +82,14 @@ module Gitlab
             new_file: false
           }
         end
+
+        def github_identifiers
+          {
+            note_id: note_id,
+            noteable_id: noteable_id,
+            noteable_type: noteable_type
+          }
+        end
       end
     end
   end

lib/gitlab/github_import/representation/issue.rb

@@ -25,7 +25,6 @@ module Gitlab
 
           hash = {
             iid: issue.number,
-            github_id: issue.number,
             title: issue.title,
             description: issue.body,
             milestone_number: issue.milestone&.number,
@@ -75,6 +74,13 @@ module Gitlab
         def issuable_type
           pull_request? ? 'MergeRequest' : 'Issue'
         end
+
+        def github_identifiers
+          {
+            iid: iid,
+            issuable_type: issuable_type
+          }
+        end
       end
     end
   end

lib/gitlab/github_import/representation/lfs_object.rb

@@ -16,8 +16,7 @@ module Gitlab
           new(
             oid: lfs_object.oid,
             link: lfs_object.link,
-            size: lfs_object.size,
-            github_id: lfs_object.oid
+            size: lfs_object.size
           )
         end
 
@@ -31,6 +30,12 @@ module Gitlab
         def initialize(attributes)
           @attributes = attributes
         end
+
+        def github_identifiers
+          {
+            oid: oid
+          }
+        end
       end
     end
   end

lib/gitlab/github_import/representation/note.rb

@@ -10,7 +10,7 @@ module Gitlab
         attr_reader :attributes
 
         expose_attribute :noteable_id, :noteable_type, :author, :note,
-                         :created_at, :updated_at, :github_id
+                         :created_at, :updated_at, :note_id
 
         NOTEABLE_TYPE_REGEX = %r{/(?<type>(pull|issues))/(?<iid>\d+)}i.freeze
 
@@ -42,7 +42,7 @@ module Gitlab
             note: note.body,
             created_at: note.created_at,
             updated_at: note.updated_at,
-            github_id: note.id
+            note_id: note.id
           }
 
           new(hash)
@@ -64,6 +64,14 @@ module Gitlab
         end
 
         alias_method :issuable_type, :noteable_type
+
+        def github_identifiers
+          {
+            note_id: note_id,
+            noteable_id: noteable_id,
+            noteable_type: noteable_type
+          }
+        end
       end
     end
   end

lib/gitlab/github_import/representation/pull_request.rb

@@ -25,7 +25,6 @@ module Gitlab
 
           hash = {
             iid: pr.number,
-            github_id: pr.number,
             title: pr.title,
             description: pr.body,
             source_branch: pr.head.ref,
@@ -108,6 +107,13 @@ module Gitlab
         def issuable_type
           'MergeRequest'
         end
+
+        def github_identifiers
+          {
+            iid: iid,
+            issuable_type: issuable_type
+          }
+        end
       end
     end
   end

lib/gitlab/github_import/representation/pull_request_review.rb

@@ -9,7 +9,7 @@ module Gitlab
 
         attr_reader :attributes
 
-        expose_attribute :author, :note, :review_type, :submitted_at, :github_id, :merge_request_id
+        expose_attribute :author, :note, :review_type, :submitted_at, :merge_request_id, :review_id
 
         def self.from_api_response(review)
           user = Representation::User.from_api_response(review.user) if review.user
@@ -20,7 +20,7 @@ module Gitlab
             note: review.body,
             review_type: review.state,
             submitted_at: review.submitted_at,
-            github_id: review.id
+            review_id: review.id
           )
         end
 
@@ -43,6 +43,13 @@ module Gitlab
         def approval?
           review_type == 'APPROVED'
         end
+
+        def github_identifiers
+          {
+            review_id: review_id,
+            merge_request_id: merge_request_id
+          }
+        end
       end
     end
   end

lib/gitlab/github_import/representation/user.rb

@@ -17,7 +17,6 @@ module Gitlab
         def self.from_api_response(user)
           new(
             id: user.id,
-            github_id: user.id,
             login: user.login
           )
         end

spec/lib/gitlab/github_import/representation/diff_note_spec.rb

@@ -51,7 +51,7 @@ RSpec.describe Gitlab::GithubImport::Representation::DiffNote do
       end
 
       it 'includes the GitHub ID' do
-        expect(note.github_id).to eq(1)
+        expect(note.note_id).to eq(1)
       end
 
       it 'returns the noteable type' do
@@ -106,7 +106,7 @@ RSpec.describe Gitlab::GithubImport::Representation::DiffNote do
           'note' => 'Hello world',
           'created_at' => created_at.to_s,
           'updated_at' => updated_at.to_s,
-          'github_id' => 1
+          'note_id' => 1
         }
       end
 
@@ -124,7 +124,7 @@ RSpec.describe Gitlab::GithubImport::Representation::DiffNote do
         'note' => 'Hello world',
         'created_at' => created_at.to_s,
         'updated_at' => updated_at.to_s,
-        'github_id' => 1
+        'note_id' => 1
       }
 
       note = described_class.from_json_hash(hash)
@@ -154,7 +154,7 @@ RSpec.describe Gitlab::GithubImport::Representation::DiffNote do
         'note' => 'Hello world',
         'created_at' => created_at.to_s,
         'updated_at' => updated_at.to_s,
-        'github_id' => 1
+        'note_id' => 1
       )
 
       expect(note.diff_hash).to eq(
@@ -167,4 +167,18 @@ RSpec.describe Gitlab::GithubImport::Representation::DiffNote do
       )
     end
   end
+
+  describe '#github_identifiers' do
+    it 'returns a hash with needed identifiers' do
+      github_identifiers = {
+        noteable_id: 42,
+        noteable_type: 'MergeRequest',
+        note_id: 1
+      }
+      other_attributes = { something_else: '_something_else_' }
+      note = described_class.new(github_identifiers.merge(other_attributes))
+
+      expect(note.github_identifiers).to eq(github_identifiers)
+    end
+  end
 end

spec/lib/gitlab/github_import/representation/issue_spec.rb

@@ -181,4 +181,17 @@ RSpec.describe Gitlab::GithubImport::Representation::Issue do
       expect(object.truncated_title).to eq('foo')
     end
   end
+
+  describe '#github_identifiers' do
+    it 'returns a hash with needed identifiers' do
+      github_identifiers = {
+        iid: 42,
+        issuable_type: 'MergeRequest'
+      }
+      other_attributes = { pull_request: true, something_else: '_something_else_' }
+      issue = described_class.new(github_identifiers.merge(other_attributes))
+
+      expect(issue.github_identifiers).to eq(github_identifiers)
+    end
+  end
 end

spec/lib/gitlab/github_import/representation/lfs_object_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::GithubImport::Representation::LfsObject do
+  describe '#github_identifiers' do
+    it 'returns a hash with needed identifiers' do
+      github_identifiers = {
+        oid: 42
+      }
+      other_attributes = { something_else: '_something_else_' }
+      lfs_object = described_class.new(github_identifiers.merge(other_attributes))
+
+      expect(lfs_object.github_identifiers).to eq(github_identifiers)
+    end
+  end
+end

spec/lib/gitlab/github_import/representation/note_spec.rb

@@ -40,8 +40,8 @@ RSpec.describe Gitlab::GithubImport::Representation::Note do
         expect(note.updated_at).to eq(updated_at)
       end
 
-      it 'includes the GitHub ID' do
-        expect(note.github_id).to eq(1)
+      it 'includes the note ID' do
+        expect(note.note_id).to eq(1)
       end
     end
   end
@@ -84,7 +84,7 @@ RSpec.describe Gitlab::GithubImport::Representation::Note do
           'note' => 'Hello world',
           'created_at' => created_at.to_s,
           'updated_at' => updated_at.to_s,
-          'github_id' => 1
+          'note_id' => 1
         }
       end
 
@@ -98,7 +98,7 @@ RSpec.describe Gitlab::GithubImport::Representation::Note do
         'note' => 'Hello world',
         'created_at' => created_at.to_s,
         'updated_at' => updated_at.to_s,
-        'github_id' => 1
+        'note_id' => 1
       }
 
       note = described_class.from_json_hash(hash)
@@ -106,4 +106,18 @@ RSpec.describe Gitlab::GithubImport::Representation::Note do
       expect(note.author).to be_nil
     end
   end
+
+  describe '#github_identifiers' do
+    it 'returns a hash with needed identifiers' do
+      github_identifiers = {
+        noteable_id: 42,
+        noteable_type: 'Issue',
+        note_id: 1
+      }
+      other_attributes = { something_else: '_something_else_' }
+      note = described_class.new(github_identifiers.merge(other_attributes))
+
+      expect(note.github_identifiers).to eq(github_identifiers)
+    end
+  end
 end

spec/lib/gitlab/github_import/representation/pull_request_review_spec.rb

@@ -14,7 +14,7 @@ RSpec.describe Gitlab::GithubImport::Representation::PullRequestReview do
       expect(review.note).to eq('note')
       expect(review.review_type).to eq('APPROVED')
       expect(review.submitted_at).to eq(submitted_at)
-      expect(review.github_id).to eq(999)
+      expect(review.review_id).to eq(999)
       expect(review.merge_request_id).to eq(42)
     end
   end
@@ -50,7 +50,7 @@ RSpec.describe Gitlab::GithubImport::Representation::PullRequestReview do
   describe '.from_json_hash' do
     let(:hash) do
       {
-        'github_id' => 999,
+        'review_id' => 999,
         'merge_request_id' => 42,
         'note' => 'note',
         'review_type' => 'APPROVED',
@@ -75,4 +75,17 @@ RSpec.describe Gitlab::GithubImport::Representation::PullRequestReview do
       expect(review.submitted_at).to be_nil
     end
   end
+
+  describe '#github_identifiers' do
+    it 'returns a hash with needed identifiers' do
+      github_identifiers = {
+        review_id: 999,
+        merge_request_id: 42
+      }
+      other_attributes = { something_else: '_something_else_' }
+      review = described_class.new(github_identifiers.merge(other_attributes))
+
+      expect(review.github_identifiers).to eq(github_identifiers)
+    end
+  end
 end

spec/lib/gitlab/github_import/representation/pull_request_spec.rb

@@ -288,4 +288,16 @@ RSpec.describe Gitlab::GithubImport::Representation::PullRequest do
       expect(object.truncated_title).to eq('foo')
     end
   end
+
+  describe '#github_identifiers' do
+    it 'returns a hash with needed identifiers' do
+      github_identifiers = {
+        iid: 1
+      }
+      other_attributes = { something_else: '_something_else_' }
+      pr = described_class.new(github_identifiers.merge(other_attributes))
+
+      expect(pr.github_identifiers).to eq(github_identifiers.merge(issuable_type: 'MergeRequest'))
+    end
+  end
 end

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

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Gitlab::GithubImport::ObjectImporter do
+RSpec.describe Gitlab::GithubImport::ObjectImporter, :aggregate_failures do
   let(:worker) do
     Class.new do
       def self.name
@@ -26,9 +26,15 @@ RSpec.describe Gitlab::GithubImport::ObjectImporter do
   let(:importer_class) { double(:importer_class, name: 'klass_name') }
   let(:importer_instance) { double(:importer_instance) }
   let(:client) { double(:client) }
+  let(:github_identifiers) do
+    {
+      some_id: 1,
+      some_type: '_some_type_'
+    }
+  end
 
-  before do
-    stub_const('MockRepresantation', Class.new do
+  let(:representation_class) do
+    Class.new do
       include Gitlab::GithubImport::Representation::ToHash
       include Gitlab::GithubImport::Representation::ExposeAttribute
 
@@ -41,7 +47,20 @@ RSpec.describe Gitlab::GithubImport::ObjectImporter do
       def initialize(attributes)
         @attributes = attributes
       end
-    end)
+
+      def github_identifiers
+        {
+          some_id: 1,
+          some_type: '_some_type_'
+        }
+      end
+    end
+  end
+
+  let(:stubbed_representation) { representation_class }
+
+  before do
+    stub_const('MockRepresantation', stubbed_representation)
   end
 
   describe '#import', :clean_gitlab_redis_cache do
@@ -64,7 +83,7 @@ RSpec.describe Gitlab::GithubImport::ObjectImporter do
       expect(Gitlab::GithubImport::Logger)
         .to receive(:info)
         .with(
-          github_id: 1,
+          github_identifiers: github_identifiers,
           message: 'starting importer',
           project_id: project.id,
           importer: 'klass_name'
@@ -73,7 +92,7 @@ RSpec.describe Gitlab::GithubImport::ObjectImporter do
       expect(Gitlab::GithubImport::Logger)
         .to receive(:info)
         .with(
-          github_id: 1,
+          github_identifiers: github_identifiers,
           message: 'importer finished',
           project_id: project.id,
           importer: 'klass_name'
@@ -101,7 +120,7 @@ RSpec.describe Gitlab::GithubImport::ObjectImporter do
       expect(Gitlab::GithubImport::Logger)
         .to receive(:info)
         .with(
-          github_id: 1,
+          github_identifiers: github_identifiers,
           message: 'starting importer',
           project_id: project.id,
           importer: 'klass_name'
@@ -125,21 +144,25 @@ RSpec.describe Gitlab::GithubImport::ObjectImporter do
       expect(project.import_failures.last.exception_message).to eq('some error')
     end
 
-    it 'logs error when representation does not have a github_id' do
-      expect(importer_class).not_to receive(:new)
+    context 'without github_identifiers defined' do
+      let(:stubbed_representation) { representation_class.instance_eval { undef_method :github_identifiers } }
 
-      expect(Gitlab::Import::ImportFailureService)
-        .to receive(:track)
-        .with(
-          project_id: project.id,
-          exception: a_kind_of(KeyError),
-          error_source: 'klass_name',
-          fail_import: true
-        )
-        .and_call_original
+      it 'logs error when representation does not have a github_id' do
+        expect(importer_class).not_to receive(:new)
 
-      expect { worker.import(project, client, { 'number' => 10 }) }
-        .to raise_error(KeyError, 'key not found: :github_id')
+        expect(Gitlab::Import::ImportFailureService)
+          .to receive(:track)
+          .with(
+            project_id: project.id,
+            exception: a_kind_of(NoMethodError),
+            error_source: 'klass_name',
+            fail_import: true
+          )
+          .and_call_original
+
+        expect { worker.import(project, client, { 'number' => 10 }) }
+          .to raise_error(NoMethodError, /^undefined method `github_identifiers/)
+      end
     end
   end
 end