Merge branch 'georgekoltsov/import-export-tmp-folder-cleanup' into 'master'

Clean up shared/tmp folder after Project/Group Import/Export

See merge request gitlab-org/gitlab!32326

app/services/groups/import_export/export_service.rb

@@ -22,7 +22,7 @@ module Groups
 
         save!
       ensure
-        cleanup
+        remove_base_tmp_dir
       end
 
       private
@@ -81,8 +81,8 @@ module Groups
         Gitlab::ImportExport::Saver.new(exportable: @group, shared: @shared)
       end
 
-      def cleanup
-        FileUtils.rm_rf(shared.archive_path) if shared&.archive_path
+      def remove_base_tmp_dir
+        FileUtils.rm_rf(shared.base_path) if shared&.base_path
       end
 
       def notify_error!

app/services/groups/import_export/import_service.rb

@@ -26,6 +26,7 @@ module Groups
         end
 
       ensure
+        remove_base_tmp_dir
         remove_import_file
       end
 
@@ -102,6 +103,10 @@ module Groups
 
         raise Gitlab::ImportExport::Error.new(@shared.errors.to_sentence)
       end
+
+      def remove_base_tmp_dir
+        FileUtils.rm_rf(@shared.base_path)
+      end
     end
   end
 end

changelogs/unreleased/georgekoltsov-import-export-tmp-folder-cleanup.yml

+---
+title: Clean up shared/tmp folder after Import/Export
+merge_request: 32326
+author:
+type: fixed

lib/gitlab/import_export/importer.rb

@@ -26,6 +26,7 @@ module Gitlab
       rescue => e
         raise Projects::ImportService::Error.new(e.message)
       ensure
+        remove_base_tmp_dir
         remove_import_file
       end
 
@@ -148,6 +149,10 @@ module Gitlab
           ::Project.find_by_full_path("#{project.namespace.full_path}/#{original_path}")
         end
       end
+
+      def remove_base_tmp_dir
+        FileUtils.rm_rf(@shared.base_path)
+      end
     end
   end
 end

lib/gitlab/import_export/saver.rb

@@ -16,8 +16,6 @@ module Gitlab
 
       def save
         if compress_and_save
