Fix bunch of test failures

app/models/ci/build.rb

@@ -338,6 +338,10 @@ module Ci
       project.running_or_pending_build_count(force: true)
     end
 
+    def browsable_artifacts?
+      artifacts_metadata?
+    end
+
     def artifacts_metadata_entry(path, **options)
       artifacts_metadata.use_file do |metadata_path|
         metadata = Gitlab::Ci::Build::Artifacts::Metadata.new(

config/gitlab.yml.example

@@ -764,6 +764,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
@@ -785,8 +786,6 @@ test:
     # user: YOUR_USERNAME
   pages:
     path: tmp/tests/pages
-  artifacts:
-    path: tmp/tests/artifacts
   repositories:
     storages:
       default:

db/migrate/20170918072949_add_file_store_job_artifacts.rb

-class AddFileStoreJobArtifacts <  ActiveRecord::Migration
+class AddFileStoreJobArtifacts < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 
   disable_ddl_transaction!
@@ -12,6 +12,6 @@ class AddFileStoreJobArtifacts <  ActiveRecord::Migration
   end
 
   def down
-    drop_column(:ci_job_artifacts, :file_store)
+    remove_column(:ci_job_artifacts, :file_store)
   end
 end

db/schema.rb

@@ -474,7 +474,6 @@ ActiveRecord::Schema.define(version: 20171124165823) do
     t.integer "source"
     t.boolean "protected"
     t.integer "failure_reason"
-    t.integer "config_source"
   end
 
   add_index "ci_pipelines", ["auto_canceled_by_id"], name: "index_ci_pipelines_on_auto_canceled_by_id", using: :btree

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

@@ -117,7 +117,8 @@ describe Projects::ArtifactsController 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) }
+      let(:job) { create(:ci_build, :success, pipeline: pipeline) }
+      let!(:artifact) { create(:ci_job_artifact, job: job, file_store: store) }
 
       shared_examples 'a valid file' do
         it 'serves the file using workhorse' do

spec/factories/ci/job_artifacts.rb

@@ -4,7 +4,6 @@ FactoryGirl.define do
   factory :ci_job_artifact, class: Ci::JobArtifact do
     job factory: :ci_build
     file_type :archive
-    file_store JobArtifactUploader::LOCAL_STORE
 
     trait :remote_store do
       file_store JobArtifactUploader::REMOTE_STORE

spec/requests/api/jobs_spec.rb

@@ -299,13 +299,16 @@ describe API::Jobs do
     context 'normal authentication' do
       before do
         stub_artifacts_object_storage
-        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) }
 
+          before do
+            get api("/projects/#{project.id}/jobs/#{job.id}/artifacts", api_user)
+          end
+
           context 'authorized user' do
             it_behaves_like 'downloads artifact'
           end
