diff options
23 files changed, 403 insertions, 111 deletions
diff --git a/app/controllers/projects/artifacts_controller.rb b/app/controllers/projects/artifacts_controller.rb index 3995a2fc37a..abc283d7aa9 100644 --- a/app/controllers/projects/artifacts_controller.rb +++ b/app/controllers/projects/artifacts_controller.rb @@ -42,8 +42,7 @@ class Projects::ArtifactsController < Projects::ApplicationController end def raw - path = Gitlab::Ci::Build::Artifacts::Path - .new(params[:path]) + path = Gitlab::Ci::Build::Artifacts::Path.new(params[:path]) send_artifacts_entry(build, path) end @@ -72,7 +71,7 @@ class Projects::ArtifactsController < Projects::ApplicationController end def validate_artifacts! - render_404 unless build && build.artifacts? + render_404 unless build&.artifacts? end def build diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index ddd075e1fcb..7cf8bdd968b 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -45,7 +45,7 @@ module Ci end scope :with_artifacts_not_expired, ->() { with_artifacts.where('artifacts_expire_at IS NULL OR artifacts_expire_at > ?', Time.now) } scope :with_expired_artifacts, ->() { with_artifacts.where('artifacts_expire_at < ?', Time.now) } - scope :with_artifacts_stored_locally, ->() { with_artifacts.where(artifacts_file_store: [nil, ArtifactUploader::LOCAL_STORE]) } + scope :with_artifacts_stored_locally, ->() { with_artifacts.where(artifacts_file_store: [nil, LegacyArtifactUploader::LOCAL_STORE]) } scope :last_month, ->() { where('created_at > ?', Date.today - 1.month) } scope :manual_actions, ->() { where(when: :manual, status: COMPLETED_STATUSES + [:manual]) } scope :ref_protected, -> { where(protected: true) } @@ -361,22 +361,10 @@ module Ci project.running_or_pending_build_count(force: true) end - def artifacts? - !artifacts_expired? && artifacts_file.exists? - end - def browsable_artifacts? artifacts_metadata? end - def downloadable_single_artifacts_file? - artifacts_metadata? && artifacts_file.file_storage? - end - - def artifacts_metadata? - artifacts? && artifacts_metadata.exists? - end - def artifacts_metadata_entry(path, **options) artifacts_metadata.use_file do |metadata_path| metadata = Gitlab::Ci::Build::Artifacts::Metadata.new( diff --git a/app/uploaders/job_artifact_uploader.rb b/app/uploaders/job_artifact_uploader.rb index 15dfb5a5763..a0757dbe6b2 100644 --- a/app/uploaders/job_artifact_uploader.rb +++ b/app/uploaders/job_artifact_uploader.rb @@ -1,5 +1,5 @@ -class JobArtifactUploader < GitlabUploader - storage :file +class JobArtifactUploader < ObjectStoreUploader + storage_options Gitlab.config.artifacts def self.local_store_path Gitlab.config.artifacts.path @@ -15,24 +15,8 @@ class JobArtifactUploader < GitlabUploader model.size end - def store_dir - default_local_path - end - - def cache_dir - File.join(self.class.local_store_path, 'tmp/cache') - end - - def work_dir - File.join(self.class.local_store_path, 'tmp/work') - end - private - def default_local_path - File.join(self.class.local_store_path, default_path) - end - def default_path creation_date = model.created_at.utc.strftime('%Y_%m_%d') diff --git a/app/uploaders/legacy_artifact_uploader.rb b/app/uploaders/legacy_artifact_uploader.rb index 4f7f8a63108..476a46c1754 100644 --- a/app/uploaders/legacy_artifact_uploader.rb +++ b/app/uploaders/legacy_artifact_uploader.rb @@ -1,5 +1,5 @@ -class LegacyArtifactUploader < GitlabUploader - storage :file +class LegacyArtifactUploader < ObjectStoreUploader + storage_options Gitlab.config.artifacts def self.local_store_path Gitlab.config.artifacts.path @@ -9,24 +9,8 @@ class LegacyArtifactUploader < GitlabUploader File.join(self.local_store_path, 'tmp/uploads/') end - def store_dir - default_local_path - end - - def cache_dir - File.join(self.class.local_store_path, 'tmp/cache') - end - - def work_dir - File.join(self.class.local_store_path, 'tmp/work') - end - private - def default_local_path - File.join(self.class.local_store_path, default_path) - end - def default_path File.join(model.created_at.utc.strftime('%Y_%m'), model.project_id.to_s, model.id.to_s) end diff --git a/app/uploaders/lfs_object_uploader.rb b/app/uploaders/lfs_object_uploader.rb index 8a5f599c1d3..88cf0450dcd 100644 --- a/app/uploaders/lfs_object_uploader.rb +++ b/app/uploaders/lfs_object_uploader.rb @@ -7,12 +7,12 @@ class LfsObjectUploader < ObjectStoreUploader end def filename - subject.oid[4..-1] + model.oid[4..-1] end private def default_path - "#{subject.oid[0, 2]}/#{subject.oid[2, 2]}" + "#{model.oid[0, 2]}/#{model.oid[2, 2]}" end end diff --git a/app/uploaders/object_store_uploader.rb b/app/uploaders/object_store_uploader.rb index 9b9f47d5943..b5de0357a5f 100644 --- a/app/uploaders/object_store_uploader.rb +++ b/app/uploaders/object_store_uploader.rb @@ -38,11 +38,16 @@ class ObjectStoreUploader < GitlabUploader end end - attr_reader :subject, :field + def file_storage? + storage.is_a?(CarrierWave::Storage::File) + end + + def file_cache_storage? + cache_storage.is_a?(CarrierWave::Storage::File) + end - def initialize(subject, field) - @subject = subject - @field = field + def real_object_store + model.public_send(store_serialization_column) # rubocop:disable GitlabSecurity/PublicSend end def object_store @@ -51,7 +56,7 @@ class ObjectStoreUploader < GitlabUploader def object_store=(value) @storage = nil - subject.public_send(:"#{field}_store=", value) + model.public_send(:"#{store_serialization_column}=", value) # rubocop:disable GitlabSecurity/PublicSend end def store_dir @@ -99,7 +104,7 @@ class ObjectStoreUploader < GitlabUploader # since we change storage store the new storage # in case of failure delete new file begin - subject.save! + model.save! rescue => e new_file.delete self.object_store = old_store @@ -113,7 +118,7 @@ class ObjectStoreUploader < GitlabUploader def schedule_migration_to_object_storage(new_file) if self.class.object_store_enabled? && licensed? && file_storage? - ObjectStorageUploadWorker.perform_async(self.class.name, subject.class.name, field, subject.id) + ObjectStorageUploadWorker.perform_async(self.class.name, model.class.name, mounted_as, model.id) end end @@ -178,6 +183,14 @@ class ObjectStoreUploader < GitlabUploader raise NotImplementedError end + def serialization_column + model.class.uploader_option(mounted_as, :mount_on) || mounted_as + end + + def store_serialization_column + :"#{serialization_column}_store" + end + def storage @storage ||= if object_store == REMOTE_STORE diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example index 0ffacad400b..e2256c5c118 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -672,6 +672,7 @@ test: aws_secret_access_key: AWS_SECRET_ACCESS_KEY region: eu-central-1 artifacts: + path: tmp/tests/artifacts enabled: true # The location where build artifacts are stored (default: shared/artifacts). # path: shared/artifacts diff --git a/db/migrate/20170918072949_add_file_store_job_artifacts.rb b/db/migrate/20170918072949_add_file_store_job_artifacts.rb new file mode 100644 index 00000000000..8c265bb6aca --- /dev/null +++ b/db/migrate/20170918072949_add_file_store_job_artifacts.rb @@ -0,0 +1,14 @@ +class AddFileStoreJobArtifacts < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + disable_ddl_transaction! + DOWNTIME = false + + def up + add_column(:ci_job_artifacts, :file_store, :integer) + end + + def down + remove_column(:ci_job_artifacts, :file_store) + end +end diff --git a/db/schema.rb b/db/schema.rb index 8846bbd975c..8aa6a87657e 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -326,6 +326,7 @@ ActiveRecord::Schema.define(version: 20171205190711) do t.integer "project_id", null: false t.integer "job_id", null: false t.integer "file_type", null: false + t.integer "file_store" t.integer "size", limit: 8 t.datetime_with_timezone "created_at", null: false t.datetime_with_timezone "updated_at", null: false diff --git a/lib/tasks/gitlab/artifacts.rake b/lib/tasks/gitlab/artifacts.rake index 29d8a145be8..494317d99c7 100644 --- a/lib/tasks/gitlab/artifacts.rake +++ b/lib/tasks/gitlab/artifacts.rake @@ -12,8 +12,8 @@ namespace :gitlab do .with_artifacts_stored_locally .find_each(batch_size: 10) do |build| begin - build.artifacts_file.migrate!(ArtifactUploader::REMOTE_STORE) - build.artifacts_metadata.migrate!(ArtifactUploader::REMOTE_STORE) + build.artifacts_file.migrate!(ObjectStoreUploader::REMOTE_STORE) + build.artifacts_metadata.migrate!(ObjectStoreUploader::REMOTE_STORE) logger.info("Transferred artifacts of #{build.id} of #{build.artifacts_size} to object storage") rescue => e diff --git a/spec/controllers/projects/artifacts_controller_spec.rb b/spec/controllers/projects/artifacts_controller_spec.rb index bc3d277fc8e..581b3e4e4ab 100644 --- a/spec/controllers/projects/artifacts_controller_spec.rb +++ b/spec/controllers/projects/artifacts_controller_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe Projects::ArtifactsController do - set(:user) { create(:user) } + let(:user) { project.owner } set(:project) { create(:project, :repository, :public) } let(:pipeline) do @@ -15,8 +15,6 @@ describe Projects::ArtifactsController do let(:job) { create(:ci_build, :success, :artifacts, pipeline: pipeline) } before do - project.add_developer(user) - sign_in(user) end @@ -115,12 +113,12 @@ describe Projects::ArtifactsController do describe 'GET raw' do context 'when the file exists' do let(:path) { 'ci_artifacts.txt' } - let(:job) { create(:ci_build, :success, :artifacts, pipeline: pipeline, artifacts_file_store: store, artifacts_metadata_store: store) } shared_examples 'a valid file' do it 'serves the file using workhorse' do subject + expect(response).to have_gitlab_http_status(200) expect(send_data).to start_with('artifacts-entry:') expect(params.keys).to eq(%w(Archive Entry)) @@ -144,8 +142,9 @@ describe Projects::ArtifactsController do context 'when using local file storage' do it_behaves_like 'a valid file' do + let(:job) { create(:ci_build, :success, :artifacts, pipeline: pipeline) } let(:store) { ObjectStoreUploader::LOCAL_STORE } - let(:archive_path) { ArtifactUploader.local_store_path } + let(:archive_path) { JobArtifactUploader.local_store_path } end end @@ -157,7 +156,7 @@ describe Projects::ArtifactsController do it_behaves_like 'a valid file' do let!(:artifact) { create(:ci_job_artifact, :archive, :remote_store, job: job) } let!(:job) { create(:ci_build, :success, pipeline: pipeline) } - let(:store) { ObjectStorage::Store::REMOTE } + let(:store) { ObjectStoreUploader::REMOTE_STORE } let(:archive_path) { 'https://' } end end diff --git a/spec/factories/ci/job_artifacts.rb b/spec/factories/ci/job_artifacts.rb index 538dc422832..39185249695 100644 --- a/spec/factories/ci/job_artifacts.rb +++ b/spec/factories/ci/job_artifacts.rb @@ -5,6 +5,10 @@ FactoryGirl.define do job factory: :ci_build file_type :archive + trait :remote_store do + file_store JobArtifactUploader::REMOTE_STORE + end + after :build do |artifact| artifact.project ||= artifact.job.project end diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 1a20c2dda00..83352421d7d 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -166,6 +166,20 @@ describe Ci::Build do context 'artifacts archive does not exist' do let(:build) { create(:ci_build) } + context 'is not expired' do + it { is_expected.to be_truthy } + end + end + end + + context 'when legacy artifacts are used' do + let(:build) { create(:ci_build, :legacy_artifacts) } + + subject { build.artifacts? } + + context 'artifacts archive does not exist' do + let(:build) { create(:ci_build) } + it { is_expected.to be_falsy } end @@ -190,7 +204,7 @@ describe Ci::Build do context 'artifacts metadata does not exist' do before do - build.update_attributes(artifacts_metadata: nil) + build.update_attributes(legacy_artifacts_metadata: nil) end it { is_expected.to be_falsy } diff --git a/spec/requests/api/jobs_spec.rb b/spec/requests/api/jobs_spec.rb index a435945fea2..9ebf5bf7e97 100644 --- a/spec/requests/api/jobs_spec.rb +++ b/spec/requests/api/jobs_spec.rb @@ -288,14 +288,21 @@ describe API::Jobs do get api("/projects/#{project.id}/jobs/#{job.id}/artifacts", api_user) end - context 'job with artifacts' do - context 'when artifacts are stored locally' do - let(:job) { create(:ci_build, :artifacts, pipeline: pipeline) } + context 'normal authentication' do + before do + stub_artifacts_object_storage + end - context 'authorized user' do - let(:download_headers) do - { 'Content-Transfer-Encoding' => 'binary', - 'Content-Disposition' => 'attachment; filename=ci_build_artifacts.zip' } + context 'job with artifacts' do + context 'when artifacts are stored locally' do + let(:job) { create(:ci_build, :artifacts, pipeline: pipeline) } + + before do + get api("/projects/#{project.id}/jobs/#{job.id}/artifacts", api_user) + end + + context 'authorized user' do + it_behaves_like 'downloads artifact' end it 'returns specific job artifacts' do @@ -305,13 +312,40 @@ describe API::Jobs do end end - context 'unauthorized user' do - let(:api_user) { nil } + context 'when artifacts are stored remotely' do + let(:job) { create(:ci_build, pipeline: pipeline) } + let!(:artifact) { create(:ci_job_artifact, :archive, :remote_store, job: job) } + + before do + job.reload + + get api("/projects/#{project.id}/jobs/#{job.id}/artifacts", api_user) + end it 'does not return specific job artifacts' do expect(response).to have_http_status(401) end end + + it 'does not return job artifacts if not uploaded' do + get api("/projects/#{project.id}/jobs/#{job.id}/artifacts", api_user) + + expect(response).to have_gitlab_http_status(404) + end + end + end + + context 'authorized by job_token' do + let(:job) { create(:ci_build, :artifacts, pipeline: pipeline, user: api_user) } + + before do + get api("/projects/#{project.id}/jobs/#{job.id}/artifacts"), job_token: job.token + end + + context 'user is developer' do + let(:api_user) { user } + + it_behaves_like 'downloads artifact' end context 'when artifacts are stored remotely' do @@ -402,7 +436,14 @@ describe API::Jobs do end context 'when artifacts are stored remotely' do - let(:job) { create(:ci_build, :artifacts, :remote_store, pipeline: pipeline) } + let(:job) { create(:ci_build, pipeline: pipeline, user: api_user) } + let!(:artifact) { create(:ci_job_artifact, :archive, :remote_store, job: job) } + + before do + job.reload + + get api("/projects/#{project.id}/jobs/#{job.id}/artifacts", api_user) + end it 'returns location redirect' do expect(response).to have_http_status(302) diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index 3406b17401f..5c6eee09285 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -1151,12 +1151,15 @@ describe API::Runner do describe 'GET /api/v4/jobs/:id/artifacts' do let(:token) { job.token } - before do - download_artifact - end - context 'when job has artifacts' do - let(:job) { create(:ci_build, :artifacts) } + let(:job) { create(:ci_build) } + let(:store) { JobArtifactUploader::LOCAL_STORE } + + before do + create(:ci_job_artifact, :archive, file_store: store, job: job) + + download_artifact + end context 'when using job token' do context 'when artifacts are stored locally' do @@ -1172,7 +1175,8 @@ describe API::Runner do end context 'when artifacts are stored remotely' do - let(:job) { create(:ci_build, :artifacts, :remote_store) } + let(:store) { JobArtifactUploader::REMOTE_STORE } + let!(:job) { create(:ci_build) } it 'download artifacts' do expect(response).to have_http_status(302) @@ -1191,12 +1195,16 @@ describe API::Runner do context 'when job does not has artifacts' do it 'responds with not found' do + download_artifact + expect(response).to have_gitlab_http_status(404) end end def download_artifact(params = {}, request_headers = headers) params = params.merge(token: token) + job.reload + get api("/jobs/#{job.id}/artifacts"), params, request_headers end end diff --git a/spec/requests/api/v3/builds_spec.rb b/spec/requests/api/v3/builds_spec.rb index 266ae654227..862bf7e540d 100644 --- a/spec/requests/api/v3/builds_spec.rb +++ b/spec/requests/api/v3/builds_spec.rb @@ -215,10 +215,13 @@ describe API::V3::Builds do end context 'when artifacts are stored remotely' do - let(:build) { create(:ci_build, :artifacts, :remote_store, pipeline: pipeline) } + let(:build) { create(:ci_build, pipeline: pipeline) } + let!(:artifact) { create(:ci_job_artifact, :archive, :remote_store, job: build) } it 'returns location redirect' do - expect(response).to have_http_status(302) + get v3_api("/projects/#{project.id}/builds/#{build.id}/artifacts", api_user) + + expect(response).to have_gitlab_http_status(302) end end @@ -309,7 +312,14 @@ describe API::V3::Builds do end context 'when artifacts are stored remotely' do - let(:build) { create(:ci_build, :artifacts, :remote_store, pipeline: pipeline) } + let(:build) { create(:ci_build, pipeline: pipeline) } + let!(:artifact) { create(:ci_job_artifact, :archive, :remote_store, job: build) } + + before do + build.reload + + get v3_api("/projects/#{project.id}/builds/#{build.id}/artifacts", api_user) + end it 'returns location redirect' do expect(response).to have_http_status(302) diff --git a/spec/serializers/pipeline_serializer_spec.rb b/spec/serializers/pipeline_serializer_spec.rb index 88d347322a6..e40edbfb421 100644 --- a/spec/serializers/pipeline_serializer_spec.rb +++ b/spec/serializers/pipeline_serializer_spec.rb @@ -117,7 +117,8 @@ describe PipelineSerializer do shared_examples 'no N+1 queries' do it 'verifies number of queries', :request_store do recorded = ActiveRecord::QueryRecorder.new { subject } - expect(recorded.count).to be_within(1).of(36) + + expect(recorded.count).to be_within(1).of(40) expect(recorded.cached_count).to eq(0) end end diff --git a/spec/support/stub_object_storage.rb b/spec/support/stub_object_storage.rb index df7e05585d2..cf5746bc29f 100644 --- a/spec/support/stub_object_storage.rb +++ b/spec/support/stub_object_storage.rb @@ -18,7 +18,7 @@ module StubConfiguration def stub_artifacts_object_storage(**params) stub_object_storage_uploader(config: Gitlab.config.artifacts.object_store, - uploader: ArtifactUploader, + uploader: JobArtifactUploader, remote_directory: 'artifacts', **params) end diff --git a/spec/tasks/gitlab/artifacts_rake_spec.rb b/spec/tasks/gitlab/artifacts_rake_spec.rb new file mode 100644 index 00000000000..a30823b8875 --- /dev/null +++ b/spec/tasks/gitlab/artifacts_rake_spec.rb @@ -0,0 +1,118 @@ +require 'rake_helper' + +describe 'gitlab:artifacts namespace rake task' do + before(:context) do + Rake.application.rake_require 'tasks/gitlab/artifacts' + end + + let(:object_storage_enabled) { false } + + before do + stub_artifacts_object_storage(enabled: object_storage_enabled) + end + + subject { run_rake_task('gitlab:artifacts:migrate') } + + context 'legacy artifacts' do + describe 'migrate' do + let!(:build) { create(:ci_build, :legacy_artifacts, artifacts_file_store: store, artifacts_metadata_store: store) } + + context 'when local storage is used' do + let(:store) { ObjectStoreUploader::LOCAL_STORE } + + context 'and job does not have file store defined' do + let(:object_storage_enabled) { true } + let(:store) { nil } + + it "migrates file to remote storage" do + subject + + expect(build.reload.artifacts_file_store).to eq(ObjectStoreUploader::REMOTE_STORE) + expect(build.reload.artifacts_metadata_store).to eq(ObjectStoreUploader::REMOTE_STORE) + end + end + + context 'and remote storage is defined' do + let(:object_storage_enabled) { true } + + it "migrates file to remote storage" do + subject + + expect(build.reload.artifacts_file_store).to eq(ObjectStoreUploader::REMOTE_STORE) + expect(build.reload.artifacts_metadata_store).to eq(ObjectStoreUploader::REMOTE_STORE) + end + end + + context 'and remote storage is not defined' do + it "fails to migrate to remote storage" do + subject + + expect(build.reload.artifacts_file_store).to eq(ObjectStoreUploader::LOCAL_STORE) + expect(build.reload.artifacts_metadata_store).to eq(ObjectStoreUploader::LOCAL_STORE) + end + end + end + + context 'when remote storage is used' do + let(:object_storage_enabled) { true } + + let(:store) { ObjectStoreUploader::REMOTE_STORE } + + it "file stays on remote storage" do + subject + + expect(build.reload.artifacts_file_store).to eq(ObjectStoreUploader::REMOTE_STORE) + expect(build.reload.artifacts_metadata_store).to eq(ObjectStoreUploader::REMOTE_STORE) + end + end + end + end + + context 'job artifacts' do + let!(:artifact) { create(:ci_job_artifact, :archive, file_store: store) } + + context 'when local storage is used' do + let(:store) { ObjectStoreUploader::LOCAL_STORE } + + context 'and job does not have file store defined' do + let(:object_storage_enabled) { true } + let(:store) { nil } + + it "migrates file to remote storage" do + subject + + expect(artifact.reload.file_store).to eq(ObjectStoreUploader::REMOTE_STORE) + end + end + + context 'and remote storage is defined' do + let(:object_storage_enabled) { true } + + it "migrates file to remote storage" do + subject + + expect(artifact.reload.file_store).to eq(ObjectStoreUploader::REMOTE_STORE) + end + end + + context 'and remote storage is not defined' do + it "fails to migrate to remote storage" do + subject + + expect(artifact.reload.file_store).to eq(ObjectStoreUploader::LOCAL_STORE) + end + end + end + + context 'when remote storage is used' do + let(:object_storage_enabled) { true } + let(:store) { ObjectStoreUploader::REMOTE_STORE } + + it "file stays on remote storage" do + subject + + expect(artifact.reload.file_store).to eq(ObjectStoreUploader::REMOTE_STORE) + end + end + end +end diff --git a/spec/uploaders/job_artifact_uploader_spec.rb b/spec/uploaders/job_artifact_uploader_spec.rb index 14fd5f3600f..decea35c86d 100644 --- a/spec/uploaders/job_artifact_uploader_spec.rb +++ b/spec/uploaders/job_artifact_uploader_spec.rb @@ -1,7 +1,8 @@ require 'spec_helper' describe JobArtifactUploader do - let(:job_artifact) { create(:ci_job_artifact) } + let(:store) { described_class::LOCAL_STORE } + let(:job_artifact) { create(:ci_job_artifact, file_store: store) } let(:uploader) { described_class.new(job_artifact, :file) } let(:local_path) { Gitlab.config.artifacts.path } @@ -15,6 +16,17 @@ describe JobArtifactUploader do it { is_expected.to match(/\h{2}\/\h{2}\/\h{64}\/\d{4}_\d{1,2}_\d{1,2}\/\d+\/\d+\z/) } it { is_expected.to end_with(path) } end + + context 'when using remote storage' do + let(:store) { described_class::REMOTE_STORE } + + before do + stub_artifacts_object_storage + end + + it { is_expected.to match(/\h{2}\/\h{2}\/\h{64}\/\d{4}_\d{1,2}_\d{1,2}\/\d+\/\d+\z/) } + it { is_expected.to end_with(path) } + end end describe '#cache_dir' do diff --git a/spec/uploaders/legacy_artifact_uploader_spec.rb b/spec/uploaders/legacy_artifact_uploader_spec.rb index efeffb78772..7b316072f47 100644 --- a/spec/uploaders/legacy_artifact_uploader_spec.rb +++ b/spec/uploaders/legacy_artifact_uploader_spec.rb @@ -1,7 +1,8 @@ require 'rails_helper' describe LegacyArtifactUploader do - let(:job) { create(:ci_build) } + let(:store) { described_class::LOCAL_STORE } + let(:job) { create(:ci_build, artifacts_file_store: store) } let(:uploader) { described_class.new(job, :legacy_artifacts_file) } let(:local_path) { Gitlab.config.artifacts.path } diff --git a/spec/uploaders/object_store_uploader_spec.rb b/spec/uploaders/object_store_uploader_spec.rb index dd08a40eb97..2f52867bb91 100644 --- a/spec/uploaders/object_store_uploader_spec.rb +++ b/spec/uploaders/object_store_uploader_spec.rb @@ -4,27 +4,91 @@ require 'carrierwave/storage/fog' describe ObjectStoreUploader do let(:uploader_class) { Class.new(described_class) } let(:object) { double } - let(:uploader) { uploader_class.new(object, :artifacts_file) } + let(:uploader) { uploader_class.new(object, :file) } + + before do + allow(object.class).to receive(:uploader_option).with(:file, :mount_on) { nil } + end describe '#object_store' do it "calls artifacts_file_store on object" do - expect(object).to receive(:artifacts_file_store) + expect(object).to receive(:file_store) uploader.object_store end + + context 'when store is null' do + before do + expect(object).to receive(:file_store).twice.and_return(nil) + end + + it "returns LOCAL_STORE" do + expect(uploader.real_object_store).to be_nil + expect(uploader.object_store).to eq(described_class::LOCAL_STORE) + end + end + + context 'when value is set' do + before do + expect(object).to receive(:file_store).twice.and_return(described_class::REMOTE_STORE) + end + + it "returns given value" do + expect(uploader.real_object_store).not_to be_nil + expect(uploader.object_store).to eq(described_class::REMOTE_STORE) + end + end end describe '#object_store=' do it "calls artifacts_file_store= on object" do - expect(object).to receive(:artifacts_file_store=).with(described_class::REMOTE_STORE) + expect(object).to receive(:file_store=).with(described_class::REMOTE_STORE) uploader.object_store = described_class::REMOTE_STORE end end - context 'when using ArtifactsUploader' do - let(:job) { create(:ci_build, :artifacts, artifacts_file_store: store) } - let(:uploader) { job.artifacts_file } + describe '#file_storage?' do + context 'when file storage is used' do + before do + expect(object).to receive(:file_store).and_return(described_class::LOCAL_STORE) + end + + it { expect(uploader).to be_file_storage } + end + + context 'when is remote storage' do + before do + uploader_class.storage_options double( + object_store: double(enabled: true)) + expect(object).to receive(:file_store).and_return(described_class::REMOTE_STORE) + end + + it { expect(uploader).not_to be_file_storage } + end + end + + describe '#file_cache_storage?' do + context 'when file storage is used' do + before do + uploader_class.cache_storage(:file) + end + + it { expect(uploader).to be_file_cache_storage } + end + + context 'when is remote storage' do + before do + uploader_class.cache_storage(:fog) + end + + it { expect(uploader).not_to be_file_cache_storage } + end + end + + context 'when using JobArtifactsUploader' do + let(:artifact) { create(:ci_job_artifact, :archive, file_store: store) } + let(:uploader) { artifact.file } context 'checking described_class' do let(:store) { described_class::LOCAL_STORE } @@ -32,6 +96,19 @@ describe ObjectStoreUploader do it "uploader is of a described_class" do expect(uploader).to be_a(described_class) end + + it 'moves files locally' do + expect(uploader.move_to_store).to be(true) + expect(uploader.move_to_cache).to be(true) + end + end + + context 'when store is null' do + let(:store) { nil } + + it "sets the store to LOCAL_STORE" do + expect(artifact.file_store).to eq(described_class::LOCAL_STORE) + end end describe '#use_file' do @@ -57,8 +134,8 @@ describe ObjectStoreUploader do end describe '#migrate!' do - let(:job) { create(:ci_build, :artifacts, artifacts_file_store: store) } - let(:uploader) { job.artifacts_file } + let(:artifact) { create(:ci_job_artifact, :archive, file_store: store) } + let(:uploader) { artifact.file } let(:store) { described_class::LOCAL_STORE } subject { uploader.migrate!(new_store) } @@ -141,7 +218,7 @@ describe ObjectStoreUploader do context 'when subject save fails' do before do - expect(job).to receive(:save!).and_raise(RuntimeError, "exception") + expect(artifact).to receive(:save!).and_raise(RuntimeError, "exception") end it "does catch an error" do @@ -199,7 +276,7 @@ describe ObjectStoreUploader do context 'when using local storage' do before do - expect(object).to receive(:artifacts_file_store) { described_class::LOCAL_STORE } + expect(object).to receive(:file_store) { described_class::LOCAL_STORE } end it "does not raise an error" do @@ -211,7 +288,7 @@ describe ObjectStoreUploader do before do uploader_class.storage_options double( object_store: double(enabled: true)) - expect(object).to receive(:artifacts_file_store) { described_class::REMOTE_STORE } + expect(object).to receive(:file_store) { described_class::REMOTE_STORE } end context 'feature is not available' do diff --git a/spec/workers/object_storage_upload_worker_spec.rb b/spec/workers/object_storage_upload_worker_spec.rb index 8a8f7a065a0..0922b5feccd 100644 --- a/spec/workers/object_storage_upload_worker_spec.rb +++ b/spec/workers/object_storage_upload_worker_spec.rb @@ -48,12 +48,12 @@ describe ObjectStorageUploadWorker do end end - context 'for artifacts' do - let(:job) { create(:ci_build, :artifacts, artifacts_file_store: store, artifacts_metadata_store: store) } - let(:uploader_class) { ArtifactUploader } + context 'for legacy artifacts' do + let(:build) { create(:ci_build, :legacy_artifacts) } + let(:uploader_class) { LegacyArtifactUploader } let(:subject_class) { Ci::Build } let(:file_field) { :artifacts_file } - let(:subject_id) { job.id } + let(:subject_id) { build.id } context 'when local storage is used' do let(:store) { local } @@ -61,13 +61,12 @@ describe ObjectStorageUploadWorker do context 'and remote storage is defined' do before do stub_artifacts_object_storage - job end it "migrates file to remote storage" do perform - expect(job.reload.artifacts_file_store).to eq(remote) + expect(build.reload.artifacts_file_store).to eq(remote) end context 'for artifacts_metadata' do @@ -76,10 +75,34 @@ describe ObjectStorageUploadWorker do it 'migrates metadata to remote storage' do perform - expect(job.reload.artifacts_metadata_store).to eq(remote) + expect(build.reload.artifacts_metadata_store).to eq(remote) end end end end end + + context 'for job artifacts' do + let(:artifact) { create(:ci_job_artifact, :archive) } + let(:uploader_class) { JobArtifactUploader } + let(:subject_class) { Ci::JobArtifact } + let(:file_field) { :file } + let(:subject_id) { artifact.id } + + context 'when local storage is used' do + let(:store) { local } + + context 'and remote storage is defined' do + before do + stub_artifacts_object_storage + end + + it "migrates file to remote storage" do + perform + + expect(artifact.reload.file_store).to eq(remote) + end + end + end + end end |