From 01de2b5df89c4eaca92408c18203050604a4e94f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= Date: Thu, 26 Jul 2018 21:18:28 -0400 Subject: [PATCH] Refactor gitlab:import:repos task to remove direct disk access --- lib/gitlab/bare_repository_import/importer.rb | 36 +++++++++---- .../bare_repository_import/repository.rb | 2 - lib/gitlab/git/repository.rb | 13 ----- .../bare_repository_import/importer_spec.rb | 54 +++++++++---------- .../git/attributes_at_ref_parser_spec.rb | 2 +- spec/lib/gitlab/git/attributes_parser_spec.rb | 2 +- spec/lib/gitlab/git/blame_spec.rb | 2 +- spec/lib/gitlab/git/blob_snippet_spec.rb | 2 +- spec/lib/gitlab/git/blob_spec.rb | 2 +- spec/lib/gitlab/git/branch_spec.rb | 2 +- spec/lib/gitlab/git/commit_spec.rb | 2 +- .../gitlab/git/committer_with_hooks_spec.rb | 2 +- spec/lib/gitlab/git/compare_spec.rb | 2 +- spec/lib/gitlab/git/diff_collection_spec.rb | 2 +- spec/lib/gitlab/git/diff_spec.rb | 2 +- spec/lib/gitlab/git/hooks_service_spec.rb | 2 +- spec/lib/gitlab/git/index_spec.rb | 2 +- spec/lib/gitlab/git/remote_repository_spec.rb | 2 +- spec/lib/gitlab/git/repository_spec.rb | 2 +- spec/lib/gitlab/git/tag_spec.rb | 2 +- spec/lib/gitlab/git/tree_spec.rb | 2 +- spec/support/helpers/seed_helper.rb | 6 --- spec/support/helpers/test_env.rb | 8 +++ spec/support/stored_repositories.rb | 4 -- 24 files changed, 77 insertions(+), 80 deletions(-) diff --git a/lib/gitlab/bare_repository_import/importer.rb b/lib/gitlab/bare_repository_import/importer.rb index 4ca5a78e068..04aa6aab771 100644 --- a/lib/gitlab/bare_repository_import/importer.rb +++ b/lib/gitlab/bare_repository_import/importer.rb @@ -26,6 +26,12 @@ module Gitlab end end + # This is called from within a rake task only used by Admins, so allow writing + # to STDOUT + def self.log(message) + puts message # rubocop:disable Rails/Output + end + attr_reader :user, :project_name, :bare_repo delegate :log, to: :class @@ -59,11 +65,10 @@ module Gitlab import_type: 'bare_repository', namespace_id: group&.id).execute - if project.persisted? && mv_repo(project) + if project.persisted? && mv_repositories(project) log " * Created #{project.name} (#{project_full_path})".color(:green) project.write_repository_config - Gitlab::Git::Repository.create_hooks(project.repository.path_to_repo, Gitlab.config.gitlab_shell.hooks_path) ProjectCacheWorker.perform_async(project.id) else @@ -74,12 +79,11 @@ module Gitlab project end - def mv_repo(project) - storage_path = storage_path_for_shard(project.repository_storage) - FileUtils.mv(repo_path, project.repository.path_to_repo) + def mv_repositories(project) + mv_repo(bare_repo.repo_path, project.repository) if bare_repo.wiki_exists? - FileUtils.mv(wiki_path, File.join(storage_path, project.disk_path + '.wiki.git')) + mv_repo(bare_repo.wiki_path, project.wiki.repository) end true @@ -89,6 +93,11 @@ module Gitlab false end + def mv_repo(path, repository) + repository.create_from_bundle(bundle(path)) + FileUtils.rm_rf(path) + end + def storage_path_for_shard(shard) Gitlab.config.repositories.storages[shard].legacy_disk_path end @@ -101,10 +110,17 @@ module Gitlab Groups::NestedCreateService.new(user, group_path: group_path).execute end - # This is called from within a rake task only used by Admins, so allow writing - # to STDOUT - def self.log(message) - puts message # rubocop:disable Rails/Output + def bundle(repo_path) + # TODO: we could save some time and disk space by using + # `git bundle create - --all` and streaming the bundle directly to + # Gitaly, rather than writing it on disk first + bundle_path = "#{repo_path}.bundle" + cmd = %W(#{Gitlab.config.git.bin_path} --git-dir=#{repo_path} bundle create #{bundle_path} --all) + output, status = Gitlab::Popen.popen(cmd) + + raise output unless status.zero? + + bundle_path end end end diff --git a/lib/gitlab/bare_repository_import/repository.rb b/lib/gitlab/bare_repository_import/repository.rb index fe267248275..c0c666dfb7b 100644 --- a/lib/gitlab/bare_repository_import/repository.rb +++ b/lib/gitlab/bare_repository_import/repository.rb @@ -1,5 +1,3 @@ -# Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/953 -# module Gitlab module BareRepositoryImport class Repository diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index eb02c7ac8e1..73151e4a4c5 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -39,19 +39,6 @@ module Gitlab ChecksumError = Class.new(StandardError) class << self - # Unlike `new`, `create` takes the repository path - def create(repo_path, bare: true, symlink_hooks_to: nil) - FileUtils.mkdir_p(repo_path, mode: 0770) - - # Equivalent to `git --git-path=#{repo_path} init [--bare]` - repo = Rugged::Repository.init_at(repo_path, bare) - repo.close - - create_hooks(repo_path, symlink_hooks_to) if symlink_hooks_to.present? - - true - end - def create_hooks(repo_path, global_hooks_path) local_hooks_path = File.join(repo_path, 'hooks') real_local_hooks_path = :not_found diff --git a/spec/lib/gitlab/bare_repository_import/importer_spec.rb b/spec/lib/gitlab/bare_repository_import/importer_spec.rb index 468f6ff6d24..6e21c846c0a 100644 --- a/spec/lib/gitlab/bare_repository_import/importer_spec.rb +++ b/spec/lib/gitlab/bare_repository_import/importer_spec.rb @@ -1,10 +1,11 @@ require 'spec_helper' -describe Gitlab::BareRepositoryImport::Importer, repository: true do +describe Gitlab::BareRepositoryImport::Importer, :seed_helper do let!(:admin) { create(:admin) } let!(:base_dir) { Dir.mktmpdir + '/' } let(:bare_repository) { Gitlab::BareRepositoryImport::Repository.new(base_dir, File.join(base_dir, "#{project_path}.git")) } let(:gitlab_shell) { Gitlab::Shell.new } + let(:source_project) { TEST_REPO_PATH } subject(:importer) { described_class.new(admin, bare_repository) } @@ -17,16 +18,11 @@ describe Gitlab::BareRepositoryImport::Importer, repository: true do after do FileUtils.rm_rf(base_dir) + TestEnv.clean_test_path + ensure_seeds Rainbow.enabled = @rainbow end - around do |example| - # TODO migrate BareRepositoryImport https://gitlab.com/gitlab-org/gitaly/issues/953 - Gitlab::GitalyClient::StorageSettings.allow_disk_access do - example.run - end - end - shared_examples 'importing a repository' do describe '.execute' do it 'creates a project for a repository in storage' do @@ -86,8 +82,8 @@ describe Gitlab::BareRepositoryImport::Importer, repository: true do importer.create_project_if_needed end - it 'creates the Git repo on disk with the proper symlink for hooks' do - create_bare_repository("#{project_path}.git") + it 'creates the Git repo on disk' do + prepare_repository("#{project_path}.git", source_project) importer.create_project_if_needed @@ -97,9 +93,6 @@ describe Gitlab::BareRepositoryImport::Importer, repository: true do expect(gitlab_shell.exists?(project.repository_storage, repo_path)).to be(true) expect(gitlab_shell.exists?(project.repository_storage, hook_path)).to be(true) - - full_hook_path = File.join(project.repository.path_to_repo, 'hooks') - expect(File.readlink(full_hook_path)).to eq(Gitlab.config.gitlab_shell.hooks_path) end context 'hashed storage enabled' do @@ -148,7 +141,7 @@ describe Gitlab::BareRepositoryImport::Importer, repository: true do end it 'creates the Git repo in disk' do - create_bare_repository("#{project_path}.git") + prepare_repository("#{project_path}.git", source_project) importer.create_project_if_needed @@ -158,23 +151,23 @@ describe Gitlab::BareRepositoryImport::Importer, repository: true do expect(gitlab_shell.exists?(project.repository_storage, project.disk_path + '.wiki.git')).to be(true) end - it 'moves an existing project to the correct path' do + context 'with a repository already on disk' do + let!(:base_dir) { TestEnv.repos_path } # This is a quick way to get a valid repository instead of copying an # existing one. Since it's not persisted, the importer will try to # create the project. - project = build(:project, :legacy_storage, :repository) - original_commit_count = project.repository.commit_count + let(:project) { build(:project, :legacy_storage, :repository) } + let(:project_path) { project.full_path } - legacy_path = Gitlab.config.repositories.storages[project.repository_storage].legacy_disk_path + it 'moves an existing project to the correct path' do + original_commit_count = project.repository.commit_count - bare_repo = Gitlab::BareRepositoryImport::Repository.new(legacy_path, project.repository.path) - gitlab_importer = described_class.new(admin, bare_repo) + expect(importer).to receive(:create_project).and_call_original - expect(gitlab_importer).to receive(:create_project).and_call_original + new_project = importer.create_project_if_needed - new_project = gitlab_importer.create_project_if_needed - - expect(new_project.repository.commit_count).to eq(original_commit_count) + expect(new_project.repository.commit_count).to eq(original_commit_count) + end end end @@ -185,8 +178,8 @@ describe Gitlab::BareRepositoryImport::Importer, repository: true do it_behaves_like 'importing a repository' it 'creates the Wiki git repo in disk' do - create_bare_repository("#{project_path}.git") - create_bare_repository("#{project_path}.wiki.git") + prepare_repository("#{project_path}.git", source_project) + prepare_repository("#{project_path}.wiki.git", source_project) expect(Projects::CreateService).to receive(:new).with(admin, hash_including(skip_wiki: true, import_type: 'bare_repository')).and_call_original @@ -213,8 +206,13 @@ describe Gitlab::BareRepositoryImport::Importer, repository: true do end end - def create_bare_repository(project_path) + def prepare_repository(project_path, source_project) repo_path = File.join(base_dir, project_path) - Gitlab::Git::Repository.create(repo_path, bare: true) + + return create_bare_repository(repo_path) unless source_project + + cmd = %W(#{Gitlab.config.git.bin_path} clone --bare #{source_project} #{repo_path}) + + system(git_env, *cmd, chdir: SEED_STORAGE_PATH, out: '/dev/null', err: '/dev/null') end end diff --git a/spec/lib/gitlab/git/attributes_at_ref_parser_spec.rb b/spec/lib/gitlab/git/attributes_at_ref_parser_spec.rb index 5d22dcfb508..ca067a29174 100644 --- a/spec/lib/gitlab/git/attributes_at_ref_parser_spec.rb +++ b/spec/lib/gitlab/git/attributes_at_ref_parser_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Gitlab::Git::AttributesAtRefParser, seed_helper: true do +describe Gitlab::Git::AttributesAtRefParser, :seed_helper do let(:project) { create(:project, :repository) } let(:repository) { project.repository } diff --git a/spec/lib/gitlab/git/attributes_parser_spec.rb b/spec/lib/gitlab/git/attributes_parser_spec.rb index 2d103123998..18ebfef38f0 100644 --- a/spec/lib/gitlab/git/attributes_parser_spec.rb +++ b/spec/lib/gitlab/git/attributes_parser_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Gitlab::Git::AttributesParser, seed_helper: true do +describe Gitlab::Git::AttributesParser, :seed_helper do let(:attributes_path) { File.join(SEED_STORAGE_PATH, 'with-git-attributes.git', 'info', 'attributes') } let(:data) { File.read(attributes_path) } diff --git a/spec/lib/gitlab/git/blame_spec.rb b/spec/lib/gitlab/git/blame_spec.rb index ba790b717ae..e704d1c673c 100644 --- a/spec/lib/gitlab/git/blame_spec.rb +++ b/spec/lib/gitlab/git/blame_spec.rb @@ -1,7 +1,7 @@ # coding: utf-8 require "spec_helper" -describe Gitlab::Git::Blame, seed_helper: true do +describe Gitlab::Git::Blame, :seed_helper do let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '') } let(:blame) do Gitlab::Git::Blame.new(repository, SeedRepo::Commit::ID, "CONTRIBUTING.md") diff --git a/spec/lib/gitlab/git/blob_snippet_spec.rb b/spec/lib/gitlab/git/blob_snippet_spec.rb index d6d365f6492..6effec8295c 100644 --- a/spec/lib/gitlab/git/blob_snippet_spec.rb +++ b/spec/lib/gitlab/git/blob_snippet_spec.rb @@ -2,7 +2,7 @@ require "spec_helper" -describe Gitlab::Git::BlobSnippet, seed_helper: true do +describe Gitlab::Git::BlobSnippet, :seed_helper do describe '#data' do context 'empty lines' do let(:snippet) { Gitlab::Git::BlobSnippet.new('master', nil, nil, nil) } diff --git a/spec/lib/gitlab/git/blob_spec.rb b/spec/lib/gitlab/git/blob_spec.rb index 034b89a46fa..ea49502ae2e 100644 --- a/spec/lib/gitlab/git/blob_spec.rb +++ b/spec/lib/gitlab/git/blob_spec.rb @@ -2,7 +2,7 @@ require "spec_helper" -describe Gitlab::Git::Blob, seed_helper: true do +describe Gitlab::Git::Blob, :seed_helper do let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '') } describe 'initialize' do diff --git a/spec/lib/gitlab/git/branch_spec.rb b/spec/lib/gitlab/git/branch_spec.rb index a8c5627e678..79ccbb79966 100644 --- a/spec/lib/gitlab/git/branch_spec.rb +++ b/spec/lib/gitlab/git/branch_spec.rb @@ -1,6 +1,6 @@ require "spec_helper" -describe Gitlab::Git::Branch, seed_helper: true do +describe Gitlab::Git::Branch, :seed_helper do let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '') } let(:rugged) do Gitlab::GitalyClient::StorageSettings.allow_disk_access do diff --git a/spec/lib/gitlab/git/commit_spec.rb b/spec/lib/gitlab/git/commit_spec.rb index 0adb684765d..2718a3c5e49 100644 --- a/spec/lib/gitlab/git/commit_spec.rb +++ b/spec/lib/gitlab/git/commit_spec.rb @@ -1,6 +1,6 @@ require "spec_helper" -describe Gitlab::Git::Commit, seed_helper: true do +describe Gitlab::Git::Commit, :seed_helper do let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '') } let(:commit) { described_class.find(repository, SeedRepo::Commit::ID) } let(:rugged_commit) do diff --git a/spec/lib/gitlab/git/committer_with_hooks_spec.rb b/spec/lib/gitlab/git/committer_with_hooks_spec.rb index 2100690f873..c7626058acd 100644 --- a/spec/lib/gitlab/git/committer_with_hooks_spec.rb +++ b/spec/lib/gitlab/git/committer_with_hooks_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Gitlab::Git::CommitterWithHooks, seed_helper: true do +describe Gitlab::Git::CommitterWithHooks, :seed_helper do # TODO https://gitlab.com/gitlab-org/gitaly/issues/1234 skip 'needs to be moved to gitaly-ruby test suite' do shared_examples 'calling wiki hooks' do diff --git a/spec/lib/gitlab/git/compare_spec.rb b/spec/lib/gitlab/git/compare_spec.rb index b6a42e422b5..7cc6f52f8ee 100644 --- a/spec/lib/gitlab/git/compare_spec.rb +++ b/spec/lib/gitlab/git/compare_spec.rb @@ -1,6 +1,6 @@ require "spec_helper" -describe Gitlab::Git::Compare, seed_helper: true do +describe Gitlab::Git::Compare, :seed_helper do let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '') } let(:compare) { Gitlab::Git::Compare.new(repository, SeedRepo::BigCommit::ID, SeedRepo::Commit::ID, straight: false) } let(:compare_straight) { Gitlab::Git::Compare.new(repository, SeedRepo::BigCommit::ID, SeedRepo::Commit::ID, straight: true) } diff --git a/spec/lib/gitlab/git/diff_collection_spec.rb b/spec/lib/gitlab/git/diff_collection_spec.rb index 65edc750f39..81658874be7 100644 --- a/spec/lib/gitlab/git/diff_collection_spec.rb +++ b/spec/lib/gitlab/git/diff_collection_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Gitlab::Git::DiffCollection, seed_helper: true do +describe Gitlab::Git::DiffCollection, :seed_helper do subject do Gitlab::Git::DiffCollection.new( iterator, diff --git a/spec/lib/gitlab/git/diff_spec.rb b/spec/lib/gitlab/git/diff_spec.rb index 11ab376ab8f..87d9fcee39e 100644 --- a/spec/lib/gitlab/git/diff_spec.rb +++ b/spec/lib/gitlab/git/diff_spec.rb @@ -1,6 +1,6 @@ require "spec_helper" -describe Gitlab::Git::Diff, seed_helper: true do +describe Gitlab::Git::Diff, :seed_helper do let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '') } before do diff --git a/spec/lib/gitlab/git/hooks_service_spec.rb b/spec/lib/gitlab/git/hooks_service_spec.rb index 9337aa39e13..55ffced36ac 100644 --- a/spec/lib/gitlab/git/hooks_service_spec.rb +++ b/spec/lib/gitlab/git/hooks_service_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Gitlab::Git::HooksService, seed_helper: true do +describe Gitlab::Git::HooksService, :seed_helper do let(:gl_id) { 'user-456' } let(:gl_username) { 'janedoe' } let(:user) { Gitlab::Git::User.new(gl_username, 'Jane Doe', 'janedoe@example.com', gl_id) } diff --git a/spec/lib/gitlab/git/index_spec.rb b/spec/lib/gitlab/git/index_spec.rb index e51b875be11..c4edd6961e1 100644 --- a/spec/lib/gitlab/git/index_spec.rb +++ b/spec/lib/gitlab/git/index_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Gitlab::Git::Index, seed_helper: true do +describe Gitlab::Git::Index, :seed_helper do let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '') } let(:index) { described_class.new(repository) } diff --git a/spec/lib/gitlab/git/remote_repository_spec.rb b/spec/lib/gitlab/git/remote_repository_spec.rb index eb148cc3804..53ed7c5a13a 100644 --- a/spec/lib/gitlab/git/remote_repository_spec.rb +++ b/spec/lib/gitlab/git/remote_repository_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Gitlab::Git::RemoteRepository, seed_helper: true do +describe Gitlab::Git::RemoteRepository, :seed_helper do let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '') } subject { described_class.new(repository) } diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index 62396af1ebe..35a6fc94753 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -1,7 +1,7 @@ # coding: utf-8 require "spec_helper" -describe Gitlab::Git::Repository, seed_helper: true do +describe Gitlab::Git::Repository, :seed_helper do include Gitlab::EncodingHelper using RSpec::Parameterized::TableSyntax diff --git a/spec/lib/gitlab/git/tag_spec.rb b/spec/lib/gitlab/git/tag_spec.rb index be2f5bfb819..2d9db576a6c 100644 --- a/spec/lib/gitlab/git/tag_spec.rb +++ b/spec/lib/gitlab/git/tag_spec.rb @@ -1,6 +1,6 @@ require "spec_helper" -describe Gitlab::Git::Tag, seed_helper: true do +describe Gitlab::Git::Tag, :seed_helper do let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '') } shared_examples 'Gitlab::Git::Repository#tags' do diff --git a/spec/lib/gitlab/git/tree_spec.rb b/spec/lib/gitlab/git/tree_spec.rb index 001e406a930..3792d6bf67b 100644 --- a/spec/lib/gitlab/git/tree_spec.rb +++ b/spec/lib/gitlab/git/tree_spec.rb @@ -1,6 +1,6 @@ require "spec_helper" -describe Gitlab::Git::Tree, seed_helper: true do +describe Gitlab::Git::Tree, :seed_helper do let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '') } context :repo do diff --git a/spec/support/helpers/seed_helper.rb b/spec/support/helpers/seed_helper.rb index 8fd107260cc..25781f5e679 100644 --- a/spec/support/helpers/seed_helper.rb +++ b/spec/support/helpers/seed_helper.rb @@ -101,10 +101,4 @@ bla/bla.txt handle.write('# hello'.encode(enc)) end end - - # Prevent developer git configurations from being persisted to test - # repositories - def git_env - { 'GIT_TEMPLATE_DIR' => '' } - end end diff --git a/spec/support/helpers/test_env.rb b/spec/support/helpers/test_env.rb index e531495d917..8e1d4cfe269 100644 --- a/spec/support/helpers/test_env.rb +++ b/spec/support/helpers/test_env.rb @@ -243,6 +243,14 @@ module TestEnv set_repo_refs(target_repo_path, refs) end + def create_bare_repository(path) + FileUtils.mkdir_p(path) + + system(git_env, *%W(#{Gitlab.config.git.bin_path} -C #{path} init --bare), + out: '/dev/null', + err: '/dev/null') + end + def repos_path @repos_path ||= Gitlab.config.repositories.storages[REPOS_STORAGE].legacy_disk_path end diff --git a/spec/support/stored_repositories.rb b/spec/support/stored_repositories.rb index 21995c89a6e..26f823cb6ef 100644 --- a/spec/support/stored_repositories.rb +++ b/spec/support/stored_repositories.rb @@ -1,8 +1,4 @@ RSpec.configure do |config| - config.before(:each, :repository) do - TestEnv.clean_test_path - end - config.before(:all, :broken_storage) do FileUtils.rm_rf Gitlab.config.repositories.storages.broken.legacy_disk_path end