Commit e2f1f359 authored by Rémy Coutable's avatar Rémy Coutable

Merge branch '25095-remove-import-and-fork-repository-from-gitlab-shell' into 'master'

Remove repository fork and import code from Gitlab::Shell

See merge request gitlab-org/gitlab!27217
parents 419133fc 7304a594
......@@ -2,8 +2,6 @@
module Projects
class ImportService < BaseService
include Gitlab::ShellAdapter
Error = Class.new(StandardError)
# Returns true if this importer is supposed to perform its work in the
......@@ -72,9 +70,9 @@ module Projects
project.ensure_repository
project.repository.fetch_as_mirror(project.import_url, refmap: refmap)
else
gitlab_shell.import_project_repository(project)
project.repository.import_repository(project.import_url)
end
rescue Gitlab::Shell::Error => e
rescue ::Gitlab::Git::CommandError => e
# Expire cache to prevent scenarios such as:
# 1. First import failed, but the repo was imported successfully, so +exists?+ returns true
# 2. Retried import, repo is broken or not imported but +exists?+ still returns true
......
......@@ -2,7 +2,6 @@
class RepositoryForkWorker # rubocop:disable Scalability/IdempotentWorker
include ApplicationWorker
include Gitlab::ShellAdapter
include ProjectStartImport
include ProjectImportOptions
......@@ -27,18 +26,8 @@ class RepositoryForkWorker # rubocop:disable Scalability/IdempotentWorker
Gitlab::Metrics.add_event(:fork_repository)
result = gitlab_shell.fork_repository(source_project, target_project)
if result
gitaly_fork!(source_project, target_project)
link_lfs_objects(source_project, target_project)
else
raise_fork_failure(
source_project,
target_project,
'Failed to create fork repository'
)
end
target_project.after_import
end
......@@ -49,6 +38,17 @@ class RepositoryForkWorker # rubocop:disable Scalability/IdempotentWorker
false
end
def gitaly_fork!(source_project, target_project)
source_repo = source_project.repository.raw
target_repo = target_project.repository.raw
::Gitlab::GitalyClient::RepositoryService.new(target_repo).fork_repository(source_repo)
rescue GRPC::BadStatus => e
Gitlab::ErrorTracking.track_exception(e, source_project_id: source_project.id, target_project_id: target_project.id)
raise_fork_failure(source_project, target_project, 'Failed to create fork repository')
end
def link_lfs_objects(source_project, target_project)
Projects::LfsPointers::LfsLinkService
.new(target_project)
......
......@@ -3,8 +3,6 @@
module Gitlab
module BitbucketImport
class Importer
include Gitlab::ShellAdapter
LABELS = [{ title: 'bug', color: '#FF0000' },
{ title: 'enhancement', color: '#428BCA' },
{ title: 'proposal', color: '#69D100' },
......@@ -80,7 +78,7 @@ module Gitlab
wiki = WikiFormatter.new(project)
gitlab_shell.import_wiki_repository(project, wiki)
project.wiki.repository.import_repository(wiki.import_url)
rescue StandardError => e
errors << { type: :wiki, errors: e.message }
end
......
......@@ -793,6 +793,14 @@ module Gitlab
end
end
def import_repository(url)
raise ArgumentError, "don't use disk paths with import_repository: #{url.inspect}" if url.start_with?('.', '/')
wrapped_gitaly_errors do
gitaly_repository_client.import_repository(url)
end
end
def blob_at(sha, path)
Gitlab::Git::Blob.find(self, sha, path) unless Gitlab::Git.blank_ref?(sha)
end
......
......@@ -4,7 +4,6 @@ module Gitlab
module GithubImport
module Importer
class RepositoryImporter
include Gitlab::ShellAdapter
include Gitlab::Utils::StrongMemoize
attr_reader :project, :client, :wiki_formatter
......@@ -65,10 +64,10 @@ module Gitlab
end
def import_wiki_repository
gitlab_shell.import_wiki_repository(project, wiki_formatter)
project.wiki.repository.import_repository(wiki_formatter.import_url)
true
rescue Gitlab::Shell::Error => e
rescue ::Gitlab::Git::CommandError => e
if e.message !~ /repository not exported/
project.create_wiki
fail_import("Failed to import the wiki: #{e.message}")
......
......@@ -3,8 +3,6 @@
module Gitlab
module LegacyGithubImport
class Importer
include Gitlab::ShellAdapter
def self.refmap
Gitlab::GithubImport.refmap
end
......@@ -264,11 +262,11 @@ module Gitlab
end
def import_wiki
unless project.wiki.repository_exists?
return if project.wiki.repository_exists?
wiki = WikiFormatter.new(project)
gitlab_shell.import_wiki_repository(project, wiki)
end
rescue Gitlab::Shell::Error => e
project.wiki.repository.import_repository(wiki.import_url)
rescue ::Gitlab::Git::CommandError => e
# GitHub error message when the wiki repo has not been created,
# this means that repo has wiki enabled, but have no pages. So,
# we can skip the import.
......
# frozen_string_literal: true
# Gitaly note: SSH key operations are not part of Gitaly so will never be migrated.
require 'securerandom'
module Gitlab
# This class is an artifact of a time when common repository operations were
# performed by calling out to scripts in the gitlab-shell project. Now, these
# operations are all performed by Gitaly, and are mostly accessible through
# the Repository class. Prefer using a Repository to functionality here.
#
# Legacy code relating to namespaces still relies on Gitlab::Shell; it can be
# converted to a module once https://gitlab.com/groups/gitlab-org/-/epics/2320
# is completed. https://gitlab.com/gitlab-org/gitlab/-/issues/25095 tracks it.
class Shell
Error = Class.new(StandardError)
......@@ -77,47 +83,6 @@ module Gitlab
end
end
# Import wiki repository from external service
#
# @param [Project] project
# @param [Gitlab::LegacyGithubImport::WikiFormatter, Gitlab::BitbucketImport::WikiFormatter] wiki_formatter
# @return [Boolean] whether repository could be imported
def import_wiki_repository(project, wiki_formatter)
import_repository(project.repository_storage, wiki_formatter.disk_path, wiki_formatter.import_url, project.wiki.full_path)
end
# Import project repository from external service
#
# @param [Project] project
# @return [Boolean] whether repository could be imported
def import_project_repository(project)
import_repository(project.repository_storage, project.disk_path, project.import_url, project.full_path)
end
# Import repository
#
# @example Import a repository
# import_repository("nfs-file06", "gitlab/gitlab-ci", "https://gitlab.com/gitlab-org/gitlab-test.git", "gitlab/gitlab-ci")
#
# @param [String] storage project's storage name
# @param [String] disk_path project path on disk
# @param [String] url from external resource to import from
# @param [String] gl_project_path project name
# @return [Boolean] whether repository could be imported
def import_repository(storage, disk_path, url, gl_project_path)
if url.start_with?('.', '/')
raise Error.new("don't use disk paths with import_repository: #{url.inspect}")
end
relative_path = "#{disk_path}.git"
cmd = GitalyGitlabProjects.new(storage, relative_path, gl_project_path)
success = cmd.import_project(url, git_timeout)
raise Error, cmd.output unless success
success
end
# Move or rename a repository
#
# @example Move/rename a repository
......@@ -127,6 +92,8 @@ module Gitlab
# @param [String] disk_path current project path on disk
# @param [String] new_disk_path new project path on disk
# @return [Boolean] whether repository could be moved/renamed on disk
#
# @deprecated
def mv_repository(storage, disk_path, new_disk_path)
return false if disk_path.empty? || new_disk_path.empty?
......@@ -139,17 +106,6 @@ module Gitlab
false
end
# Fork repository to new path
#
# @param [Project] source_project forked-from Project
# @param [Project] target_project forked-to Project
def fork_repository(source_project, target_project)
forked_from_relative_path = "#{source_project.disk_path}.git"
fork_args = [target_project.repository_storage, "#{target_project.disk_path}.git", target_project.full_path]
GitalyGitlabProjects.new(source_project.repository_storage, forked_from_relative_path, source_project.full_path).fork_repository(*fork_args)
end
# Removes a repository from file system, using rm_diretory which is an alias
# for rm_namespace. Given the underlying implementation removes the name
# passed as second argument on the passed storage.
......@@ -159,6 +115,8 @@ module Gitlab
#
# @param [String] storage project's storage path
# @param [String] disk_path current project path on disk
#
# @deprecated
def remove_repository(storage, disk_path)
return false if disk_path.empty?
......@@ -179,6 +137,8 @@ module Gitlab
#
# @param [String] storage project's storage path
# @param [String] name namespace name
#
# @deprecated
def add_namespace(storage, name)
Gitlab::GitalyClient.allow_n_plus_1_calls do
Gitlab::GitalyClient::NamespaceService.new(storage).add(name)
......@@ -195,6 +155,8 @@ module Gitlab
#
# @param [String] storage project's storage path
# @param [String] name namespace name
#
# @deprecated
def rm_namespace(storage, name)
Gitlab::GitalyClient::NamespaceService.new(storage).remove(name)
rescue GRPC::InvalidArgument => e
......@@ -210,6 +172,8 @@ module Gitlab
# @param [String] storage project's storage path
# @param [String] old_name current namespace name
# @param [String] new_name new namespace name
#
# @deprecated
def mv_namespace(storage, old_name, new_name)
Gitlab::GitalyClient::NamespaceService.new(storage).rename(old_name, new_name)
rescue GRPC::InvalidArgument => e
......@@ -226,67 +190,12 @@ module Gitlab
# @return [Boolean] whether repository exists or not
# @param [String] storage project's storage path
# @param [Object] dir_name repository dir name
#
# @deprecated
def repository_exists?(storage, dir_name)
Gitlab::Git::Repository.new(storage, dir_name, nil, nil).exists?
rescue GRPC::Internal
false
end
protected
def full_path(storage, dir_name)
raise ArgumentError.new("Directory name can't be blank") if dir_name.blank?
File.join(Gitlab.config.repositories.storages[storage].legacy_disk_path, dir_name)
end
private
def git_timeout
Gitlab.config.gitlab_shell.git_timeout
end
def wrapped_gitaly_errors
yield
rescue GRPC::NotFound, GRPC::BadStatus => e
# Old Popen code returns [Error, output] to the caller, so we
# need to do the same here...
raise Error, e
end
class GitalyGitlabProjects
attr_reader :shard_name, :repository_relative_path, :output, :gl_project_path
def initialize(shard_name, repository_relative_path, gl_project_path)
@shard_name = shard_name
@repository_relative_path = repository_relative_path
@output = ''
@gl_project_path = gl_project_path
end
def import_project(source, _timeout)
raw_repository = Gitlab::Git::Repository.new(shard_name, repository_relative_path, nil, gl_project_path)
Gitlab::GitalyClient::RepositoryService.new(raw_repository).import_repository(source)
true
rescue GRPC::BadStatus => e
@output = e.message
false
end
def fork_repository(new_shard_name, new_repository_relative_path, new_project_name)
target_repository = Gitlab::Git::Repository.new(new_shard_name, new_repository_relative_path, nil, new_project_name)
raw_repository = Gitlab::Git::Repository.new(shard_name, repository_relative_path, nil, gl_project_path)
Gitlab::GitalyClient::RepositoryService.new(target_repository).fork_repository(raw_repository)
rescue GRPC::BadStatus => e
logger.error "fork-repository failed: #{e.message}"
false
end
def logger
Rails.logger # rubocop:disable Gitlab/RailsLogger
end
end
end
end
......@@ -80,8 +80,7 @@ describe Gitlab::BitbucketImport::Importer do
end
let(:importer) { described_class.new(project) }
let(:gitlab_shell) { double }
let(:sample) { RepoHelpers.sample_compare }
let(:issues_statuses_sample_data) do
{
count: sample_issues_statuses.count,
......@@ -89,12 +88,6 @@ describe Gitlab::BitbucketImport::Importer do
}
end
let(:sample) { RepoHelpers.sample_compare }
before do
allow(importer).to receive(:gitlab_shell) { gitlab_shell }
end
subject { described_class.new(project) }
describe '#import_pull_requests' do
......@@ -316,7 +309,7 @@ describe Gitlab::BitbucketImport::Importer do
describe 'wiki import' do
it 'is skipped when the wiki exists' do
expect(project.wiki).to receive(:repository_exists?) { true }
expect(importer.gitlab_shell).not_to receive(:import_wiki_repository)
expect(project.wiki.repository).not_to receive(:import_repository)
importer.execute
......@@ -325,7 +318,7 @@ describe Gitlab::BitbucketImport::Importer do
it 'imports to the project disk_path' do
expect(project.wiki).to receive(:repository_exists?) { false }
expect(importer.gitlab_shell).to receive(:import_wiki_repository)
expect(project.wiki.repository).to receive(:import_repository)
importer.execute
......
......@@ -2138,6 +2138,33 @@ describe Gitlab::Git::Repository, :seed_helper do
end
end
describe '#import_repository' do
let_it_be(:project) { create(:project) }
let(:repository) { project.repository }
let(:url) { 'http://invalid.invalid' }
it 'raises an error if a relative path is provided' do
expect { repository.import_repository('/foo') }.to raise_error(ArgumentError, /disk path/)
end
it 'raises an error if an absolute path is provided' do
expect { repository.import_repository('./foo') }.to raise_error(ArgumentError, /disk path/)
end
it 'delegates to Gitaly' do
expect_next_instance_of(Gitlab::GitalyClient::RepositoryService) do |svc|
expect(svc).to receive(:import_repository).with(url).and_return(nil)
end
repository.import_repository(url)
end
it_behaves_like 'wrapping gRPC errors', Gitlab::GitalyClient::RepositoryService, :import_repository do
subject { repository.import_repository('http://invalid.invalid') }
end
end
describe '#replicate' do
let(:new_repository) do
Gitlab::Git::Repository.new('test_second_storage', TEST_REPO_PATH, '', 'group/project')
......
......@@ -11,10 +11,15 @@ describe Gitlab::GithubImport::Importer::RepositoryImporter do
double(
:wiki,
disk_path: 'foo.wiki',
full_path: 'group/foo.wiki'
full_path: 'group/foo.wiki',
repository: wiki_repository
)
end
let(:wiki_repository) do
double(:wiki_repository)
end
let(:project) do
double(
:project,
......@@ -221,17 +226,19 @@ describe Gitlab::GithubImport::Importer::RepositoryImporter do
describe '#import_wiki_repository' do
it 'imports the wiki repository' do
expect(importer.gitlab_shell)
expect(wiki_repository)
.to receive(:import_repository)
.with('foo', 'foo.wiki', 'foo.wiki.git', 'group/foo.wiki')
.with(importer.wiki_url)
.and_return(true)
expect(importer.import_wiki_repository).to eq(true)
end
it 'marks the import as failed and creates an empty repo if an error was raised' do
expect(importer.gitlab_shell)
expect(wiki_repository)
.to receive(:import_repository)
.and_raise(Gitlab::Shell::Error)
.with(importer.wiki_url)
.and_raise(Gitlab::Git::CommandError)
expect(importer)
.to receive(:fail_import)
......
......@@ -45,7 +45,7 @@ describe Gitlab::LegacyGithubImport::Importer do
allow(Rails).to receive(:cache).and_return(ActiveSupport::Cache::MemoryStore.new)
allow_any_instance_of(Octokit::Client).to receive(:rate_limit!).and_raise(Octokit::NotFound)
allow_any_instance_of(Gitlab::Shell).to receive(:import_repository).and_raise(Gitlab::Shell::Error)
allow(project.wiki.repository).to receive(:import_repository).and_raise(Gitlab::Git::CommandError)
allow_any_instance_of(Octokit::Client).to receive(:user).and_return(octocat)
allow_any_instance_of(Octokit::Client).to receive(:labels).and_return([label1, label2])
......@@ -169,7 +169,7 @@ describe Gitlab::LegacyGithubImport::Importer do
errors: [
{ type: :label, url: "#{api_root}/repos/octocat/Hello-World/labels/bug", errors: "Validation failed: Title can't be blank, Title is invalid" },
{ type: :issue, url: "#{api_root}/repos/octocat/Hello-World/issues/1348", errors: "Validation failed: Title can't be blank" },
{ type: :wiki, errors: "Gitlab::Shell::Error" }
{ type: :wiki, errors: "Gitlab::Git::CommandError" }
]
}
......
......@@ -9,7 +9,6 @@ describe Gitlab::Shell do
let(:gitlab_shell) { described_class.new }
it { is_expected.to respond_to :remove_repository }
it { is_expected.to respond_to :fork_repository }
describe '.url_to_repo' do
let(:full_path) { 'diaspora/disaspora-rails' }
......@@ -95,52 +94,6 @@ describe Gitlab::Shell do
expect(TestEnv.storage_dir_exists?(project2.repository_storage, "#{project2.disk_path}.git")).to be(true)
end
end
describe '#fork_repository' do
let(:target_project) { create(:project) }
subject do
gitlab_shell.fork_repository(project, target_project)
end
it 'returns true when the command succeeds' do
expect_any_instance_of(Gitlab::GitalyClient::RepositoryService).to receive(:fork_repository)
.with(repository.raw_repository) { :gitaly_response_object }
is_expected.to be_truthy
end
it 'return false when the command fails' do
expect_any_instance_of(Gitlab::GitalyClient::RepositoryService).to receive(:fork_repository)
.with(repository.raw_repository) { raise GRPC::BadStatus, 'bla' }
is_expected.to be_falsy
end
end
describe '#import_repository' do
let(:import_url) { 'https://gitlab.com/gitlab-org/gitlab-foss.git' }
context 'with gitaly' do
it 'returns true when the command succeeds' do
expect_any_instance_of(Gitlab::GitalyClient::RepositoryService).to receive(:import_repository).with(import_url)
result = gitlab_shell.import_repository(project.repository_storage, project.disk_path, import_url, project.full_path)
expect(result).to be_truthy
end
it 'raises an exception when the command fails' do
expect_any_instance_of(Gitlab::GitalyClient::RepositoryService).to receive(:import_repository)
.with(import_url) { raise GRPC::BadStatus, 'bla' }
expect_any_instance_of(Gitlab::Shell::GitalyGitlabProjects).to receive(:output) { 'error'}
expect do
gitlab_shell.import_repository(project.repository_storage, project.disk_path, import_url, project.full_path)
end.to raise_error(Gitlab::Shell::Error, "error")
end
end
end
end
describe 'namespace actions' do
......
......@@ -122,7 +122,7 @@ describe Projects::ImportService do
end
it 'succeeds if repository import is successful' do
expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).and_return(true)
expect(project.repository).to receive(:import_repository).and_return(true)
expect_any_instance_of(Gitlab::BitbucketImport::Importer).to receive(:execute).and_return(true)
expect_any_instance_of(Projects::LfsPointers::LfsImportService).to receive(:execute).and_return(status: :success)
......@@ -132,7 +132,9 @@ describe Projects::ImportService do
end
it 'fails if repository import fails' do
expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).and_raise(Gitlab::Shell::Error.new('Failed to import the repository /a/b/c'))
expect(project.repository)
.to receive(:import_repository)
.and_raise(Gitlab::Git::CommandError, 'Failed to import the repository /a/b/c')
result = subject.execute
......@@ -144,7 +146,7 @@ describe Projects::ImportService do
it 'logs the error' do
error_message = 'error message'
expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).and_return(true)
expect(project.repository).to receive(:import_repository).and_return(true)
expect_any_instance_of(Gitlab::BitbucketImport::Importer).to receive(:execute).and_return(true)
expect_any_instance_of(Projects::LfsPointers::LfsImportService).to receive(:execute).and_return(status: :error, message: error_message)
expect(Gitlab::AppLogger).to receive(:error).with("The Lfs import process failed. #{error_message}")
......@@ -155,7 +157,7 @@ describe Projects::ImportService do
context 'when repository import scheduled' do
before do
allow_any_instance_of(Gitlab::Shell).to receive(:import_repository).and_return(true)
expect(project.repository).to receive(:import_repository).and_return(true)
allow(subject).to receive(:import_data)
end
......
......@@ -13,7 +13,6 @@ describe RepositoryForkWorker do
describe "#perform" do
let(:project) { create(:project, :public, :repository) }
let(:shell) { Gitlab::Shell.new }
let(:forked_project) { create(:project, :repository, :import_scheduled) }
before do
......@@ -21,12 +20,17 @@ describe RepositoryForkWorker do
end
shared_examples 'RepositoryForkWorker performing' do
before do
allow(subject).to receive(:gitlab_shell).and_return(shell)
end
def expect_fork_repository(success:)
allow(::Gitlab::GitalyClient::RepositoryService).to receive(:new).and_call_original
expect_next_instance_of(::Gitlab::GitalyClient::RepositoryService, forked_project.repository.raw) do |svc|
exp = expect(svc).to receive(:fork_repository).with(project.repository.raw)
def expect_fork_repository
expect(shell).to receive(:fork_repository).with(project, forked_project)
if success
exp.and_return(true)
else
exp.and_raise(GRPC::BadStatus, 'Fork failed in tests')
end
end
end
describe 'when a worker was reset without cleanup' do
......@@ -35,20 +39,20 @@ describe RepositoryForkWorker do
it 'creates a new repository from a fork' do
allow(subject).to receive(:jid).and_return(jid)
expect_fork_repository.and_return(true)
expect_fork_repository(success: true)
perform!
end
end
it "creates a new repository from a fork" do
expect_fork_repository.and_return(true)
expect_fork_repository(success: true)
perform!
end
it 'protects the default branch' do
expect_fork_repository.and_return(true)
expect_fork_repository(success: true)
perform!
......@@ -56,7 +60,7 @@ describe RepositoryForkWorker do
end
it 'flushes various caches' do
expect_fork_repository.and_return(true)
expect_fork_repository(success: true)
# Works around https://github.com/rspec/rspec-mocks/issues/910
expect(Project).to receive(:find).with(forked_project.id).and_return(forked_project)
......@@ -75,13 +79,13 @@ describe RepositoryForkWorker do
it 'handles bad fork' do
error_message = "Unable to fork project #{forked_project.id} for repository #{project.disk_path} -> #{forked_project.disk_path}: Failed to create fork repository"
expect_fork_repository.and_return(false)
expect_fork_repository(success: false)
expect { perform! }.to raise_error(StandardError, error_message)
end
it 'calls Projects::LfsPointers::LfsLinkService#execute with OIDs of source project LFS objects' do
expect_fork_repository.and_return(true)
expect_fork_repository(success: true)
expect_next_instance_of(Projects::LfsPointers::LfsLinkService) do |service|
expect(service).to receive(:execute).with(project.all_lfs_objects_oids)
end
......@@ -92,7 +96,7 @@ describe RepositoryForkWorker do
it "handles LFS objects link failure" do
error_message = "Unable to fork project #{forked_project.id} for repository #{project.disk_path} -> #{forked_project.disk_path}: Source project has too many LFS objects"
expect_fork_repository.and_return(true)
expect_fork_repository(success: true)
expect_next_instance_of(Projects::LfsPointers::LfsLinkService) do |service|
expect(service).to receive(:execute).and_raise(Projects::LfsPointers::LfsLinkService::TooManyOidsError)
end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment