Merge branch 'sh-fix-refactor-uploader-work-dir' into 'master'

Set artifact working directory to be in the destination store to prevent unnecessary I/O

Closes #33274

See merge request !11905

3 files changed, 50 insertions, 19 deletions

diff --git a/spec/uploaders/artifact_uploader_spec.rb b/spec/uploaders/artifact_uploader_spec.rb
index 24e2e3a9f0e..b3fac65c55e 100644
--- a/spec/uploaders/artifact_uploader_spec.rb
+++ b/spec/uploaders/artifact_uploader_spec.rb
@@ -17,22 +17,29 @@ describe ArtifactUploader do
 
   describe '.artifacts_upload_path' do
     subject { described_class.artifacts_upload_path }
-    
+
     it { is_expected.to start_with(path) }
     it { is_expected.to end_with('tmp/uploads/') }
   end
 
   describe '#store_dir' do
     subject { uploader.store_dir }
-    
+
     it { is_expected.to start_with(path) }
     it { is_expected.to end_with("#{job.project_id}/#{job.id}") }
   end
 
   describe '#cache_dir' do
     subject { uploader.cache_dir }
-    
+
+    it { is_expected.to start_with(path) }
+    it { is_expected.to end_with('/tmp/cache') }
+  end
+
+  describe '#work_dir' do
+    subject { uploader.work_dir }
+
     it { is_expected.to start_with(path) }
-    it { is_expected.to end_with('tmp/cache') }
+    it { is_expected.to end_with('/tmp/work') }
   end
 end
diff --git a/spec/uploaders/gitlab_uploader_spec.rb b/spec/uploaders/gitlab_uploader_spec.rb
index 78e9d9cf46c..a144b39f74f 100644
--- a/spec/uploaders/gitlab_uploader_spec.rb
+++ b/spec/uploaders/gitlab_uploader_spec.rb
@@ -53,4 +53,19 @@ describe GitlabUploader do
       expect(subject.move_to_store).to eq(true)
     end
   end
+
+  describe '#cache!' do
+    it 'moves the file from the working directory to the cache directory' do
+      # One to get the work dir, the other to remove it
+      expect(subject).to receive(:workfile_path).exactly(2).times.and_call_original
+      # Test https://github.com/carrierwavesubject/carrierwave/blob/v1.0.0/lib/carrierwave/sanitized_file.rb#L200
+      expect(FileUtils).to receive(:mv).with(anything, /^#{subject.work_dir}/).and_call_original
+      expect(FileUtils).to receive(:mv).with(/^#{subject.work_dir}/, /#{subject.cache_dir}/).and_call_original
+
+      fixture = Rails.root.join('spec', 'fixtures', 'rails_sample.jpg')
+      subject.cache!(fixture_file_upload(fixture))
+
+      expect(subject.file.path).to match(/#{subject.cache_dir}/)
+    end
+  end
 end
diff --git a/spec/uploaders/lfs_object_uploader_spec.rb b/spec/uploaders/lfs_object_uploader_spec.rb
index c3b72e7d677..7088bc23334 100644
--- a/spec/uploaders/lfs_object_uploader_spec.rb
+++ b/spec/uploaders/lfs_object_uploader_spec.rb
@@ -1,21 +1,9 @@
 require 'spec_helper'
 
 describe LfsObjectUploader do
-  let(:uploader) { described_class.new(build_stubbed(:empty_project)) }
-
-  describe '#cache!' do
-    it 'caches the file in the cache directory' do
-      # One to get the work dir, the other to remove it
-      expect(uploader).to receive(:workfile_path).exactly(2).times.and_call_original
-      expect(FileUtils).to receive(:mv).with(anything, /^#{uploader.work_dir}/).and_call_original
-      expect(FileUtils).to receive(:mv).with(/^#{uploader.work_dir}/, /^#{uploader.cache_dir}/).and_call_original
-
-      fixture = Rails.root.join('spec', 'fixtures', 'rails_sample.jpg')
-      uploader.cache!(fixture_file_upload(fixture))
-
-      expect(uploader.file.path).to start_with(uploader.cache_dir)
-    end
-  end
+  let(:lfs_object) { create(:lfs_object, :with_file) }
+  let(:uploader) { described_class.new(lfs_object) }
+  let(:path) { Gitlab.config.lfs.storage_path }
 
   describe '#move_to_cache' do
     it 'is true' do
@@ -28,4 +16,25 @@ describe LfsObjectUploader do
       expect(uploader.move_to_store).to eq(true)
     end
   end
+
+  describe '#store_dir' do
+    subject { uploader.store_dir }
+
+    it { is_expected.to start_with(path) }
+    it { is_expected.to end_with("#{lfs_object.oid[0, 2]}/#{lfs_object.oid[2, 2]}") }
+  end
+
+  describe '#cache_dir' do
+    subject { uploader.cache_dir }
+
+    it { is_expected.to start_with(path) }
+    it { is_expected.to end_with('/tmp/cache') }
+  end
+
+  describe '#work_dir' do
+    subject { uploader.work_dir }
+
+    it { is_expected.to start_with(path) }
+    it { is_expected.to end_with('/tmp/work') }
+  end
 end