diff options
author | Rémy Coutable <remy@rymai.me> | 2018-02-05 15:16:36 +0300 |
---|---|---|
committer | Rémy Coutable <remy@rymai.me> | 2018-02-05 15:16:36 +0300 |
commit | 1e66ff6d8003abeab261c90fc94d8fcb4d93083d (patch) | |
tree | 35a70f9dbbb4715311881c14710d2f3cbdb2f398 | |
parent | 4ff15fcf8fdafb068c20ca99697e56a145debaaf (diff) | |
parent | 30a43b7e04b98616f379bdd800532c3354df8b19 (diff) |
Merge branch '42270-fix-namespace-remove-exports-for-hashed-storage' into 'master'
Fix export removal for hashed-storage projects within a renamed or deleted namespace
Closes #42270
See merge request gitlab-org/gitlab-ce!16658
-rw-r--r-- | app/models/concerns/storage/legacy_namespace.rb | 16 | ||||
-rw-r--r-- | app/models/group.rb | 6 | ||||
-rw-r--r-- | app/models/namespace.rb | 18 | ||||
-rw-r--r-- | app/models/project.rb | 8 | ||||
-rw-r--r-- | changelogs/unreleased/42270-fix-namespace-remove-exports-for-hashed-storage.yml | 6 | ||||
-rw-r--r-- | spec/factories/projects.rb | 6 | ||||
-rw-r--r-- | spec/features/projects/import_export/namespace_export_file_spec.rb | 55 | ||||
-rw-r--r-- | spec/models/namespace_spec.rb | 20 | ||||
-rw-r--r-- | spec/models/project_spec.rb | 31 |
9 files changed, 120 insertions, 46 deletions
diff --git a/app/models/concerns/storage/legacy_namespace.rb b/app/models/concerns/storage/legacy_namespace.rb index 99dbd4fbacf..b12c10a84de 100644 --- a/app/models/concerns/storage/legacy_namespace.rb +++ b/app/models/concerns/storage/legacy_namespace.rb @@ -87,20 +87,10 @@ module Storage remove_exports! end - def remove_exports! - Gitlab::Popen.popen(%W(find #{export_path} -not -path #{export_path} -delete)) - end - - def export_path - File.join(Gitlab::ImportExport.storage_path, full_path_was) - end + def remove_legacy_exports! + legacy_export_path = File.join(Gitlab::ImportExport.storage_path, full_path_was) - def full_path_was - if parent - parent.full_path + '/' + path_was - else - path_was - end + FileUtils.rm_rf(legacy_export_path) end end end diff --git a/app/models/group.rb b/app/models/group.rb index 62b1322ebe6..5b7f1b38612 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -274,12 +274,6 @@ class Group < Namespace list_of_ids.reverse.map { |group| variables[group.id] }.compact.flatten end - def full_path_was - return path_was unless has_parent? - - "#{parent.full_path}/#{path_was}" - end - def group_member(user) if group_members.loaded? group_members.find { |gm| gm.user_id == user.id } diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 5010dd73c11..7b82d076975 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -221,6 +221,24 @@ class Namespace < ActiveRecord::Base has_parent? end + def full_path_was + return path_was unless has_parent? + + "#{parent.full_path}/#{path_was}" + end + + # Exports belonging to projects with legacy storage are placed in a common + # subdirectory of the namespace, so a simple `rm -rf` is sufficient to remove + # them. + # + # Exports of projects using hashed storage are placed in a location defined + # only by the project ID, so each must be removed individually. + def remove_exports! + remove_legacy_exports! + + all_projects.with_storage_feature(:repository).find_each(&:remove_exports) + end + private def refresh_access_of_projects_invited_groups diff --git a/app/models/project.rb b/app/models/project.rb index 03c5475c31f..12d5f28f5ea 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -69,6 +69,7 @@ class Project < ActiveRecord::Base before_destroy :remove_private_deploy_keys after_destroy -> { run_after_commit { remove_pages } } + after_destroy :remove_exports after_validation :check_pending_delete @@ -1525,6 +1526,8 @@ class Project < ActiveRecord::Base end def export_path + return nil unless namespace.present? || hashed_storage?(:repository) + File.join(Gitlab::ImportExport.storage_path, disk_path) end @@ -1533,8 +1536,9 @@ class Project < ActiveRecord::Base end def remove_exports - _, status = Gitlab::Popen.popen(%W(find #{export_path} -not -path #{export_path} -delete)) - status.zero? + return nil unless export_path.present? + + FileUtils.rm_rf(export_path) end def full_path_slug diff --git a/changelogs/unreleased/42270-fix-namespace-remove-exports-for-hashed-storage.yml b/changelogs/unreleased/42270-fix-namespace-remove-exports-for-hashed-storage.yml new file mode 100644 index 00000000000..d7a8b6e6f81 --- /dev/null +++ b/changelogs/unreleased/42270-fix-namespace-remove-exports-for-hashed-storage.yml @@ -0,0 +1,6 @@ +--- +title: Fix export removal for hashed-storage projects within a renamed or deleted + namespace +merge_request: 16658 +author: +type: fixed diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb index 16d328a5bc2..20976977f21 100644 --- a/spec/factories/projects.rb +++ b/spec/factories/projects.rb @@ -93,6 +93,12 @@ FactoryBot.define do avatar { fixture_file_upload('spec/fixtures/dk.png') } end + trait :with_export do + after(:create) do |project, evaluator| + ProjectExportWorker.new.perform(project.creator.id, project.id) + end + end + trait :broken_storage do after(:create) do |project| project.update_column(:repository_storage, 'broken') diff --git a/spec/features/projects/import_export/namespace_export_file_spec.rb b/spec/features/projects/import_export/namespace_export_file_spec.rb index e76bc6f1220..c1fccf4a40b 100644 --- a/spec/features/projects/import_export/namespace_export_file_spec.rb +++ b/spec/features/projects/import_export/namespace_export_file_spec.rb @@ -1,44 +1,37 @@ require 'spec_helper' feature 'Import/Export - Namespace export file cleanup', :js do - let(:export_path) { "#{Dir.tmpdir}/import_file_spec" } - let(:config_hash) { YAML.load_file(Gitlab::ImportExport.config_file).deep_stringify_keys } + let(:export_path) { Dir.mktmpdir('namespace_export_file_spec') } - let(:project) { create(:project) } - - background do - allow_any_instance_of(Gitlab::ImportExport).to receive(:storage_path).and_return(export_path) + before do + allow(Gitlab::ImportExport).to receive(:storage_path).and_return(export_path) end after do FileUtils.rm_rf(export_path, secure: true) end - context 'admin user' do + shared_examples_for 'handling project exports on namespace change' do + let!(:old_export_path) { project.export_path } + before do sign_in(create(:admin)) + + setup_export_project end context 'moving the namespace' do - scenario 'removes the export file' do - setup_export_project - - old_export_path = project.export_path.dup - + it 'removes the export file' do expect(File).to exist(old_export_path) - project.namespace.update(path: 'new_path') + project.namespace.update!(path: build(:namespace).path) expect(File).not_to exist(old_export_path) end end context 'deleting the namespace' do - scenario 'removes the export file' do - setup_export_project - - old_export_path = project.export_path.dup - + it 'removes the export file' do expect(File).to exist(old_export_path) project.namespace.destroy @@ -46,17 +39,29 @@ feature 'Import/Export - Namespace export file cleanup', :js do expect(File).not_to exist(old_export_path) end end + end - def setup_export_project - visit edit_project_path(project) + describe 'legacy storage' do + let(:project) { create(:project) } - expect(page).to have_content('Export project') + it_behaves_like 'handling project exports on namespace change' + end + + describe 'hashed storage' do + let(:project) { create(:project, :hashed) } - find(:link, 'Export project').send_keys(:return) + it_behaves_like 'handling project exports on namespace change' + end - visit edit_project_path(project) + def setup_export_project + visit edit_project_path(project) - expect(page).to have_content('Download export') - end + expect(page).to have_content('Export project') + + find(:link, 'Export project').send_keys(:return) + + visit edit_project_path(project) + + expect(page).to have_content('Download export') end end diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb index 6b7dbad128c..824cca66fb4 100644 --- a/spec/models/namespace_spec.rb +++ b/spec/models/namespace_spec.rb @@ -596,4 +596,24 @@ describe Namespace do end end end + + describe '#remove_exports' do + let(:legacy_project) { create(:project, :with_export, namespace: namespace) } + let(:hashed_project) { create(:project, :with_export, :hashed, namespace: namespace) } + let(:export_path) { Dir.mktmpdir('namespace_remove_exports_spec') } + let(:legacy_export) { legacy_project.export_project_path } + let(:hashed_export) { hashed_project.export_project_path } + + it 'removes exports for legacy and hashed projects' do + allow(Gitlab::ImportExport).to receive(:storage_path) { export_path } + + expect(File.exist?(legacy_export)).to be_truthy + expect(File.exist?(hashed_export)).to be_truthy + + namespace.remove_exports! + + expect(File.exist?(legacy_export)).to be_falsy + expect(File.exist?(hashed_export)).to be_falsy + end + end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index df4c796c61e..da940571bc1 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -2503,6 +2503,37 @@ describe Project do end end + describe '#remove_exports' do + let(:project) { create(:project, :with_export) } + + it 'removes the exports directory for the project' do + expect(File.exist?(project.export_path)).to be_truthy + + allow(FileUtils).to receive(:rm_rf).and_call_original + expect(FileUtils).to receive(:rm_rf).with(project.export_path).and_call_original + project.remove_exports + + expect(File.exist?(project.export_path)).to be_falsy + end + + it 'is a no-op when there is no namespace' do + export_path = project.export_path + project.update_column(:namespace_id, nil) + + expect(FileUtils).not_to receive(:rm_rf).with(export_path) + + project.remove_exports + + expect(File.exist?(export_path)).to be_truthy + end + + it 'is run when the project is destroyed' do + expect(project).to receive(:remove_exports).and_call_original + + project.destroy + end + end + describe '#forks_count' do it 'returns the number of forks' do project = build(:project) |