Merge branch 'zj-multiple-artifacts-ee' into 'master'

Multiple artifacts ee

See merge request gitlab-org/gitlab-ee!3276

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

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(

app/uploaders/job_artifact_uploader.rb

-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')

app/uploaders/legacy_artifact_uploader.rb

-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

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

app/uploaders/object_store_uploader.rb

@@ -38,11 +38,16 @@ class ObjectStoreUploader < GitlabUploader
     end
   end
-  attr_reader :subject, :field
-  def initialize(subject, field)
-    @subject = subject
-    @field = field
+  def file_storage?
+    storage.is_a?(CarrierWave::Storage::File)
+  end
+
+  def file_cache_storage?
+    cache_storage.is_a?(CarrierWave::Storage::File)
+  end
+  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

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

db/migrate/20170918072949_add_file_store_job_artifacts.rb

+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

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

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

spec/controllers/projects/artifacts_controller_spec.rb

 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

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

spec/models/ci/build_spec.rb

@@ -158,6 +158,20 @@ describe Ci::Build do
       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) }
+
+        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) }
@@ -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 }

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 'authorized user' do
-          let(:download_headers) do
-            { 'Content-Transfer-Encoding' => 'binary',
-              'Content-Disposition' => 'attachment; filename=ci_build_artifacts.zip' }
+    context 'normal authentication' do
+      before do
+        stub_artifacts_object_storage
+      end
+      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)

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

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)

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

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

spec/tasks/gitlab/artifacts_rake_spec.rb

+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

spec/uploaders/job_artifact_uploader_spec.rb

 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