Merge branch 'feature/24072-improve-importing-of-github-pull-requests' into 'master'

Improve importing of github pull requests

Closes #24072

See merge request !7241

5 files changed, 38 insertions, 33 deletions

diff --git a/changelogs/unreleased/24072-improve-importing-of-github-pull-requests.yml b/changelogs/unreleased/24072-improve-importing-of-github-pull-requests.yml
new file mode 100644
index 00000000000..2c265960d67
--- /dev/null
+++ b/changelogs/unreleased/24072-improve-importing-of-github-pull-requests.yml
@@ -0,0 +1,4 @@
+---
+title: Reduce API calls needed when importing issues and pull requests from GitHub
+merge_request: 7241
+author: Andrew Smith (EspadaV8)
diff --git a/lib/gitlab/github_import/importer.rb b/lib/gitlab/github_import/importer.rb
index 90cf38a8513..281b65bdeba 100644
--- a/lib/gitlab/github_import/importer.rb
+++ b/lib/gitlab/github_import/importer.rb
@@ -20,10 +20,18 @@ module Gitlab
       end
 
       def execute
+        # The ordering of importing is important here due to the way GitHub structures their data
+        # 1. Labels are required by other items while not having a dependency on anything else
+        # so need to be first
+        # 2. Pull requests must come before issues. Every pull request is also an issue but not
+        # all issues are pull requests. Only the issue entity has labels defined in GitHub. GitLab
+        # doesn't structure data like this so we need to make sure that we've created the MRs
+        # before we attempt to add the labels defined in the GitHub issue for the related, already
+        # imported, pull request
         import_labels
         import_milestones
-        import_issues
         import_pull_requests
+        import_issues
         import_comments(:issues)
         import_comments(:pull_requests)
         import_wiki
@@ -79,13 +87,17 @@ module Gitlab
           issues.each do |raw|
             gh_issue = IssueFormatter.new(project, raw)
 
-            if gh_issue.valid?
-              begin
-                issue = gh_issue.create!
-                apply_labels(issue, raw)
-              rescue => e
-                errors << { type: :issue, url: Gitlab::UrlSanitizer.sanitize(raw.url), errors: e.message }
-              end
+            begin
+              issuable =
+                if gh_issue.pull_request?
+                  MergeRequest.find_by_iid(gh_issue.number)
+                else
+                  gh_issue.create!
+                end
+
+              apply_labels(issuable, raw)
+            rescue => e
+              errors << { type: :issue, url: Gitlab::UrlSanitizer.sanitize(raw.url), errors: e.message }
             end
           end
         end
@@ -101,8 +113,7 @@ module Gitlab
               restore_source_branch(pull_request) unless pull_request.source_branch_exists?
               restore_target_branch(pull_request) unless pull_request.target_branch_exists?
 
-              merge_request = pull_request.create!
-              apply_labels(merge_request, raw)
+              pull_request.create!
             rescue => e
               errors << { type: :pull_request, url: Gitlab::UrlSanitizer.sanitize(pull_request.url), errors: e.message }
             ensure
@@ -133,21 +144,14 @@ module Gitlab
         remove_branch(pull_request.target_branch_name) unless pull_request.target_branch_exists?
       end
 
-      def apply_labels(issuable, raw_issuable)
-        # GH returns labels for issues but not for pull requests!
-        labels = if issuable.is_a?(MergeRequest)
-                   client.labels_for_issue(repo, raw_issuable.number)
-                 else
-                   raw_issuable.labels
-                 end
+      def apply_labels(issuable, raw)
+        return unless raw.labels.count > 0
 
-        if labels.count > 0
-          label_ids = labels
-            .map { |attrs| @labels[attrs.name] }
-            .compact
+        label_ids = raw.labels
+          .map { |attrs| @labels[attrs.name] }
+          .compact
 
-          issuable.update_attribute(:label_ids, label_ids)
-        end
+        issuable.update_attribute(:label_ids, label_ids)
       end
 
       def import_comments(issuable_type)
diff --git a/lib/gitlab/github_import/issue_formatter.rb b/lib/gitlab/github_import/issue_formatter.rb
index 8c32ac59fc5..887690bcc7c 100644
--- a/lib/gitlab/github_import/issue_formatter.rb
+++ b/lib/gitlab/github_import/issue_formatter.rb
@@ -32,8 +32,8 @@ module Gitlab
         raw_data.number
       end
 
-      def valid?
-        raw_data.pull_request.nil?
+      def pull_request?
+        raw_data.pull_request.present?
       end
 
       private
diff --git a/spec/lib/gitlab/github_import/importer_spec.rb b/spec/lib/gitlab/github_import/importer_spec.rb
index 7478f86bd28..000b9aa6f83 100644
--- a/spec/lib/gitlab/github_import/importer_spec.rb
+++ b/spec/lib/gitlab/github_import/importer_spec.rb
@@ -101,7 +101,6 @@ describe Gitlab::GithubImport::Importer, lib: true do
           closed_at: nil,
           merged_at: nil,
           url: 'https://api.github.com/repos/octocat/Hello-World/pulls/1347',
-          labels: [double(name: 'Label #3')],
         )
       end
 
@@ -157,8 +156,6 @@ describe Gitlab::GithubImport::Importer, lib: true do
           errors: [
             { type: :label, url: "https://api.github.com/repos/octocat/Hello-World/labels/bug", errors: "Validation failed: Title can't be blank, Title is invalid" },
             { type: :issue, url: "https://api.github.com/repos/octocat/Hello-World/issues/1348", errors: "Validation failed: Title can't be blank, Title is too short (minimum is 0 characters)" },
-            { type: :pull_request, url: "https://api.github.com/repos/octocat/Hello-World/pulls/1347", errors: "Invalid Repository. Use user/repo format." },
-            { type: :pull_request, url: "https://api.github.com/repos/octocat/Hello-World/pulls/1347", errors: "Invalid Repository. Use user/repo format." },
             { type: :wiki, errors: "Gitlab::Shell::Error" },
             { type: :release, url: 'https://api.github.com/repos/octocat/Hello-World/releases/2', errors: "Validation failed: Description can't be blank" }
           ]
diff --git a/spec/lib/gitlab/github_import/issue_formatter_spec.rb b/spec/lib/gitlab/github_import/issue_formatter_spec.rb
index c2f1f6b91a1..95339e2f128 100644
--- a/spec/lib/gitlab/github_import/issue_formatter_spec.rb
+++ b/spec/lib/gitlab/github_import/issue_formatter_spec.rb
@@ -144,20 +144,20 @@ describe Gitlab::GithubImport::IssueFormatter, lib: true do
     end
   end
 
-  describe '#valid?' do
+  describe '#pull_request?' do
     context 'when mention a pull request' do
       let(:raw_data) { double(base_data.merge(pull_request: double)) }
 
-      it 'returns false' do
-        expect(issue.valid?).to eq false
+      it 'returns true' do
+        expect(issue.pull_request?).to eq true
       end
     end
 
     context 'when does not mention a pull request' do
       let(:raw_data) { double(base_data.merge(pull_request: nil)) }
 
-      it 'returns true' do
-        expect(issue.valid?).to eq true
+      it 'returns false' do
+        expect(issue.pull_request?).to eq false
       end
     end
   end