-          remove_export_path
-
           Gitlab::Export::Logger.info(
             message: 'Export archive saved',
             exportable_class: @exportable.class.to_s,
@@ -33,8 +31,7 @@ module Gitlab
         @shared.error(e)
         false
       ensure
-        remove_archive
-        remove_export_path
+        remove_base_tmp_dir
       end
 
       private
@@ -43,12 +40,8 @@ module Gitlab
         tar_czf(archive: archive_file, dir: @shared.export_path)
       end
 
-      def remove_export_path
-        FileUtils.rm_rf(@shared.export_path)
-      end
-
-      def remove_archive
-        FileUtils.rm_rf(@shared.archive_path)
+      def remove_base_tmp_dir
+        FileUtils.rm_rf(@shared.base_path)
       end
 
       def archive_file

spec/lib/gitlab/import_export/importer_spec.rb

@@ -18,6 +18,7 @@ describe Gitlab::ImportExport::Importer do
 
     FileUtils.mkdir_p(shared.export_path)
     ImportExportUpload.create(project: project, import_file: import_file)
+    allow(FileUtils).to receive(:rm_rf).and_call_original
   end
 
   after do
@@ -78,6 +79,13 @@ describe Gitlab::ImportExport::Importer do
         expect(project.import_export_upload.import_file&.file).to be_nil
       end
 
+      it 'removes tmp files' do
+        importer.execute
+
+        expect(FileUtils).to have_received(:rm_rf).with(shared.base_path)
+        expect(Dir.exist?(shared.base_path)).to eq(false)
+      end
+
       it 'sets the correct visibility_level when visibility level is a string' do
         project.create_or_update_import_data(
           data: { override_params: { visibility_level: Gitlab::VisibilityLevel::PRIVATE.to_s } }

spec/lib/gitlab/import_export/saver_spec.rb

@@ -5,18 +5,21 @@ require 'fileutils'
 
 describe Gitlab::ImportExport::Saver do
   let!(:project) { create(:project, :public, name: 'project') }
-  let(:export_path) { "#{Dir.tmpdir}/project_tree_saver_spec" }
+  let(:base_path) { "#{Dir.tmpdir}/project_tree_saver_spec" }
+  let(:export_path) { "#{base_path}/project_tree_saver_spec/export" }
   let(:shared) { project.import_export_shared }
 
   subject { described_class.new(exportable: project, shared: shared) }
 
   before do
+    allow(shared).to receive(:base_path).and_return(base_path)
     allow_next_instance_of(Gitlab::ImportExport) do |instance|
       allow(instance).to receive(:storage_path).and_return(export_path)
     end
 
     FileUtils.mkdir_p(shared.export_path)
     FileUtils.touch("#{shared.export_path}/tmp.bundle")
+    allow(FileUtils).to receive(:rm_rf).and_call_original
   end
 
   after do
@@ -31,4 +34,11 @@ describe Gitlab::ImportExport::Saver do
     expect(ImportExportUpload.find_by(project: project).export_file.url)
       .to match(%r[\/uploads\/-\/system\/import_export_upload\/export_file.*])
   end
+
+  it 'removes tmp files' do
+    subject.save
+
+    expect(FileUtils).to have_received(:rm_rf).with(base_path)
+    expect(Dir.exist?(base_path)).to eq(false)
+  end
 end

spec/services/groups/import_export/export_service_spec.rb

@@ -134,7 +134,7 @@ describe Groups::ImportExport::ExportService do
           expect { service.execute }.to raise_error(Gitlab::ImportExport::Error)
 
           expect(group.import_export_upload).to be_nil
-          expect(File.exist?(shared.archive_path)).to eq(false)
+          expect(Dir.exist?(shared.base_path)).to eq(false)
         end
 
         it 'notifies the user about failed group export' do
@@ -159,7 +159,7 @@ describe Groups::ImportExport::ExportService do
           expect { service.execute }.to raise_error(Gitlab::ImportExport::Error)
 
           expect(group.import_export_upload).to be_nil
-          expect(File.exist?(shared.archive_path)).to eq(false)
+          expect(Dir.exist?(shared.base_path)).to eq(false)
         end
 
         it 'notifies logger' do

spec/services/groups/import_export/import_service_spec.rb

@@ -91,6 +91,7 @@ describe Groups::ImportExport::ImportService do
         allow(Gitlab::Import::Logger).to receive(:build).and_return(import_logger)
         allow(import_logger).to receive(:error)
         allow(import_logger).to receive(:info)
+        allow(FileUtils).to receive(:rm_rf).and_call_original
       end
 
       context 'when user has correct permissions' do
@@ -104,6 +105,16 @@ describe Groups::ImportExport::ImportService do
           expect(group.import_export_upload.import_file.file).to be_nil
         end
 
+        it 'removes tmp files' do
+          shared = Gitlab::ImportExport::Shared.new(group)
+          allow(Gitlab::ImportExport::Shared).to receive(:new).and_return(shared)
+
+          subject
+
+          expect(FileUtils).to have_received(:rm_rf).with(shared.base_path)
+          expect(Dir.exist?(shared.base_path)).to eq(false)
+        end
+
         it 'logs the import success' do
           expect(import_logger).to receive(:info).with(
             group_id:   group.id,
@@ -191,6 +202,7 @@ describe Groups::ImportExport::ImportService do
         allow(Gitlab::Import::Logger).to receive(:build).and_return(import_logger)
         allow(import_logger).to receive(:error)
         allow(import_logger).to receive(:info)
+        allow(FileUtils).to receive(:rm_rf).and_call_original
       end
 
       context 'when user has correct permissions' do
@@ -204,6 +216,16 @@ describe Groups::ImportExport::ImportService do
           expect(group.import_export_upload.import_file.file).to be_nil
         end
 
+        it 'removes tmp files' do
+          shared = Gitlab::ImportExport::Shared.new(group)
+          allow(Gitlab::ImportExport::Shared).to receive(:new).and_return(shared)
+
+          subject
+
+          expect(FileUtils).to have_received(:rm_rf).with(shared.base_path)
+          expect(Dir.exist?(shared.base_path)).to eq(false)
+        end
+
         it 'logs the import success' do
           expect(import_logger).to receive(:info).with(
             group_id:   group.id,