Merge branch '38198-fetch-github-api-per-100' into 'master'

Resolve "GitHub import should fetch 100 results per page to limit the change to hit rate limiting"

Closes #38198

See merge request gitlab-org/gitlab-ce!14421

lib/github/client.rb

 module Github
   class Client
     TIMEOUT = 60
+    DEFAULT_PER_PAGE = 100
 
     attr_reader :connection, :rate_limit
 
@@ -20,7 +21,7 @@ module Github
       exceed, reset_in = rate_limit.get
       sleep reset_in if exceed
 
-      Github::Response.new(connection.get(url, query))
+      Github::Response.new(connection.get(url, { per_page: DEFAULT_PER_PAGE }.merge(query)))
     end
 
     private

lib/github/import.rb

@@ -202,13 +202,8 @@ module Github
             merge_request.save!(validate: false)
             merge_request.merge_request_diffs.create
 
-            # Fetch review comments
             review_comments_url = "/repos/#{repo}/pulls/#{pull_request.iid}/comments"
             fetch_comments(merge_request, :review_comment, review_comments_url, LegacyDiffNote)
-
-            # Fetch comments
-            comments_url = "/repos/#{repo}/issues/#{pull_request.iid}/comments"
-            fetch_comments(merge_request, :comment, comments_url)
           rescue => e
             error(:pull_request, pull_request.url, e.message)
           ensure
@@ -241,12 +236,17 @@ module Github
         # for both features, like manipulating assignees, labels
         # and milestones, are provided within the Issues API.
         if representation.pull_request?
-          return unless representation.has_labels?
+          return unless representation.has_labels? || representation.has_comments?
 
           merge_request = MergeRequest.find_by!(target_project_id: project.id, iid: representation.iid)
-          merge_request.update_attribute(:label_ids, label_ids(representation.labels))
+
+          if representation.has_labels?
+            merge_request.update_attribute(:label_ids, label_ids(representation.labels))
+          end
+
+          fetch_comments_conditionally(merge_request, representation)
         else
-          return if Issue.where(iid: representation.iid, project_id: project.id).exists?
+          return if Issue.exists?(iid: representation.iid, project_id: project.id)
 
           author_id          = user_id(representation.author, project.creator_id)
           issue              = Issue.new
@@ -263,17 +263,20 @@ module Github
           issue.updated_at   = representation.updated_at
           issue.save!(validate: false)
 
-          # Fetch comments
-          if representation.has_comments?
-            comments_url = "/repos/#{repo}/issues/#{issue.iid}/comments"
-            fetch_comments(issue, :comment, comments_url)
-          end
+          fetch_comments_conditionally(issue, representation)
         end
       rescue => e
         error(:issue, representation.url, e.message)
       end
     end
 
+    def fetch_comments_conditionally(issuable, representation)
+      if representation.has_comments?
+        comments_url = "/repos/#{repo}/issues/#{issuable.iid}/comments"
+        fetch_comments(issuable, :comment, comments_url)
+      end
+    end
+
     def fetch_comments(noteable, type, url, klass = Note)
       while url
         comments = Github::Client.new(options).get(url)

spec/lib/github/client_spec.rb

+require 'spec_helper'
+
+describe Github::Client do
+  let(:connection) { spy }
+  let(:rate_limit) { double(get: [false, 1]) }
+  let(:client) { described_class.new({}) }
+  let(:results) { double }
+  let(:response) { double }
+
+  before do
+    allow(Faraday).to receive(:new).and_return(connection)
+    allow(Github::RateLimit).to receive(:new).with(connection).and_return(rate_limit)
+  end
+
+  describe '#get' do
+    before do
+      allow(Github::Response).to receive(:new).with(results).and_return(response)
+    end
+
+    it 'uses a default per_page param' do
+      expect(connection).to receive(:get).with('/foo', per_page: 100).and_return(results)
+
+      expect(client.get('/foo')).to eq(response)
+    end
+
+    context 'with per_page given' do
+      it 'overwrites the default per_page' do
+        expect(connection).to receive(:get).with('/foo', per_page: 30).and_return(results)
+
+        expect(client.get('/foo', per_page: 30)).to eq(response)
+      end
+    end
+  end
+end