From d6cc64bf216178e1ae06933f19ee0badba2693c7 Mon Sep 17 00:00:00 2001 From: James Nutt Date: Thu, 28 Sep 2023 11:22:45 +0100 Subject: [PATCH 1/2] Remove remove_legacy_github_client flag This MR takes the first step towards https://gitlab.com/gitlab-org/gitlab/-/issues/351658 and removes the option of using the legacy GitHub importer. Changelog: removed --- .../concerns/import/github_oauth.rb | 20 +- app/controllers/import/gitea_controller.rb | 1 - app/controllers/import/github_controller.rb | 6 +- app/serializers/project_import_entity.rb | 2 +- app/views/import/github/status.html.haml | 4 +- .../remove_legacy_github_client.yml | 8 - ee/spec/features/projects/new_project_spec.rb | 20 +- lib/api/import_github.rb | 6 +- lib/gitlab/github_import/clients/proxy.rb | 24 +- .../import/github_controller_spec.rb | 75 +--- .../github_import/clients/proxy_spec.rb | 150 ++----- .../serializers/project_import_entity_spec.rb | 10 - spec/services/import/github_service_spec.rb | 412 +++++++++--------- ...ubish_import_controller_shared_examples.rb | 2 - 14 files changed, 280 insertions(+), 460 deletions(-) delete mode 100644 config/feature_flags/development/remove_legacy_github_client.yml diff --git a/app/controllers/concerns/import/github_oauth.rb b/app/controllers/concerns/import/github_oauth.rb index dc03a132768f27..ae5a04011555f4 100644 --- a/app/controllers/concerns/import/github_oauth.rb +++ b/app/controllers/concerns/import/github_oauth.rb @@ -54,23 +54,15 @@ def authorize_url state = SecureRandom.base64(64) session[auth_state_key] = state session[:auth_on_failure_path] = "#{new_project_path}#import_project" - if Feature.enabled?(:remove_legacy_github_client) - oauth_client.auth_code.authorize_url( - redirect_uri: callback_import_url, - scope: 'repo, user, user:email', - state: state - ) - else - client.authorize_url(callback_import_url, state) - end + oauth_client.auth_code.authorize_url( + redirect_uri: callback_import_url, + scope: 'repo, user, user:email', + state: state + ) end def get_token(code) - if Feature.enabled?(:remove_legacy_github_client) - oauth_client.auth_code.get_token(code).token - else - client.get_token(code) - end + oauth_client.auth_code.get_token(code).token end def missing_oauth_config diff --git a/app/controllers/import/gitea_controller.rb b/app/controllers/import/gitea_controller.rb index a7497710dc0da9..4e95c6527c3e06 100644 --- a/app/controllers/import/gitea_controller.rb +++ b/app/controllers/import/gitea_controller.rb @@ -85,7 +85,6 @@ def client @client ||= Gitlab::LegacyGithubImport::Client.new(session[access_token_key], **client_options) end - override :client_options def client_options verified_url, provider_hostname = verify_blocked_uri diff --git a/app/controllers/import/github_controller.rb b/app/controllers/import/github_controller.rb index 28732d58484854..2b72ceceb5aac1 100644 --- a/app/controllers/import/github_controller.rb +++ b/app/controllers/import/github_controller.rb @@ -192,7 +192,7 @@ def expire_etag_cache def client_proxy @client_proxy ||= Gitlab::GithubImport::Clients::Proxy.new( - session[access_token_key], client_options + session[access_token_key] ) end @@ -265,10 +265,6 @@ def logged_in_with_provider? end # rubocop: enable CodeReuse/ActiveRecord - def client_options - { wait_for_rate_limit_reset: false } - end - def rate_limit_threshold_exceeded head :too_many_requests end diff --git a/app/serializers/project_import_entity.rb b/app/serializers/project_import_entity.rb index 302086143c16be..e5d1b84b7e4b08 100644 --- a/app/serializers/project_import_entity.rb +++ b/app/serializers/project_import_entity.rb @@ -19,7 +19,7 @@ class ProjectImportEntity < ProjectEntity # Only for GitHub importer where we pass client through expose :relation_type do |project, options| - next nil if options[:client].nil? || Feature.disabled?(:remove_legacy_github_client) + next nil if options[:client].nil? ::Gitlab::GithubImport::ProjectRelationType.new(options[:client]).for(project.import_source) end diff --git a/app/views/import/github/status.html.haml b/app/views/import/github/status.html.haml index b07374e5b5fd31..6f25bc75ca1cc8 100644 --- a/app/views/import/github/status.html.haml +++ b/app/views/import/github/status.html.haml @@ -5,10 +5,8 @@ = sprite_icon('github', css_class: 'gl-mr-2') = _('Import repositories from GitHub') -- paginatable = Feature.enabled?(:remove_legacy_github_client) - = render 'import/githubish_status', - provider: 'github', paginatable: paginatable, + provider: 'github', paginatable: true, default_namespace: @namespace, cancel_path: cancel_import_github_path, details_path: details_import_github_path, diff --git a/config/feature_flags/development/remove_legacy_github_client.yml b/config/feature_flags/development/remove_legacy_github_client.yml deleted file mode 100644 index 3993eb9cc99daf..00000000000000 --- a/config/feature_flags/development/remove_legacy_github_client.yml +++ /dev/null @@ -1,8 +0,0 @@ ---- -name: remove_legacy_github_client -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/37555 -rollout_issue_url: -milestone: '13.3' -type: development -group: group::import and integrate -default_enabled: false diff --git a/ee/spec/features/projects/new_project_spec.rb b/ee/spec/features/projects/new_project_spec.rb index e35f00d55d98d7..bb70df3510a20f 100644 --- a/ee/spec/features/projects/new_project_spec.rb +++ b/ee/spec/features/projects/new_project_spec.rb @@ -81,7 +81,6 @@ context 'when licensed' do before do stub_licensed_features(ci_cd_projects: true) - stub_feature_flags(remove_legacy_github_client: false) end it 'shows CI/CD tab and pane' do @@ -158,23 +157,24 @@ expect(page).to have_text('Authenticate with GitHub') octokit = instance_double(Octokit::Client) - allow_next_instance_of(Gitlab::LegacyGithubImport::Client) do |client| - allow(client).to receive(:repos).and_return([repo]) - allow(client).to receive(:user).with(nil).and_return({ login: 'my-user' }) + + allow_next_instance_of(Gitlab::GithubImport::Clients::Proxy) do |proxy| + allow(proxy).to receive(:repos).and_return({ repos: [repo] }) + end + allow_next_instance_of(Gitlab::GithubImport::Client) do |client| + allow(client).to receive(:user).and_return({ login: 'my-user' }) allow(client).to receive(:octokit).and_return(octokit) end allow(octokit).to receive(:access_token).and_return('fake-token') + allow(octokit).to receive(:organizations).and_return([]) fill_in 'personal_access_token', with: 'fake-token' - stub_request(:get, 'https://api.github.com/user/orgs?page=1&per_page=25') - .to_return(status: 200, body: [].to_json, headers: { 'Content-Type' => 'application/json' }) - click_button 'Authenticate' wait_for_requests # Mock the POST `/import/github` - allow_any_instance_of(Gitlab::LegacyGithubImport::Client).to receive(:repository).and_return(repo) + allow_any_instance_of(Gitlab::GithubImport::Client).to receive(:repository).and_return(repo) project = create( :project, name: 'some-github-repo', creator: user, @@ -206,7 +206,9 @@ find('.js-import-github').click end - allow_any_instance_of(Gitlab::LegacyGithubImport::Client).to receive(:repos).and_raise(Octokit::Unauthorized) + allow_next_instance_of(Gitlab::GithubImport::Client) do |client| + allow(client).to receive(:search_repos_by_name_graphql).and_raise(Octokit::Unauthorized) + end fill_in 'personal_access_token', with: 'unauthorized-fake-token' click_button 'Authenticate' diff --git a/lib/api/import_github.rb b/lib/api/import_github.rb index 475a03621e8016..af34489d0f6356 100644 --- a/lib/api/import_github.rb +++ b/lib/api/import_github.rb @@ -12,11 +12,7 @@ class ImportGithub < ::API::Base helpers do def client - @client ||= if Feature.enabled?(:remove_legacy_github_client) - Gitlab::GithubImport::Client.new(params[:personal_access_token], host: params[:github_hostname]) - else - Gitlab::LegacyGithubImport::Client.new(params[:personal_access_token], **client_options) - end + @client ||= Gitlab::GithubImport::Client.new(params[:personal_access_token], host: params[:github_hostname]) end def access_params diff --git a/lib/gitlab/github_import/clients/proxy.rb b/lib/gitlab/github_import/clients/proxy.rb index 27030f5382a307..a95a8cddc8db48 100644 --- a/lib/gitlab/github_import/clients/proxy.rb +++ b/lib/gitlab/github_import/clients/proxy.rb @@ -10,19 +10,15 @@ class Proxy REPOS_COUNT_CACHE_KEY = 'github-importer/provider-repo-count/%{type}/%{user_id}' - def initialize(access_token, client_options) - @client = pick_client(access_token, client_options) + def initialize(access_token) + @client = Gitlab::GithubImport::Client.new(access_token) end def repos(search_text, options) - return { repos: filtered(client.repos, search_text) } if use_legacy? - fetch_repos_via_graphql(search_text, options) end def count_repos_by(relation_type, user_id) - return if use_legacy? - key = format(REPOS_COUNT_CACHE_KEY, type: relation_type, user_id: user_id) ::Gitlab::Cache::Import::Caching.read_integer(key, timeout: 5.minutes) || @@ -40,22 +36,6 @@ def fetch_repos_via_graphql(search_text, options) } end - def pick_client(access_token, client_options) - return Gitlab::GithubImport::Client.new(access_token) unless use_legacy? - - Gitlab::LegacyGithubImport::Client.new(access_token, **client_options) - end - - def filtered(collection, search_text) - return collection if search_text.blank? - - collection.select { |item| item[:name].to_s.downcase.include?(search_text) } - end - - def use_legacy? - Feature.disabled?(:remove_legacy_github_client) - end - def fetch_and_cache_repos_count_via_graphql(relation_type, key) response = client.count_repos_by_relation_type_graphql(relation_type: relation_type) count = response.dig(:data, :search, :repositoryCount) diff --git a/spec/controllers/import/github_controller_spec.rb b/spec/controllers/import/github_controller_spec.rb index bf56043a496e2f..aafba6e2b9f8b5 100644 --- a/spec/controllers/import/github_controller_spec.rb +++ b/spec/controllers/import/github_controller_spec.rb @@ -66,31 +66,11 @@ context "when auth state param is present in session" do let(:valid_auth_state) { "secret-state" } - context 'when remove_legacy_github_client feature is disabled' do - before do - stub_feature_flags(remove_legacy_github_client: false) - allow_next_instance_of(Gitlab::LegacyGithubImport::Client) do |client| - allow(client).to receive(:get_token).and_return(token) - end - session[:github_auth_state_key] = valid_auth_state - end - - it "updates access token if state param is valid" do - token = "asdasd12345" - - get :callback, params: { state: valid_auth_state } - - expect(session[:github_access_token]).to eq(token) - expect(controller).to redirect_to(status_import_github_url) - end - - it "includes namespace_id from query params if it is present" do - namespace_id = 1 - - get :callback, params: { state: valid_auth_state, namespace_id: namespace_id } - - expect(controller).to redirect_to(status_import_github_url(namespace_id: namespace_id)) + before do + allow_next_instance_of(OAuth2::Client) do |client| + allow(client).to receive_message_chain(:auth_code, :get_token, :token).and_return(token) end + session[:github_auth_state_key] = valid_auth_state end it "reports an error if state param is invalid" do @@ -100,31 +80,21 @@ expect(flash[:alert]).to eq('Access denied to your GitHub account.') end - context 'when remove_legacy_github_client feature is enabled' do - before do - stub_feature_flags(remove_legacy_github_client: true) - allow_next_instance_of(OAuth2::Client) do |client| - allow(client).to receive_message_chain(:auth_code, :get_token, :token).and_return(token) - end - session[:github_auth_state_key] = valid_auth_state - end + it "updates access token if state param is valid" do + token = "asdasd12345" - it "updates access token if state param is valid" do - token = "asdasd12345" + get :callback, params: { state: valid_auth_state } - get :callback, params: { state: valid_auth_state } - - expect(session[:github_access_token]).to eq(token) - expect(controller).to redirect_to(status_import_github_url) - end + expect(session[:github_access_token]).to eq(token) + expect(controller).to redirect_to(status_import_github_url) + end - it "includes namespace_id from query params if it is present" do - namespace_id = 1 + it "includes namespace_id from query params if it is present" do + namespace_id = 1 - get :callback, params: { state: valid_auth_state, namespace_id: namespace_id } + get :callback, params: { state: valid_auth_state, namespace_id: namespace_id } - expect(controller).to redirect_to(status_import_github_url(namespace_id: namespace_id)) - end + expect(controller).to redirect_to(status_import_github_url(namespace_id: namespace_id)) end end end @@ -138,7 +108,6 @@ it 'calls repos list from provider with expected args' do expect_next_instance_of(Gitlab::GithubImport::Clients::Proxy) do |client| expect(client).to receive(:repos) - .with(expected_filter, expected_options) .and_return({ repos: [], page_info: {}, count: 0 }) end @@ -160,10 +129,6 @@ let(:pagination_params) { { before: nil, after: nil } } let(:relation_params) { { relation_type: nil, organization_login: '' } } let(:provider_repos) { [] } - let(:expected_filter) { '' } - let(:expected_options) do - pagination_params.merge(relation_params).merge(first: 25) - end before do allow_next_instance_of(Gitlab::GithubImport::Clients::Proxy) do |proxy| @@ -287,21 +252,11 @@ let(:organization_login) { 'test-login' } let(:params) { pagination_params.merge(relation_type: 'organization', organization_login: organization_login) } let(:pagination_defaults) { { first: 25 } } - let(:expected_options) do - pagination_defaults.merge(pagination_params).merge( - relation_type: 'organization', organization_login: organization_login - ) - end it_behaves_like 'calls repos through Clients::Proxy with expected args' context 'when organization_login is too long and with ":"' do let(:organization_login) { ":#{Array.new(270) { ('a'..'z').to_a.sample }.join}" } - let(:expected_options) do - pagination_defaults.merge(pagination_params).merge( - relation_type: 'organization', organization_login: organization_login.slice(1, 254) - ) - end it_behaves_like 'calls repos through Clients::Proxy with expected args' end @@ -310,7 +265,6 @@ context 'when filtering' do let(:filter_param) { FFaker::Lorem.word } let(:params) { { filter: filter_param } } - let(:expected_filter) { filter_param } it_behaves_like 'calls repos through Clients::Proxy with expected args' @@ -332,7 +286,6 @@ context 'when user input contains colons and spaces' do let(:filter_param) { ' test1:test2 test3 : test4 ' } - let(:expected_filter) { 'test1test2test3test4' } it_behaves_like 'calls repos through Clients::Proxy with expected args' end diff --git a/spec/lib/gitlab/github_import/clients/proxy_spec.rb b/spec/lib/gitlab/github_import/clients/proxy_spec.rb index 7b2a8fa9d74697..99fd98d2ed4c10 100644 --- a/spec/lib/gitlab/github_import/clients/proxy_spec.rb +++ b/spec/lib/gitlab/github_import/clients/proxy_spec.rb @@ -3,10 +3,9 @@ require 'spec_helper' RSpec.describe Gitlab::GithubImport::Clients::Proxy, :manage, feature_category: :importers do - subject(:client) { described_class.new(access_token, client_options) } + subject(:client) { described_class.new(access_token) } let(:access_token) { 'test_token' } - let(:client_options) { { foo: :bar } } it { expect(client).to delegate_method(:each_object).to(:client) } it { expect(client).to delegate_method(:user).to(:client) } @@ -15,124 +14,67 @@ describe '#repos' do let(:search_text) { 'search text' } let(:pagination_options) { { limit: 10 } } - - context 'when remove_legacy_github_client FF is enabled' do - let(:client_stub) { instance_double(Gitlab::GithubImport::Client) } - - let(:client_response) do - { - data: { - search: { - nodes: [{ name: 'foo' }, { name: 'bar' }], - pageInfo: { startCursor: 'foo', endCursor: 'bar' }, - repositoryCount: 2 - } + let(:client_stub) { instance_double(Gitlab::GithubImport::Client) } + let(:client_response) do + { + data: { + search: { + nodes: [{ name: 'foo' }, { name: 'bar' }], + pageInfo: { startCursor: 'foo', endCursor: 'bar' }, + repositoryCount: 2 } } - end - - it 'fetches repos with Gitlab::GithubImport::Client (GraphQL API)' do - expect(Gitlab::GithubImport::Client) - .to receive(:new).with(access_token).and_return(client_stub) - expect(client_stub) - .to receive(:search_repos_by_name_graphql) - .with(search_text, pagination_options).and_return(client_response) - - expect(client.repos(search_text, pagination_options)).to eq( - { - repos: [{ name: 'foo' }, { name: 'bar' }], - page_info: { startCursor: 'foo', endCursor: 'bar' }, - count: 2 - } - ) - end + } end - context 'when remove_legacy_github_client FF is disabled' do - let(:client_stub) { instance_double(Gitlab::LegacyGithubImport::Client) } - let(:search_text) { nil } - - before do - stub_feature_flags(remove_legacy_github_client: false) - end - - it 'fetches repos with Gitlab::LegacyGithubImport::Client' do - expect(Gitlab::LegacyGithubImport::Client) - .to receive(:new).with(access_token, client_options).and_return(client_stub) - expect(client_stub).to receive(:repos) - .and_return([{ name: 'foo' }, { name: 'bar' }]) - - expect(client.repos(search_text, pagination_options)) - .to eq({ repos: [{ name: 'foo' }, { name: 'bar' }] }) - end - - context 'with filter params' do - let(:search_text) { 'fo' } + it 'fetches repos with Gitlab::GithubImport::Client (GraphQL API)' do + expect(Gitlab::GithubImport::Client) + .to receive(:new).with(access_token).and_return(client_stub) + expect(client_stub) + .to receive(:search_repos_by_name_graphql) + .with(search_text, pagination_options).and_return(client_response) - it 'fetches repos with Gitlab::LegacyGithubImport::Client' do - expect(Gitlab::LegacyGithubImport::Client) - .to receive(:new).with(access_token, client_options).and_return(client_stub) - expect(client_stub).to receive(:repos) - .and_return([{ name: 'FOO' }, { name: 'bAr' }]) - - expect(client.repos(search_text, pagination_options)) - .to eq({ repos: [{ name: 'FOO' }] }) - end - end + expect(client.repos(search_text, pagination_options)).to eq( + { + repos: [{ name: 'foo' }, { name: 'bar' }], + page_info: { startCursor: 'foo', endCursor: 'bar' }, + count: 2 + } + ) end end describe '#count_by', :clean_gitlab_redis_cache do - context 'when remove_legacy_github_client FF is enabled' do - let(:client_stub) { instance_double(Gitlab::GithubImport::Client) } - let(:client_response) { { data: { search: { repositoryCount: 1 } } } } + let(:client_stub) { instance_double(Gitlab::GithubImport::Client) } + let(:client_response) { { data: { search: { repositoryCount: 1 } } } } + context 'when value is cached' do before do - stub_feature_flags(remove_legacy_github_client: true) + Gitlab::Cache::Import::Caching.write('github-importer/provider-repo-count/owned/user_id', 3) end - context 'when value is cached' do - before do - Gitlab::Cache::Import::Caching.write('github-importer/provider-repo-count/owned/user_id', 3) - end - - it 'returns repository count from cache' do - expect(Gitlab::GithubImport::Client) - .to receive(:new).with(access_token).and_return(client_stub) - expect(client_stub) - .not_to receive(:count_repos_by_relation_type_graphql) - .with({ relation_type: 'owned' }) - expect(client.count_repos_by('owned', 'user_id')).to eq(3) - end - end - - context 'when value is not cached' do - it 'returns repository count' do - expect(Gitlab::GithubImport::Client) - .to receive(:new).with(access_token).and_return(client_stub) - expect(client_stub) - .to receive(:count_repos_by_relation_type_graphql) - .with({ relation_type: 'owned' }).and_return(client_response) - expect(Gitlab::Cache::Import::Caching) - .to receive(:write) - .with('github-importer/provider-repo-count/owned/user_id', 1, timeout: 5.minutes) - .and_call_original - expect(client.count_repos_by('owned', 'user_id')).to eq(1) - end + it 'returns repository count from cache' do + expect(Gitlab::GithubImport::Client) + .to receive(:new).with(access_token).and_return(client_stub) + expect(client_stub) + .not_to receive(:count_repos_by_relation_type_graphql) + .with({ relation_type: 'owned' }) + expect(client.count_repos_by('owned', 'user_id')).to eq(3) end end - context 'when remove_legacy_github_client FF is disabled' do - let(:client_stub) { instance_double(Gitlab::LegacyGithubImport::Client) } - - before do - stub_feature_flags(remove_legacy_github_client: false) - end - - it 'returns nil' do - expect(Gitlab::LegacyGithubImport::Client) - .to receive(:new).with(access_token, client_options).and_return(client_stub) - expect(client.count_repos_by('owned', 'user_id')).to be_nil + context 'when value is not cached' do + it 'returns repository count' do + expect(Gitlab::GithubImport::Client) + .to receive(:new).with(access_token).and_return(client_stub) + expect(client_stub) + .to receive(:count_repos_by_relation_type_graphql) + .with({ relation_type: 'owned' }).and_return(client_response) + expect(Gitlab::Cache::Import::Caching) + .to receive(:write) + .with('github-importer/provider-repo-count/owned/user_id', 1, timeout: 5.minutes) + .and_call_original + expect(client.count_repos_by('owned', 'user_id')).to eq(1) end end end diff --git a/spec/serializers/project_import_entity_spec.rb b/spec/serializers/project_import_entity_spec.rb index 521d0127dbb1d1..a2f895219beff5 100644 --- a/spec/serializers/project_import_entity_spec.rb +++ b/spec/serializers/project_import_entity_spec.rb @@ -39,16 +39,6 @@ it 'includes relation_type' do expect(subject[:relation_type]).to eq('owned') end - - context 'with remove_legacy_github_client FF is disabled' do - before do - stub_feature_flags(remove_legacy_github_client: false) - end - - it "doesn't include relation_type" do - expect(subject[:relation_type]).to eq(nil) - end - end end context 'when import is failed' do diff --git a/spec/services/import/github_service_spec.rb b/spec/services/import/github_service_spec.rb index 99cc5ad9874075..39832ee4b13d58 100644 --- a/spec/services/import/github_service_spec.rb +++ b/spec/services/import/github_service_spec.rb @@ -26,99 +26,131 @@ } end + let(:client) { Gitlab::GithubImport::Client.new(token) } + let(:project_double) { instance_double(Project, persisted?: true) } + subject(:github_importer) { described_class.new(client, user, params) } - shared_examples 'handles errors' do |klass| - let(:client) { klass.new(token) } - let(:project_double) { instance_double(Project, persisted?: true) } + before do + allow(Gitlab::GithubImport::Settings).to receive(:new).with(project_double).and_return(settings) + allow(settings) + .to receive(:write) + .with( + optional_stages: optional_stages, + additional_access_tokens: access_params[:additional_access_tokens], + timeout_strategy: timeout_strategy + ) + end + + context 'do not raise an exception on input error' do + let(:exception) { Octokit::ClientError.new(status: 404, body: 'Not Found') } before do - allow(Gitlab::GithubImport::Settings).to receive(:new).with(project_double).and_return(settings) - allow(settings) - .to receive(:write) - .with( - optional_stages: optional_stages, - additional_access_tokens: access_params[:additional_access_tokens], - timeout_strategy: timeout_strategy - ) + expect(client).to receive(:repository).and_raise(exception) end - context 'do not raise an exception on input error' do - let(:exception) { Octokit::ClientError.new(status: 404, body: 'Not Found') } + it 'logs the original error' do + expect(Gitlab::Import::Logger).to receive(:error).with({ + message: 'Import failed due to a GitHub error', + status: 404, + error: 'Not Found' + }).and_call_original - before do - expect(client).to receive(:repository).and_raise(exception) - end + subject.execute(access_params, :github) + end - it 'logs the original error' do - expect(Gitlab::Import::Logger).to receive(:error).with({ - message: 'Import failed due to a GitHub error', - status: 404, - error: 'Not Found' - }).and_call_original + it 'returns an error with message and code' do + result = subject.execute(access_params, :github) - subject.execute(access_params, :github) - end + expect(result).to include( + message: 'Import failed due to a GitHub error: Not Found (HTTP 404)', + status: :error, + http_status: :unprocessable_entity + ) + end + end - it 'returns an error with message and code' do - result = subject.execute(access_params, :github) + it 'raises an exception for unknown error causes' do + exception = StandardError.new('Not Implemented') - expect(result).to include( - message: 'Import failed due to a GitHub error: Not Found (HTTP 404)', - status: :error, - http_status: :unprocessable_entity - ) - end - end + expect(client).to receive(:repository).and_raise(exception) - it 'raises an exception for unknown error causes' do - exception = StandardError.new('Not Implemented') + expect(Gitlab::Import::Logger).not_to receive(:error) - expect(client).to receive(:repository).and_raise(exception) + expect { subject.execute(access_params, :github) }.to raise_error(exception) + end + + context 'repository size validation' do + let(:repository_double) { { name: 'repository', size: 99 } } + + before do + allow(subject).to receive(:authorized?).and_return(true) + expect(client).to receive(:repository).and_return(repository_double) - expect(Gitlab::Import::Logger).not_to receive(:error) + allow_next_instance_of(Gitlab::LegacyGithubImport::ProjectCreator) do |creator| + allow(creator).to receive(:execute).and_return(project_double) + end + end - expect { subject.execute(access_params, :github) }.to raise_error(exception) + context 'when there is no repository size limit defined' do + it 'skips the check, succeeds, and tracks an access level' do + expect(subject.execute(access_params, :github)).to include(status: :success) + expect(settings) + .to have_received(:write) + .with(optional_stages: nil, + additional_access_tokens: access_params[:additional_access_tokens], + timeout_strategy: timeout_strategy + ) + expect_snowplow_event( + category: 'Import::GithubService', + action: 'create', + label: 'import_access_level', + user: user, + extra: { import_type: 'github', user_role: 'Owner' } + ) + end end - context 'repository size validation' do - let(:repository_double) { { name: 'repository', size: 99 } } + context 'when the target namespace repository size limit is defined' do + let_it_be(:group) { create(:group, repository_size_limit: 100) } before do - allow(subject).to receive(:authorized?).and_return(true) - expect(client).to receive(:repository).and_return(repository_double) - - allow_next_instance_of(Gitlab::LegacyGithubImport::ProjectCreator) do |creator| - allow(creator).to receive(:execute).and_return(project_double) - end + params[:target_namespace] = group.full_path end - context 'when there is no repository size limit defined' do - it 'skips the check, succeeds, and tracks an access level' do - expect(subject.execute(access_params, :github)).to include(status: :success) - expect(settings) - .to have_received(:write) - .with(optional_stages: nil, - additional_access_tokens: access_params[:additional_access_tokens], - timeout_strategy: timeout_strategy - ) - expect_snowplow_event( - category: 'Import::GithubService', - action: 'create', - label: 'import_access_level', - user: user, - extra: { import_type: 'github', user_role: 'Owner' } + it 'succeeds when the repository is smaller than the limit' do + expect(subject.execute(access_params, :github)).to include(status: :success) + expect(settings) + .to have_received(:write) + .with( + optional_stages: nil, + additional_access_tokens: access_params[:additional_access_tokens], + timeout_strategy: timeout_strategy ) - end + expect_snowplow_event( + category: 'Import::GithubService', + action: 'create', + label: 'import_access_level', + user: user, + extra: { import_type: 'github', user_role: 'Not a member' } + ) end - context 'when the target namespace repository size limit is defined' do - let_it_be(:group) { create(:group, repository_size_limit: 100) } + it 'returns error when the repository is larger than the limit' do + repository_double[:size] = 101 - before do - params[:target_namespace] = group.full_path - end + expect(subject.execute(access_params, :github)).to include(size_limit_error) + end + end + context 'when target namespace repository limit is not defined' do + let_it_be(:group) { create(:group) } + + before do + stub_application_setting(repository_size_limit: 100) + end + + context 'when application size limit is defined' do it 'succeeds when the repository is smaller than the limit' do expect(subject.execute(access_params, :github)).to include(status: :success) expect(settings) @@ -133,7 +165,7 @@ action: 'create', label: 'import_access_level', user: user, - extra: { import_type: 'github', user_role: 'Not a member' } + extra: { import_type: 'github', user_role: 'Owner' } ) end @@ -143,192 +175,142 @@ expect(subject.execute(access_params, :github)).to include(size_limit_error) end end + end - context 'when target namespace repository limit is not defined' do - let_it_be(:group) { create(:group) } - - before do - stub_application_setting(repository_size_limit: 100) - end - - context 'when application size limit is defined' do - it 'succeeds when the repository is smaller than the limit' do - expect(subject.execute(access_params, :github)).to include(status: :success) - expect(settings) - .to have_received(:write) - .with( - optional_stages: nil, - additional_access_tokens: access_params[:additional_access_tokens], - timeout_strategy: timeout_strategy - ) - expect_snowplow_event( - category: 'Import::GithubService', - action: 'create', - label: 'import_access_level', - user: user, - extra: { import_type: 'github', user_role: 'Owner' } - ) - end - - it 'returns error when the repository is larger than the limit' do - repository_double[:size] = 101 - - expect(subject.execute(access_params, :github)).to include(size_limit_error) - end - end + context 'when optional stages params present' do + let(:optional_stages) do + { + single_endpoint_issue_events_import: true, + single_endpoint_notes_import: 'false', + attachments_import: false, + collaborators_import: true + } end - context 'when optional stages params present' do - let(:optional_stages) do - { - single_endpoint_issue_events_import: true, - single_endpoint_notes_import: 'false', - attachments_import: false, - collaborators_import: true - } - end - - it 'saves optional stages choice to import_data' do - subject.execute(access_params, :github) + it 'saves optional stages choice to import_data' do + subject.execute(access_params, :github) - expect(settings) - .to have_received(:write) - .with( - optional_stages: optional_stages, - additional_access_tokens: access_params[:additional_access_tokens], - timeout_strategy: timeout_strategy - ) - end + expect(settings) + .to have_received(:write) + .with( + optional_stages: optional_stages, + additional_access_tokens: access_params[:additional_access_tokens], + timeout_strategy: timeout_strategy + ) end + end - context 'when timeout strategy param is present' do - let(:timeout_strategy) { 'pessimistic' } - - it 'saves timeout strategy to import_data' do - subject.execute(access_params, :github) + context 'when timeout strategy param is present' do + let(:timeout_strategy) { 'pessimistic' } - expect(settings) - .to have_received(:write) - .with( - optional_stages: optional_stages, - additional_access_tokens: access_params[:additional_access_tokens], - timeout_strategy: timeout_strategy - ) - end - end - - context 'when additional access tokens are present' do - it 'saves additional access tokens to import_data' do - subject.execute(access_params, :github) + it 'saves timeout strategy to import_data' do + subject.execute(access_params, :github) - expect(settings) - .to have_received(:write) - .with( - optional_stages: optional_stages, - additional_access_tokens: %w[foo bar], - timeout_strategy: timeout_strategy - ) - end + expect(settings) + .to have_received(:write) + .with( + optional_stages: optional_stages, + additional_access_tokens: access_params[:additional_access_tokens], + timeout_strategy: timeout_strategy + ) end end - context 'when import source is disabled' do - let(:repository_double) do - { - name: 'vim', - description: 'test', - full_name: 'test/vim', - clone_url: 'http://repo.com/repo/repo.git', - private: false, - has_wiki?: false - } - end - - before do - stub_application_setting(import_sources: nil) - allow(client).to receive(:repository).and_return(repository_double) - end - - it 'returns forbidden' do - result = subject.execute(access_params, :github) + context 'when additional access tokens are present' do + it 'saves additional access tokens to import_data' do + subject.execute(access_params, :github) - expect(result).to include( - status: :error, - http_status: :forbidden - ) + expect(settings) + .to have_received(:write) + .with( + optional_stages: optional_stages, + additional_access_tokens: %w[foo bar], + timeout_strategy: timeout_strategy + ) end end + end - context 'when a blocked/local URL is used as github_hostname' do - let(:message) { 'Error while attempting to import from GitHub' } - let(:error) { "Invalid URL: #{url}" } - - before do - stub_application_setting(allow_local_requests_from_web_hooks_and_services: false) - end + context 'when import source is disabled' do + let(:repository_double) do + { + name: 'vim', + description: 'test', + full_name: 'test/vim', + clone_url: 'http://repo.com/repo/repo.git', + private: false, + has_wiki?: false + } + end - where(url: %w[https://localhost https://10.0.0.1]) + before do + stub_application_setting(import_sources: nil) + allow(client).to receive(:repository).and_return(repository_double) + end - with_them do - it 'returns and logs an error' do - allow(github_importer).to receive(:url).and_return(url) + it 'returns forbidden' do + result = subject.execute(access_params, :github) - expect(Gitlab::Import::Logger).to receive(:error).with({ - message: message, - error: error - }).and_call_original - expect(github_importer.execute(access_params, :github)).to include(blocked_url_error(url)) - end - end + expect(result).to include( + status: :error, + http_status: :forbidden + ) end + end - context 'when target_namespace is blank' do - before do - params[:target_namespace] = '' - end + context 'when a blocked/local URL is used as github_hostname' do + let(:message) { 'Error while attempting to import from GitHub' } + let(:error) { "Invalid URL: #{url}" } - it 'raises an exception' do - expect { subject.execute(access_params, :github) }.to raise_error(ArgumentError, 'Target namespace is required') - end + before do + stub_application_setting(allow_local_requests_from_web_hooks_and_services: false) end - context 'when namespace to import repository into does not exist' do - before do - params[:target_namespace] = 'unknown_path' - end + where(url: %w[https://localhost https://10.0.0.1]) - it 'returns an error' do - expect(github_importer.execute(access_params, :github)).to include(not_existed_namespace_error) + with_them do + it 'returns and logs an error' do + allow(github_importer).to receive(:url).and_return(url) + + expect(Gitlab::Import::Logger).to receive(:error).with({ + message: message, + error: error + }).and_call_original + expect(github_importer.execute(access_params, :github)).to include(blocked_url_error(url)) end end + end - context 'when user has no permissions to import repository into the specified namespace' do - let_it_be(:group) { create(:group) } - - before do - params[:target_namespace] = group.full_path - end + context 'when target_namespace is blank' do + before do + params[:target_namespace] = '' + end - it 'returns an error' do - expect(github_importer.execute(access_params, :github)).to include(taken_namespace_error) - end + it 'raises an exception' do + expect { subject.execute(access_params, :github) }.to raise_error(ArgumentError, 'Target namespace is required') end end - context 'when remove_legacy_github_client feature flag is enabled' do + context 'when namespace to import repository into does not exist' do before do - stub_feature_flags(remove_legacy_github_client: true) + params[:target_namespace] = 'unknown_path' end - include_examples 'handles errors', Gitlab::GithubImport::Client + it 'returns an error' do + expect(github_importer.execute(access_params, :github)).to include(not_existed_namespace_error) + end end - context 'when remove_legacy_github_client feature flag is disabled' do + context 'when user has no permissions to import repository into the specified namespace' do + let_it_be(:group) { create(:group) } + before do - stub_feature_flags(remove_legacy_github_client: false) + params[:target_namespace] = group.full_path end - include_examples 'handles errors', Gitlab::LegacyGithubImport::Client + it 'returns an error' do + expect(github_importer.execute(access_params, :github)).to include(taken_namespace_error) + end end def size_limit_error diff --git a/spec/support/shared_examples/controllers/githubish_import_controller_shared_examples.rb b/spec/support/shared_examples/controllers/githubish_import_controller_shared_examples.rb index af1843bae2857e..c921da103479d0 100644 --- a/spec/support/shared_examples/controllers/githubish_import_controller_shared_examples.rb +++ b/spec/support/shared_examples/controllers/githubish_import_controller_shared_examples.rb @@ -161,8 +161,6 @@ def assign_session_token(provider) group.add_owner(user) client = stub_client(repos: repos, orgs: [org], org_repos: [org_repo]) allow(client).to receive(:each_page).and_return([double('client', objects: repos)].to_enum) - # GitHub controller has filtering done using GitHub Search API - stub_feature_flags(remove_legacy_github_client: false) end it 'filters list of repositories by name' do -- GitLab From 7d4d44bc8f10522eb467d38c94987126d95ace02 Mon Sep 17 00:00:00 2001 From: James Nutt Date: Fri, 29 Sep 2023 09:48:50 +0100 Subject: [PATCH 2/2] Remove unused method --- lib/api/import_github.rb | 4 ---- 1 file changed, 4 deletions(-) diff --git a/lib/api/import_github.rb b/lib/api/import_github.rb index af34489d0f6356..f403875314389f 100644 --- a/lib/api/import_github.rb +++ b/lib/api/import_github.rb @@ -22,10 +22,6 @@ def access_params } end - def client_options - { host: params[:github_hostname] } - end - def provider :github end -- GitLab