summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--app/uploaders/artifact_uploader.rb4
-rw-r--r--app/uploaders/gitlab_uploader.rb19
-rw-r--r--app/uploaders/lfs_object_uploader.rb12
-rw-r--r--changelogs/unreleased/sh-fix-refactor-uploader-work-dir.yml4
-rw-r--r--spec/uploaders/artifact_uploader_spec.rb15
-rw-r--r--spec/uploaders/gitlab_uploader_spec.rb15
-rw-r--r--spec/uploaders/lfs_object_uploader_spec.rb39
7 files changed, 77 insertions, 31 deletions
diff --git a/app/uploaders/artifact_uploader.rb b/app/uploaders/artifact_uploader.rb
index 3bc0408f557..14addb6cf14 100644
--- a/app/uploaders/artifact_uploader.rb
+++ b/app/uploaders/artifact_uploader.rb
@@ -23,6 +23,10 @@ class ArtifactUploader < GitlabUploader
File.join(self.class.local_artifacts_store, 'tmp/cache')
end
+ def work_dir
+ File.join(self.class.local_artifacts_store, 'tmp/work')
+ end
+
private
def default_local_path
diff --git a/app/uploaders/gitlab_uploader.rb b/app/uploaders/gitlab_uploader.rb
index e4e6d6f46b1..136ec6cc6af 100644
--- a/app/uploaders/gitlab_uploader.rb
+++ b/app/uploaders/gitlab_uploader.rb
@@ -53,4 +53,23 @@ class GitlabUploader < CarrierWave::Uploader::Base
def exists?
file.try(:exists?)
end
+
+ # Override this if you don't want to save files by default to the Rails.root directory
+ def work_dir
+ # Default path set by CarrierWave:
+ # https://github.com/carrierwaveuploader/carrierwave/blob/v1.0.0/lib/carrierwave/uploader/cache.rb#L182
+ CarrierWave.tmp_path
+ end
+
+ private
+
+ # To prevent files from moving across filesystems, override the default
+ # implementation:
+ # http://github.com/carrierwaveuploader/carrierwave/blob/v1.0.0/lib/carrierwave/uploader/cache.rb#L181-L183
+ def workfile_path(for_file = original_filename)
+ # To be safe, keep this directory outside of the the cache directory
+ # because calling CarrierWave.clean_cache_files! will remove any files in
+ # the cache directory.
+ File.join(work_dir, @cache_id, version_name.to_s, for_file)
+ end
end
diff --git a/app/uploaders/lfs_object_uploader.rb b/app/uploaders/lfs_object_uploader.rb
index 02589959c2f..d11ebf0f9ca 100644
--- a/app/uploaders/lfs_object_uploader.rb
+++ b/app/uploaders/lfs_object_uploader.rb
@@ -16,16 +16,4 @@ class LfsObjectUploader < GitlabUploader
def work_dir
File.join(Gitlab.config.lfs.storage_path, 'tmp', 'work')
end
-
- private
-
- # To prevent LFS files from moving across filesystems, override the default
- # implementation:
- # http://github.com/carrierwaveuploader/carrierwave/blob/v1.0.0/lib/carrierwave/uploader/cache.rb#L181-L183
- def workfile_path(for_file = original_filename)
- # To be safe, keep this directory outside of the the cache directory
- # because calling CarrierWave.clean_cache_files! will remove any files in
- # the cache directory.
- File.join(work_dir, @cache_id, version_name.to_s, for_file)
- end
end
diff --git a/changelogs/unreleased/sh-fix-refactor-uploader-work-dir.yml b/changelogs/unreleased/sh-fix-refactor-uploader-work-dir.yml
new file mode 100644
index 00000000000..255608bd89a
--- /dev/null
+++ b/changelogs/unreleased/sh-fix-refactor-uploader-work-dir.yml
@@ -0,0 +1,4 @@
+---
+title: Set artifact working directory to be in the destination store to prevent unnecessary I/O
+merge_request:
+author:
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