Generalize artifact store when deletingmc/bug/optimize-project-deletion-jobartifacts

Pass the file store when deleting artifacts to make the implementation
store agnostic.

6 files changed, 49 insertions, 18 deletions

diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb
index fd33dee682f..5adc7501e37 100644
--- a/app/models/ci/job_artifact.rb
+++ b/app/models/ci/job_artifact.rb
@@ -152,7 +152,7 @@ module Ci
 
       def begin_fast_destroy
         preload(:project).find_each.map do |artifact|
-          [artifact.project_id, artifact.store_path, artifact.local_store?, artifact.size]
+          [artifact.project_id, artifact.store_path, artifact.file_store, artifact.size]
         end
       end
 
@@ -161,7 +161,7 @@ module Ci
           Ci::DeleteStoredArtifactsWorker.perform_async(
             artifact_info[0], # project_id
             artifact_info[1], # store_path
-            artifact_info[2], # local_store
+            artifact_info[2], # file_store
             artifact_info[3]  # size
           )
         end
diff --git a/app/services/ci/delete_stored_artifacts_service.rb b/app/services/ci/delete_stored_artifacts_service.rb
index f389754efcd..132e7268a47 100644
--- a/app/services/ci/delete_stored_artifacts_service.rb
+++ b/app/services/ci/delete_stored_artifacts_service.rb
@@ -2,8 +2,17 @@
 
 module Ci
   class DeleteStoredArtifactsService < ::BaseService
-    def execute(store_path, local_store)
-      local_store ? delete_local_file(store_path) : delete_remote_file(store_path)
+    InvalidStoreError = Class.new(StandardError)
+
+    def execute(store_path, file_store)
+      case file_store
+      when nil, ObjectStorage::Store::LOCAL
+        delete_local_file(store_path)
+      when ObjectStorage::Store::REMOTE
+        delete_remote_file(store_path)
+      else
+        raise InvalidStoreError
+      end
     end
 
     def delete_local_file(file_path)
diff --git a/app/workers/ci/delete_stored_artifacts_worker.rb b/app/workers/ci/delete_stored_artifacts_worker.rb
index 636d958b143..9f6d72cd376 100644
--- a/app/workers/ci/delete_stored_artifacts_worker.rb
+++ b/app/workers/ci/delete_stored_artifacts_worker.rb
@@ -4,9 +4,9 @@ module Ci
   class DeleteStoredArtifactsWorker
     include ApplicationWorker
 
-    def perform(project_id, store_path, local_store, size)
+    def perform(project_id, store_path, file_store, size)
       Project.find_by_id(project_id).try do |project|
-        Ci::DeleteStoredArtifactsService.new(project).execute(store_path, local_store)
+        Ci::DeleteStoredArtifactsService.new(project).execute(store_path, file_store)
 
         UpdateProjectStatistics.update_project_statistics!(project, :build_artifacts_size, -size)
       end
diff --git a/spec/models/ci/job_artifact_spec.rb b/spec/models/ci/job_artifact_spec.rb
index 845fa5b9eb8..177b1984daa 100644
--- a/spec/models/ci/job_artifact_spec.rb
+++ b/spec/models/ci/job_artifact_spec.rb
@@ -122,7 +122,7 @@ describe Ci::JobArtifact do
 
     let(:artifact_list) do
       described_class.all.map do |artifact|
-        [artifact.project_id, artifact.store_path, artifact.local_store?, artifact.size]
+        [artifact.project_id, artifact.store_path, artifact.file_store, artifact.size]
       end
     end
 
@@ -136,8 +136,8 @@ describe Ci::JobArtifact do
     subject { described_class.finalize_fast_destroy(artifact_list) }
 
     it 'calls the async deletion worker' do
