From ba48ae1dd18987a0df94080fab443a8dfed1feb5 Mon Sep 17 00:00:00 2001 From: George Koltsov Date: Fri, 24 Jul 2020 11:17:16 +0100 Subject: [PATCH] Remove LegacyGitHubClient usage from Gitea Importer - Replace use of legacy github client from Gitea Importer with github parallel importer with an end goal of removing legacy code from the codebase --- app/controllers/import/gitea_controller.rb | 18 ++--- app/controllers/import/github_controller.rb | 8 ++- .../import_issues_and_diff_notes_worker.rb | 3 + lib/gitlab/github_import.rb | 19 +++++- lib/gitlab/github_import/client.rb | 12 +++- .../github_import/importer/issue_importer.rb | 2 +- .../github_import/representation/issue.rb | 6 +- .../github_import/representation/note.rb | 4 +- lib/gitlab/import_sources.rb | 2 +- spec/lib/gitlab/github_import/client_spec.rb | 10 +++ .../importer/issue_importer_spec.rb | 24 +++++++ .../representation/issue_spec.rb | 25 +++++++ .../github_import/representation/note_spec.rb | 25 +++++++ .../github_import/sequential_importer_spec.rb | 2 +- spec/lib/gitlab/github_import_spec.rb | 66 ++++++++++++++----- spec/lib/gitlab/import_sources_spec.rb | 4 +- 16 files changed, 189 insertions(+), 41 deletions(-) diff --git a/app/controllers/import/gitea_controller.rb b/app/controllers/import/gitea_controller.rb index 4785a71b8a1d78..9ff1d1fb327b9b 100644 --- a/app/controllers/import/gitea_controller.rb +++ b/app/controllers/import/gitea_controller.rb @@ -54,19 +54,15 @@ def provider_auth end end - override :client_repos - def client_repos - @client_repos ||= filtered(client.repos) - end - - override :client - def client - @client ||= Gitlab::LegacyGithubImport::Client.new(session[access_token_key], client_options) - end - override :client_options def client_options - { host: provider_url, api_version: 'v1' } + options = { host: provider_url, api_version: 'v1' } + + if Feature.enabled?(:remove_legacy_github_client) + options[:parallel] = true + end + + options end def verify_blocked_uri diff --git a/app/controllers/import/github_controller.rb b/app/controllers/import/github_controller.rb index 998439b2120427..2e31e6ca527b6b 100644 --- a/app/controllers/import/github_controller.rb +++ b/app/controllers/import/github_controller.rb @@ -106,7 +106,7 @@ def expire_etag_cache def client @client ||= if Feature.enabled?(:remove_legacy_github_client, default_enabled: false) - Gitlab::GithubImport::Client.new(session[access_token_key]) + Gitlab::GithubImport::Client.new(session[access_token_key], client_options) else Gitlab::LegacyGithubImport::Client.new(session[access_token_key], client_options) end @@ -229,7 +229,11 @@ def ci_cd_only? end def client_options - { wait_for_rate_limit_reset: false } + if Feature.enabled?(:remove_legacy_github_client) + { parallel: true } + else + { wait_for_rate_limit_reset: false } + end end def extra_import_params diff --git a/app/workers/gitlab/github_import/stage/import_issues_and_diff_notes_worker.rb b/app/workers/gitlab/github_import/stage/import_issues_and_diff_notes_worker.rb index 68b6e159fa4715..b5bda4173413b0 100644 --- a/app/workers/gitlab/github_import/stage/import_issues_and_diff_notes_worker.rb +++ b/app/workers/gitlab/github_import/stage/import_issues_and_diff_notes_worker.rb @@ -19,6 +19,9 @@ class ImportIssuesAndDiffNotesWorker # rubocop:disable Scalability/IdempotentWor # project - An instance of Project. def import(client, project) waiters = IMPORTERS.each_with_object({}) do |klass, hash| + # Gitea does not support pull request comments + next if project.gitea_import? && klass == Importer::DiffNotesImporter + waiter = klass.new(project, client).execute hash[waiter.key] = waiter.jobs_remaining end diff --git a/lib/gitlab/github_import.rb b/lib/gitlab/github_import.rb index 9a7c406d981ddd..3c7ea32d54e7c9 100644 --- a/lib/gitlab/github_import.rb +++ b/lib/gitlab/github_import.rb @@ -2,6 +2,8 @@ module Gitlab module GithubImport + GITEA_API_VERSION = 'v1' + def self.refmap [:heads, :tags, '+refs/pull/*/head:refs/merge-requests/*/head'] end @@ -9,7 +11,13 @@ def self.refmap def self.new_client_for(project, token: nil, parallel: true) token_to_use = token || project.import_data&.credentials&.fetch(:user) - Client.new(token_to_use, parallel: parallel) + opts = { + parallel: parallel + } + + opts.merge!(self.gitea_opts(project)) if project.gitea_import? + + Client.new(token_to_use, opts) end # Returns the ID of the ghost user. @@ -18,5 +26,14 @@ def self.ghost_user_id Gitlab::Cache::Import::Caching.read_integer(key) || Gitlab::Cache::Import::Caching.write(key, User.select(:id).ghost.id) end + + def self.gitea_opts(project) + uri = URI.parse(project.import_url) + + { + host: "#{uri.scheme}://#{uri.host}:#{uri.port}", + api_version: GITEA_API_VERSION + } + end end end diff --git a/lib/gitlab/github_import/client.rb b/lib/gitlab/github_import/client.rb index 22803c5cd71ad5..f0f95d10cf2b13 100644 --- a/lib/gitlab/github_import/client.rb +++ b/lib/gitlab/github_import/client.rb @@ -39,7 +39,9 @@ class Client # this value to `true` for parallel importing is crucial as # otherwise hitting the rate limit will result in a thread # being blocked in a `sleep()` call for up to an hour. - def initialize(token, per_page: 100, parallel: true) + def initialize(token, per_page: 100, parallel: true, host: nil, api_version: nil) + @host = host.to_s.sub(%r{/+\z}, '') + @api_version = api_version @octokit = ::Octokit::Client.new( access_token: token, per_page: per_page, @@ -181,7 +183,7 @@ def rate_limiting_enabled? end def api_endpoint - custom_api_endpoint || default_api_endpoint + passed_in_endpoint || custom_api_endpoint || default_api_endpoint end def custom_api_endpoint @@ -213,6 +215,12 @@ def request_count_counter 'The number of GitHub API calls performed when importing projects' ) end + + private + + def passed_in_endpoint + "#{@host}/api/#{@api_version}" if @host.present? && @api_version.present? + end end end end diff --git a/lib/gitlab/github_import/importer/issue_importer.rb b/lib/gitlab/github_import/importer/issue_importer.rb index 13061d2c9df730..530afbaf46e03a 100644 --- a/lib/gitlab/github_import/importer/issue_importer.rb +++ b/lib/gitlab/github_import/importer/issue_importer.rb @@ -69,7 +69,7 @@ def create_issue def create_assignees(issue_id) assignees = [] - issue.assignees.each do |assignee| + Array.wrap(issue.assignees).each do |assignee| if (user_id = user_finder.user_id_for(assignee)) assignees << { issue_id: issue_id, user_id: user_id } end diff --git a/lib/gitlab/github_import/representation/issue.rb b/lib/gitlab/github_import/representation/issue.rb index f3071b3e2b3b1e..66b2be7ae9e024 100644 --- a/lib/gitlab/github_import/representation/issue.rb +++ b/lib/gitlab/github_import/representation/issue.rb @@ -29,10 +29,10 @@ def self.from_api_response(issue) description: issue.body, milestone_number: issue.milestone&.number, state: issue.state == 'open' ? :opened : :closed, - assignees: issue.assignees.map do |u| + assignees: Array.wrap(issue.assignees).map do |u| Representation::User.from_api_response(u) end, - label_names: issue.labels.map(&:name), + label_names: Array.wrap(issue.labels).map(&:name), author: user, created_at: issue.created_at, updated_at: issue.updated_at, @@ -47,7 +47,7 @@ def self.from_json_hash(raw_hash) hash = Representation.symbolize_hash(raw_hash) hash[:state] = hash[:state].to_sym - hash[:assignees].map! { |u| Representation::User.from_json_hash(u) } + Array.wrap(hash[:assignees]).map! { |u| Representation::User.from_json_hash(u) } hash[:author] &&= Representation::User.from_json_hash(hash[:author]) new(hash) diff --git a/lib/gitlab/github_import/representation/note.rb b/lib/gitlab/github_import/representation/note.rb index 5b98ce7d5edf04..5161f812b160df 100644 --- a/lib/gitlab/github_import/representation/note.rb +++ b/lib/gitlab/github_import/representation/note.rb @@ -12,7 +12,7 @@ class Note expose_attribute :noteable_id, :noteable_type, :author, :note, :created_at, :updated_at, :github_id - NOTEABLE_TYPE_REGEX = %r{/(?(pull|issues))/(?\d+)}i.freeze + NOTEABLE_TYPE_REGEX = %r{/(?(pulls?|issues))/(?\d+)}i.freeze # Builds a note from a GitHub API response. # @@ -28,7 +28,7 @@ def self.from_api_response(note) end noteable_type = - if matches[:type] == 'pull' + if matches[:type].match?(/^pulls?$/) 'MergeRequest' else 'Issue' diff --git a/lib/gitlab/import_sources.rb b/lib/gitlab/import_sources.rb index 58c7744fae0957..e73e2be99c9d7f 100644 --- a/lib/gitlab/import_sources.rb +++ b/lib/gitlab/import_sources.rb @@ -19,7 +19,7 @@ module ImportSources ImportSource.new('fogbugz', 'FogBugz', Gitlab::FogbugzImport::Importer), ImportSource.new('git', 'Repo by URL', nil), ImportSource.new('gitlab_project', 'GitLab export', Gitlab::ImportExport::Importer), - ImportSource.new('gitea', 'Gitea', Gitlab::LegacyGithubImport::Importer), + ImportSource.new('gitea', 'Gitea', Gitlab::GithubImport::ParallelImporter), ImportSource.new('manifest', 'Manifest file', nil), ImportSource.new('phabricator', 'Phabricator', Gitlab::PhabricatorImport::Importer) ].freeze diff --git a/spec/lib/gitlab/github_import/client_spec.rb b/spec/lib/gitlab/github_import/client_spec.rb index 5f6ab42d0d2d05..09d8c3b1b01076 100644 --- a/spec/lib/gitlab/github_import/client_spec.rb +++ b/spec/lib/gitlab/github_import/client_spec.rb @@ -297,6 +297,16 @@ expect(client.api_endpoint).to eq(endpoint) end end + + context 'with passed in endpoint' do + let(:client) { described_class.new('foo', host: 'https://example.com', api_version: 'v2') } + + it 'returns passed in endpoint' do + endpoint = 'https://example.com/api/v2' + + expect(client.api_endpoint).to eq(endpoint) + end + end end describe '#custom_api_endpoint' do diff --git a/spec/lib/gitlab/github_import/importer/issue_importer_spec.rb b/spec/lib/gitlab/github_import/importer/issue_importer_spec.rb index fb826c987e1486..a66421257094cf 100644 --- a/spec/lib/gitlab/github_import/importer/issue_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/issue_importer_spec.rb @@ -70,6 +70,30 @@ importer.execute end + + context 'when there are no assignees' do + let(:issue) do + Gitlab::GithubImport::Representation::Issue.new( + iid: 1, + title: 'issue', + description: 'issue', + state: :opened, + assignees: nil, + author: Gitlab::GithubImport::Representation::User.new(id: 5, login: 'test'), + created_at: created_at, + updated_at: updated_at, + pull_request: false + ) + end + + before do + allow(importer).to receive(:create_issue).and_return(10) + end + + it 'does not raise error' do + expect { importer.execute }.not_to raise_error + end + end end describe '#create_issue' do diff --git a/spec/lib/gitlab/github_import/representation/issue_spec.rb b/spec/lib/gitlab/github_import/representation/issue_spec.rb index 3d306a4a3a3d17..d7e29c5a48f453 100644 --- a/spec/lib/gitlab/github_import/representation/issue_spec.rb +++ b/spec/lib/gitlab/github_import/representation/issue_spec.rb @@ -97,6 +97,31 @@ expect(issue.author).to be_nil end + + context 'when collections in the response return nil' do + let(:response) do + double( + :response, + number: 1, + title: 'issue', + body: 'issue', + milestone: double(:milestone, number: 1), + state: 'open', + assignees: nil, + labels: nil, + user: double(:user, id: 1, login: 'test'), + created_at: created_at, + updated_at: updated_at, + pull_request: false + ) + end + + let(:issue) { described_class.from_api_response(response) } + + it 'does not raise error' do + expect { issue }.not_to raise_error + end + end end describe '.from_json_hash' do diff --git a/spec/lib/gitlab/github_import/representation/note_spec.rb b/spec/lib/gitlab/github_import/representation/note_spec.rb index 112bb7eb908e44..02102cc2292f14 100644 --- a/spec/lib/gitlab/github_import/representation/note_spec.rb +++ b/spec/lib/gitlab/github_import/representation/note_spec.rb @@ -63,6 +63,31 @@ let(:note) { described_class.from_api_response(response) } end + context 'when resource is merge request' do + shared_examples 'a merge request note' do |html_url| + let(:response) do + double( + :response, + html_url: html_url, + user: double(:user, id: 4, login: 'alice'), + body: 'Hello world', + created_at: created_at, + updated_at: updated_at, + id: 1 + ) + end + + let(:note) { described_class.from_api_response(response) } + + it 'includes notable type' do + expect(note.noteable_type).to eq('MergeRequest') + end + end + + it_behaves_like 'a merge request note', 'https://github.com/foo/bar/pull/42' + it_behaves_like 'a merge request note', 'https://github.com/foo/bar/pulls/42' + end + it 'does not set the user if the response did not include a user' do allow(response) .to receive(:user) diff --git a/spec/lib/gitlab/github_import/sequential_importer_spec.rb b/spec/lib/gitlab/github_import/sequential_importer_spec.rb index fe13fcd2568829..e0617e221ddf2c 100644 --- a/spec/lib/gitlab/github_import/sequential_importer_spec.rb +++ b/spec/lib/gitlab/github_import/sequential_importer_spec.rb @@ -6,7 +6,7 @@ describe '#execute' do it 'imports a project in sequence' do repository = double(:repository) - project = double(:project, id: 1, repository: repository) + project = double(:project, id: 1, repository: repository, gitea_import?: false) importer = described_class.new(project, token: 'foo') expect_next_instance_of(Gitlab::GithubImport::Importer::RepositoryImporter) do |instance| diff --git a/spec/lib/gitlab/github_import_spec.rb b/spec/lib/gitlab/github_import_spec.rb index 1a690b81d2b45a..56a293b307c222 100644 --- a/spec/lib/gitlab/github_import_spec.rb +++ b/spec/lib/gitlab/github_import_spec.rb @@ -6,26 +6,47 @@ let(:project) { double(:project) } describe '.new_client_for' do - it 'returns a new Client with a custom token' do - expect(described_class::Client) - .to receive(:new) - .with('123', parallel: true) + context 'when importing from GitHub' do + before do + allow(project).to receive(:gitea_import?).and_return(false) + end - described_class.new_client_for(project, token: '123') - end + it 'returns a new Client with a custom token' do + expect(described_class::Client) + .to receive(:new) + .with('123', parallel: true) - it 'returns a new Client with a token stored in the import data' do - import_data = double(:import_data, credentials: { user: '123' }) + described_class.new_client_for(project, token: '123') + end - expect(project) - .to receive(:import_data) - .and_return(import_data) + it 'returns a new Client with a token stored in the import data' do + import_data = double(:import_data, credentials: { user: '123' }) - expect(described_class::Client) - .to receive(:new) - .with('123', parallel: true) + expect(project) + .to receive(:import_data) + .and_return(import_data) - described_class.new_client_for(project) + expect(described_class::Client) + .to receive(:new) + .with('123', parallel: true) + + described_class.new_client_for(project) + end + end + + context 'when importing from Gitea' do + before do + allow(project).to receive(:gitea_import?).and_return(true) + allow(project).to receive(:import_url).and_return('https://gitea.example.com/testing/repo.git') + end + + it 'returns a new Client with gitea opts' do + expect(described_class::Client) + .to receive(:new) + .with('123', parallel: true, host: 'https://gitea.example.com:443', api_version: 'v1') + + described_class.new_client_for(project, token: '123') + end end end @@ -45,4 +66,19 @@ end end end + + describe '.gitea_opts' do + before do + allow(project).to receive(:import_url).and_return('https://gitea.example.com/testing/repo.git') + end + + it 'returns host and api version' do + expected_opts = { + host: 'https://gitea.example.com:443', + api_version: 'v1' + } + + expect(described_class.gitea_opts(project)).to eq(expected_opts) + end + end end diff --git a/spec/lib/gitlab/import_sources_spec.rb b/spec/lib/gitlab/import_sources_spec.rb index 0dfd8a2ee505a7..53e8c92c0e1448 100644 --- a/spec/lib/gitlab/import_sources_spec.rb +++ b/spec/lib/gitlab/import_sources_spec.rb @@ -74,7 +74,7 @@ 'fogbugz' => Gitlab::FogbugzImport::Importer, 'git' => nil, 'gitlab_project' => Gitlab::ImportExport::Importer, - 'gitea' => Gitlab::LegacyGithubImport::Importer, + 'gitea' => Gitlab::GithubImport::ParallelImporter, 'manifest' => nil, 'phabricator' => Gitlab::PhabricatorImport::Importer } @@ -109,7 +109,7 @@ end describe 'imports_repository? checker' do - let(:allowed_importers) { %w[github gitlab_project bitbucket_server phabricator] } + let(:allowed_importers) { %w[github gitea gitlab_project bitbucket_server phabricator] } it 'fails if any importer other than the allowed ones implements this method' do current_importers = described_class.values.select { |kind| described_class.importer(kind).try(:imports_repository?) } -- GitLab