Refactor Storage Migration

Specs were reviewed and improved to better cover the current behavior.
There was some standardization done as well to facilitate the
implementation of the rollback functionality.

StorageMigratorWorker was extracted to HashedStorage namespace were
RollbackerWorker will live one as well.

2 files changed, 38 insertions, 18 deletions

diff --git a/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb b/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb
index 28d8a95fe07..bef12df583e 100644
--- a/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb
+++ b/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb
@@ -3,7 +3,7 @@ require 'spec_helper'
 describe Projects::HashedStorage::MigrateAttachmentsService do
   subject(:service) { described_class.new(project, project.full_path, logger: nil) }
 
-  let(:project) { create(:project, :legacy_storage) }
+  let(:project) { create(:project, :repository, storage_version: 1, skip_disk_validation: true) }
   let(:legacy_storage) { Storage::LegacyProject.new(project) }
   let(:hashed_storage) { Storage::HashedProject.new(project) }
 
@@ -28,6 +28,10 @@ describe Projects::HashedStorage::MigrateAttachmentsService do
         expect(File.file?(old_disk_path)).to be_falsey
         expect(File.file?(new_disk_path)).to be_truthy
       end
+
+      it 'returns true' do
+        expect(service.execute).to be_truthy
+      end
     end
 
     context 'when original folder does not exist anymore' do
@@ -43,6 +47,12 @@ describe Projects::HashedStorage::MigrateAttachmentsService do
         expect(File.exist?(base_path(hashed_storage))).to be_falsey
         expect(File.file?(new_disk_path)).to be_falsey
       end
+
+      it 'returns true' do
+        expect(FileUtils).not_to receive(:mv).with(base_path(legacy_storage), base_path(hashed_storage))
+
+        expect(service.execute).to be_truthy
+      end
     end
 
     context 'when target folder already exists' do
@@ -58,6 +68,18 @@ describe Projects::HashedStorage::MigrateAttachmentsService do
     end
   end
 
+  context '#old_disk_path' do
+    it 'returns old disk_path for project' do
+      expect(service.old_disk_path).to eq(project.full_path)
+    end
+  end
+
+  context '#new_disk_path' do
+    it 'returns new disk_path for project' do
+      expect(service.new_disk_path).to eq(project.disk_path)
+    end
+  end
+
   def base_path(storage)
     File.join(FileUploader.root, storage.disk_path)
   end
diff --git a/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb b/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb
index b720f37ffdb..0772dc4b85b 100644
--- a/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb
+++ b/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb
@@ -8,9 +8,12 @@ describe Projects::HashedStorage::MigrateRepositoryService do
   let(:legacy_storage) { Storage::LegacyProject.new(project) }
   let(:hashed_storage) { Storage::HashedProject.new(project) }
 
-  subject(:service) { described_class.new(project, project.full_path) }
+  subject(:service) { described_class.new(project, project.disk_path) }
 
   describe '#execute' do
+    let(:old_disk_path) { legacy_storage.disk_path }
+    let(:new_disk_path) { hashed_storage.disk_path }
+
     before do
       allow(service).to receive(:gitlab_shell) { gitlab_shell }
     end
@@ -33,8 +36,8 @@ describe Projects::HashedStorage::MigrateRepositoryService do
       it 'renames project and wiki repositories' do
         service.execute
 
-        expect(gitlab_shell.exists?(project.repository_storage, "#{hashed_storage.disk_path}.git")).to be_truthy
-        expect(gitlab_shell.exists?(project.repository_storage, "#{hashed_storage.disk_path}.wiki.git")).to be_truthy
+        expect(gitlab_shell.exists?(project.repository_storage, "#{new_disk_path}.git")).to be_truthy
+        expect(gitlab_shell.exists?(project.repository_storage, "#{new_disk_path}.wiki.git")).to be_truthy
       end
 
       it 'updates project to be hashed and not read-only' do
@@ -45,8 +48,8 @@ describe Projects::HashedStorage::MigrateRepositoryService do
       end
 
       it 'move operation is called for both repositories' do
-        expect_move_repository(project.disk_path, hashed_storage.disk_path)
-        expect_move_repository("#{project.disk_path}.wiki", "#{hashed_storage.disk_path}.wiki")
+        expect_move_repository(old_disk_path, new_disk_path)
+        expect_move_repository("#{old_disk_path}.wiki", "#{new_disk_path}.wiki")
 
         service.execute
       end
@@ -62,32 +65,27 @@ describe Projects::HashedStorage::MigrateRepositoryService do
 
     context 'when one move fails' do
       it 'rollsback repositories to original name' do
-        from_name = project.disk_path
-        to_name = hashed_storage.disk_path
         allow(service).to receive(:move_repository).and_call_original
-        allow(service).to receive(:move_repository).with(from_name, to_name).once { false } # will disable first move only
+        allow(service).to receive(:move_repository).with(old_disk_path, new_disk_path).once { false } # will disable first move only
 
         expect(service).to receive(:rollback_folder_move).and_call_original
 
         service.execute
 
-        expect(gitlab_shell.exists?(project.repository_storage, "#{hashed_storage.disk_path}.git")).to be_falsey
-        expect(gitlab_shell.exists?(project.repository_storage, "#{hashed_storage.disk_path}.wiki.git")).to be_falsey
+        expect(gitlab_shell.exists?(project.repository_storage, "#{new_disk_path}.git")).to be_falsey
+        expect(gitlab_shell.exists?(project.repository_storage, "#{new_disk_path}.wiki.git")).to be_falsey
         expect(project.repository_read_only?).to be_falsey
       end
 
       context 'when rollback fails' do
-        let(:from_name) { legacy_storage.disk_path }
-        let(:to_name) { hashed_storage.disk_path }
-
         before do
           hashed_storage.ensure_storage_path_exists
-          gitlab_shell.mv_repository(project.repository_storage, from_name, to_name)
+          gitlab_shell.mv_repository(project.repository_storage, old_disk_path, new_disk_path)
         end
 
-        it 'does not try to move nil repository over hashed' do
-          expect(gitlab_shell).not_to receive(:mv_repository).with(project.repository_storage, from_name, to_name)
-          expect_move_repository("#{project.disk_path}.wiki", "#{hashed_storage.disk_path}.wiki")
+        it 'does not try to move nil repository over existing' do
+          expect(gitlab_shell).not_to receive(:mv_repository).with(project.repository_storage, old_disk_path, new_disk_path)
+          expect_move_repository("#{old_disk_path}.wiki", "#{new_disk_path}.wiki")
 
           service.execute
         end