From d936cdc5fbf054b129650c1fc5774233a463f1df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Sat, 31 Aug 2019 14:06:10 +0200 Subject: Generalize artifact store when deleting Pass the file store when deleting artifacts to make the implementation store agnostic. --- app/models/ci/job_artifact.rb | 4 +-- app/services/ci/delete_stored_artifacts_service.rb | 13 +++++++-- app/workers/ci/delete_stored_artifacts_worker.rb | 4 +-- spec/models/ci/job_artifact_spec.rb | 6 ++-- .../ci/delete_stored_artifacts_service_spec.rb | 34 ++++++++++++++++++---- .../ci/delete_stored_artifacts_worker_spec.rb | 6 ++-- 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 -- cgit v1.2.3