@@ -320,7 +323,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) }
+          let!(:artifact) { create(:ci_job_artifact, :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_gitlab_http_status(302)
@@ -328,6 +338,8 @@ describe API::Jobs do
         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
@@ -440,7 +452,14 @@ describe API::Jobs do
         end
 
         context 'when artifacts are stored remotely' do
-          let(:job) { create(:ci_build, :artifacts, :remote_store, pipeline: pipeline, user: api_user) }
+          let(:job) { create(:ci_build, pipeline: pipeline, user: api_user) }
+          let!(:artifact) { create(:ci_job_artifact, :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_gitlab_http_status(302)

spec/requests/api/runner_spec.rb

@@ -1134,7 +1134,7 @@ describe API::Runner do
             # by configuring this path we allow to pass file from @tmpdir only
             # but all temporary files are stored in system tmp directory
             @tmpdir = Dir.mktmpdir
-            allow(ArtifactUploader).to receive(:artifacts_upload_path).and_return(@tmpdir)
+            allow(JobArtifactUploader).to receive(:artifacts_upload_path).and_return(@tmpdir)
           end
 
           after do
@@ -1161,12 +1161,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, file_store: store, job: job)
+
+            download_artifact
+          end
 
           context 'when using job token' do
             context 'when artifacts are stored locally' do
@@ -1182,7 +1185,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_gitlab_http_status(302)
@@ -1201,12 +1205,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

@@ -3,7 +3,7 @@ require 'spec_helper'
 describe API::V3::Builds do
   set(:user) { create(:user) }
   let(:api_user) { user }
-  let!(:project) { create(:project, :repository, creator: user, public_builds: false) }
+  set(:project) { create(:project, :repository, creator: user, public_builds: false) }
   let!(:developer) { create(:project_member, :developer, user: user, project: project) }
   let(:reporter) { create(:project_member, :reporter, project: project) }
   let(:guest) { create(:project_member, :guest, project: project) }
@@ -215,9 +215,12 @@ 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, :remote_store, job: build) }
 
         it 'returns location redirect' do
+          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, :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_gitlab_http_status(302)

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 :all do
+  before(:context) do
     Rake.application.rake_require 'tasks/gitlab/artifacts'
   end
 
-  describe 'migrate' do
-    let(:job) { create(:ci_build, :artifacts, artifacts_file_store: store, artifacts_metadata_store: store) }
+  subject { run_rake_task('gitlab:artifacts:migrate') }
 
-    subject { run_rake_task('gitlab:artifacts:migrate') }
+  context 'legacy artifacts' do
+    describe 'migrate' do
+      let(:build) { create(:ci_build, artifacts_file_store: store, artifacts_metadata_store: store) }
+
+      before do
+        # Mock the legacy way of artifacts
+        path = Rails.root.join('shared/artifacts',
+                               build.created_at.utc.strftime('%Y_%m'),
+                               build.project_id.to_s,
+                               build.id.to_s)
+
+        FileUtils.mkdir_p(path)
+        FileUtils.copy(
+          Rails.root.join('spec/fixtures/ci_build_artifacts.zip'),
+          File.join(path, "ci_build_artifacts.zip"))
+
+        FileUtils.copy(
+          Rails.root.join('spec/fixtures/ci_build_artifacts_metadata.gz'),
+          File.join(path, "ci_build_artifacts_metadata.gz"))
+
+        build.update_columns(
+          artifacts_file: 'ci_build_artifacts.zip',
+          artifacts_metadata: 'ci_build_artifacts_metadata.gz')
+      end
+
+      context 'when local storage is used' do
+        let(:store) { ObjectStoreUploader::LOCAL_STORE }
+
+        context 'and job does not have file store defined' do
+          before do
+            build.update(artifacts_file_store: nil)
+          end
+
+          it "migrates file to remote storage" do
+            stub_artifacts_object_storage
+
+            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
+          it "migrates file to remote storage" do
+            stub_artifacts_object_storage
+
+            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(:store) { ObjectStoreUploader::REMOTE_STORE }
+
+        it "file stays on remote storage" do
+          stub_artifacts_object_storage
+
+          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, 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
         before do
-          stub_artifacts_object_storage
-          job.update(artifacts_file_store: nil)
+          artifact.update(file_store: nil)
         end
 
         it "migrates file to remote storage" do
+          stub_artifacts_object_storage
+
           subject
 
-          expect(job.reload.artifacts_file_store).to eq(ObjectStoreUploader::REMOTE_STORE)
-          expect(job.reload.artifacts_metadata_store).to eq(ObjectStoreUploader::REMOTE_STORE)
+          expect(artifact.reload.file_store).to eq(ObjectStoreUploader::REMOTE_STORE)
         end
       end
 
       context 'and remote storage is defined' do
-        before do
+        it "migrates file to remote storage" do
           stub_artifacts_object_storage
-          job
-        end
 
-        it "migrates file to remote storage" do
           subject
 
-          expect(job.reload.artifacts_file_store).to eq(ObjectStoreUploader::REMOTE_STORE)
-          expect(job.reload.artifacts_metadata_store).to eq(ObjectStoreUploader::REMOTE_STORE)
+          expect(artifact.reload.file_store).to eq(ObjectStoreUploader::REMOTE_STORE)
         end
       end
 
       context 'and remote storage is not defined' do
-        before do
-          job
-        end
-
         it "fails to migrate to remote storage" do
           subject
 
-          expect(job.reload.artifacts_file_store).to eq(ObjectStoreUploader::LOCAL_STORE)
-          expect(job.reload.artifacts_metadata_store).to eq(ObjectStoreUploader::LOCAL_STORE)
+          expect(artifact.reload.file_store).to eq(ObjectStoreUploader::LOCAL_STORE)
         end
       end
     end
@@ -58,16 +128,12 @@ describe 'gitlab:artifacts namespace rake task' do
     context 'when remote storage is used' do
       let(:store) { ObjectStoreUploader::REMOTE_STORE }
 
-      before do
+      it "file stays on remote storage" do
         stub_artifacts_object_storage
-        job
-      end
 
-      it "file stays on remote storage" do
         subject
 
-        expect(job.reload.artifacts_file_store).to eq(ObjectStoreUploader::REMOTE_STORE)
-        expect(job.reload.artifacts_metadata_store).to eq(ObjectStoreUploader::REMOTE_STORE)
+        expect(artifact.reload.file_store).to eq(ObjectStoreUploader::REMOTE_STORE)
       end
     end
   end

spec/uploaders/object_store_uploader_spec.rb

@@ -4,18 +4,18 @@ 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) }
 
   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(:artifacts_file_store).twice.and_return(nil)