-      expect(Ci::DeleteStoredArtifactsWorker).to receive(:perform_async).with(project.id, instance_of(String), true, instance_of(Integer)).exactly(2).times
-      expect(Ci::DeleteStoredArtifactsWorker).to receive(:perform_async).with(project.id, instance_of(String), false, instance_of(Integer)).exactly(2).times
+      expect(Ci::DeleteStoredArtifactsWorker).to receive(:perform_async).with(project.id, instance_of(String), nil, instance_of(Integer)).exactly(2).times
+      expect(Ci::DeleteStoredArtifactsWorker).to receive(:perform_async).with(project.id, instance_of(String), ObjectStorage::Store::REMOTE, instance_of(Integer)).exactly(2).times
 
       subject
     end
diff --git a/spec/services/ci/delete_stored_artifacts_service_spec.rb b/spec/services/ci/delete_stored_artifacts_service_spec.rb
index 625f86d6074..666547754ce 100644
--- a/spec/services/ci/delete_stored_artifacts_service_spec.rb
+++ b/spec/services/ci/delete_stored_artifacts_service_spec.rb
@@ -7,27 +7,40 @@ describe Ci::DeleteStoredArtifactsService do
     let(:project) { create(:project) }
     let(:service) { described_class.new(project) }
 
-    subject { service.execute(artifact_store_path, local) }
+    subject { service.execute(artifact_store_path, file_store) }
 
     context 'with a local artifact' do
       let(:artifact_store_path) { 'local_file_path' }
-      let(:local) { true }
       let(:full_path) { File.join(Gitlab.config.artifacts['storage_path'], 'local_file_path') }
 
       before do
         allow(File).to receive(:exist?).with(full_path).and_return(true)
       end
 
-      it 'deletes the local artifact' do
-        expect(File).to receive(:delete).with(full_path)
+      context 'when store is local' do
+        let(:file_store) { ObjectStorage::Store::LOCAL }
 
-        subject
+        it 'deletes the local artifact' do
+          expect(File).to receive(:delete).with(full_path)
+
+          subject
+        end
+      end
+
+      context 'when store is nil' do
+        let(:file_store) { nil }
+
+        it 'deletes the local artifact' do
+          expect(File).to receive(:delete).with(full_path)
+
+          subject
+        end
       end
     end
 
     context 'with a remote artifact' do
       let(:artifact_store_path) { 'remote_file_path' }
-      let(:local) { false }
+      let(:file_store) { ObjectStorage::Store::REMOTE }
       let(:file_double) { double }
 
       before do
@@ -42,5 +55,14 @@ describe Ci::DeleteStoredArtifactsService do
         subject
       end
     end
+
+    context 'with an invalid store' do
+      let(:artifact_store_path) { 'file_path' }
+      let(:file_store) { 'notavalidstore' }
+
+      it 'raises an error' do
+        expect { subject }.to raise_error(described_class::InvalidStoreError)
+      end
+    end
   end
 end
diff --git a/spec/workers/ci/delete_stored_artifacts_worker_spec.rb b/spec/workers/ci/delete_stored_artifacts_worker_spec.rb
index 3438574f77d..5aaf440a600 100644
--- a/spec/workers/ci/delete_stored_artifacts_worker_spec.rb
+++ b/spec/workers/ci/delete_stored_artifacts_worker_spec.rb
@@ -7,10 +7,10 @@ describe Ci::DeleteStoredArtifactsWorker do
     let(:project) { create(:project) }
     let(:worker) { described_class.new }
     let(:store_path) { 'file_path' }
-    let(:local_store) { true }
+    let(:file_store) { ObjectStorage::Store::LOCAL }
     let(:size) { 10 }
 
-    subject { worker.perform(project.id, store_path, local_store, size) }
+    subject { worker.perform(project.id, store_path, file_store, size) }
 
     before do
       allow(UpdateProjectStatistics).to receive(:update_project_statistics!)
@@ -18,7 +18,7 @@ describe Ci::DeleteStoredArtifactsWorker do
     end
 
     it 'calls the delete service' do
-      expect(Ci::DeleteStoredArtifactsService).to receive_message_chain(:new, :execute).with(store_path, local_store)
+      expect(Ci::DeleteStoredArtifactsService).to receive_message_chain(:new, :execute).with(store_path, file_store)
 
       subject
     end