diff options
author | Grzegorz Bizon <grzegorz@gitlab.com> | 2017-12-05 17:31:33 +0300 |
---|---|---|
committer | Kamil TrzciĆski <ayufan@ayufan.eu> | 2018-02-28 22:44:09 +0300 |
commit | 6ca02a41500790b3e9061dd8836540955b9aaf7c (patch) | |
tree | 5c66c4826cafa2657fe25d85eb9e189b5f290f32 /spec | |
parent | ec72abf53fd82ca3e7f126536a83b27b368696ec (diff) |
Merge branch 'zj-multiple-artifacts-ee' into 'master'
Multiple artifacts ee
See merge request gitlab-org/gitlab-ee!3276
Diffstat (limited to 'spec')
-rw-r--r-- | spec/controllers/projects/artifacts_controller_spec.rb | 11 | ||||
-rw-r--r-- | spec/factories/ci/job_artifacts.rb | 4 | ||||
-rw-r--r-- | spec/models/ci/build_spec.rb | 16 | ||||
-rw-r--r-- | spec/requests/api/jobs_spec.rb | 61 | ||||
-rw-r--r-- | spec/requests/api/runner_spec.rb | 20 | ||||
-rw-r--r-- | spec/requests/api/v3/builds_spec.rb | 16 | ||||
-rw-r--r-- | spec/serializers/pipeline_serializer_spec.rb | 3 | ||||
-rw-r--r-- | spec/support/stub_object_storage.rb | 2 | ||||
-rw-r--r-- | spec/tasks/gitlab/artifacts_rake_spec.rb | 118 | ||||
-rw-r--r-- | spec/uploaders/job_artifact_uploader_spec.rb | 14 | ||||
-rw-r--r-- | spec/uploaders/legacy_artifact_uploader_spec.rb | 3 | ||||
-rw-r--r-- | spec/uploaders/object_store_uploader_spec.rb | 99 | ||||
-rw-r--r-- | spec/workers/object_storage_upload_worker_spec.rb | 37 |
13 files changed, 356 insertions, 48 deletions
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 |