+        expect(object).to receive(:file_store).twice.and_return(nil)
       end
 
       it "returns LOCAL_STORE" do
@@ -26,7 +26,7 @@ describe ObjectStoreUploader do
 
     context 'when value is set' do
       before do
-        expect(object).to receive(:artifacts_file_store).twice.and_return(described_class::REMOTE_STORE)
+        expect(object).to receive(:file_store).twice.and_return(described_class::REMOTE_STORE)
       end
 
       it "returns given value" do
@@ -38,7 +38,7 @@ describe ObjectStoreUploader do
 
   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
@@ -47,7 +47,7 @@ describe ObjectStoreUploader do
   describe '#file_storage?' do
     context 'when file storage is used' do
       before do
-        expect(object).to receive(:artifacts_file_store).and_return(described_class::LOCAL_STORE)
+        expect(object).to receive(:file_store).and_return(described_class::LOCAL_STORE)
       end
 
       it { expect(uploader).to be_file_storage }
@@ -57,7 +57,7 @@ describe ObjectStoreUploader do
       before do
         uploader_class.storage_options double(
           object_store: double(enabled: true))
-        expect(object).to receive(:artifacts_file_store).and_return(described_class::REMOTE_STORE)
+        expect(object).to receive(:file_store).and_return(described_class::REMOTE_STORE)
       end
 
       it { expect(uploader).not_to be_file_storage }
@@ -82,9 +82,9 @@ describe ObjectStoreUploader do
     end
   end
 
-  context 'when using ArtifactsUploader' do
-    let(:job) { create(:ci_build, :artifacts, artifacts_file_store: store) }
-    let(:uploader) { job.artifacts_file }
+  context 'when using JobArtifactsUploader' do
+    let(:artifact) { create(:ci_job_artifact, file_store: store) }
+    let(:uploader) { artifact.file }
 
     context 'checking described_class' do
       let(:store) { described_class::LOCAL_STORE }
@@ -103,7 +103,7 @@ describe ObjectStoreUploader do
       let(:store) { nil }
 
       it "sets the store to LOCAL_STORE" do
-        expect(job.artifacts_file_store).to eq(described_class::LOCAL_STORE)
+        expect(artifact.file_store).to eq(described_class::LOCAL_STORE)
       end
     end
 
@@ -130,8 +130,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, file_store: store) }
+      let(:uploader) { artifact.file }
       let(:store) { described_class::LOCAL_STORE }
 
       subject { uploader.migrate!(new_store) }
@@ -214,7 +214,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
@@ -272,7 +272,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
@@ -284,7 +284,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

spec/workers/object_storage_upload_worker_spec.rb

@@ -48,12 +48,33 @@ describe ObjectStorageUploadWorker do
     end
   end
 
-  context 'for artifacts' do
-    let(:job) { create(:ci_build, :artifacts, artifacts_file_store: store, artifacts_metadata_store: store) }
+  context 'for legacy artifacts' do
+    let(:build) { create(:ci_build) }
     let(:uploader_class) { ArtifactUploader }
     let(:subject_class) { Ci::Build }
     let(:file_field) { :artifacts_file }
-    let(:subject_id) { job.id }
+    let(:subject_id) { build.id }
+
+    before do
+      # Mock the legacy way of artifacts
+      path = Rails.root.join('shared/artifacts',
+                  build.created_at.utc.strftime('%Y_%m'),
+                  build.project_id.to_s,
+                  build.id.to_s)
+
+      FileUtils.mkdir_p(path)
+      FileUtils.copy(
+        Rails.root.join('spec/fixtures/ci_build_artifacts.zip'),
+        File.join(path, "ci_build_artifacts.zip"))
+
+      FileUtils.copy(
+        Rails.root.join('spec/fixtures/ci_build_artifacts_metadata.gz'),
+        File.join(path, "ci_build_artifacts_metadata.gz"))
+
+      build.update_columns(
+        artifacts_file: 'ci_build_artifacts.zip',
+        artifacts_metadata: 'ci_build_artifacts_metadata.gz')
+    end
 
     context 'when local storage is used' do
       let(:store) { local }
@@ -61,13 +82,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 +96,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) }
+    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