Merge branch 'improve-artifacts-file-store-ee' into 'master'

Artifacts store does not use default value

See merge request !2411

app/uploaders/object_store_uploader.rb

@@ -37,10 +37,14 @@ class ObjectStoreUploader < CarrierWave::Uploader::Base
     cache_storage.is_a?(CarrierWave::Storage::File)
   end
 
-  def object_store
+  def real_object_store
     subject.public_send(:"#{field}_store")
   end
 
+  def object_store
+    real_object_store || LOCAL_STORE
+  end
+
   def object_store=(value)
     @storage = nil
     subject.public_send(:"#{field}_store=", value)
@@ -127,7 +131,7 @@ class ObjectStoreUploader < CarrierWave::Uploader::Base
   private
 
   def set_default_local_store(new_file)
-    self.object_store = LOCAL_STORE unless self.object_store
+    self.object_store = LOCAL_STORE unless self.real_object_store
   end
 
   def storage

db/migrate/20170601163708_add_artifacts_store_to_ci_build.rb

@@ -3,15 +3,8 @@ class AddArtifactsStoreToCiBuild < ActiveRecord::Migration
 
   DOWNTIME = false
 
-  disable_ddl_transaction!
-
-  def up
-    add_column_with_default(:ci_builds, :artifacts_file_store, :integer, default: 1)
-    add_column_with_default(:ci_builds, :artifacts_metadata_store, :integer, default: 1)
-  end
-
-  def down
-    remove_column(:ci_builds, :artifacts_file_store)
-    remove_column(:ci_builds, :artifacts_metadata_store)
+  def change
+    add_column(:ci_builds, :artifacts_file_store, :integer)
+    add_column(:ci_builds, :artifacts_metadata_store, :integer)
   end
 end

db/schema.rb

@@ -294,8 +294,8 @@ ActiveRecord::Schema.define(version: 20170707184244) do
     t.integer "auto_canceled_by_id"
     t.boolean "retried"
     t.integer "stage_id"
-    t.integer "artifacts_file_store", default: 1, null: false
-    t.integer "artifacts_metadata_store", default: 1, null: false
+    t.integer "artifacts_file_store"
+    t.integer "artifacts_metadata_store"
   end
 
   add_index "ci_builds", ["auto_canceled_by_id"], name: "index_ci_builds_on_auto_canceled_by_id", using: :btree

lib/tasks/gitlab/artifacts.rake

@@ -3,15 +3,18 @@ namespace :gitlab do
   namespace :artifacts do
     task migrate: :environment do
       puts 'Artifacts'.color(:yellow)
-      Ci::Build.joins(:project).with_artifacts
-        .where(artifacts_file_store: ArtifactUploader::LOCAL_STORE)
-        .find_each(batch_size: 100) do |issue|
+      Ci::Build.joins(:project)
+        .with_artifacts
+        .order(id: :asc)
+        .where(artifacts_file_store: [nil, ArtifactUploader::LOCAL_STORE])
+        .find_each(batch_size: 10) do |build|
         begin
+          print "Migrating job #{build.id} of size #{build.artifacts_size.to_i.bytes} to remote storage... "
           build.artifacts_file.migrate!(ArtifactUploader::REMOTE_STORE)
           build.artifacts_metadata.migrate!(ArtifactUploader::REMOTE_STORE)
-          print '.'
-        rescue
-          print 'F'
+          puts "OK".color(:green)
+        rescue => e
+          puts "Failed: #{e.message}".color(:red)
         end
       end
     end

spec/tasks/gitlab/artifacts_rake_spec.rb

+require 'rake_helper'
+
+describe 'gitlab:artifacts namespace rake task' do
+  before :all 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') }
+
+    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)
+        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)
+        end
+      end
+
+      context 'and remote storage is defined' do
+        before 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)
+        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)
+        end
+      end
+    end
+
+    context 'when remote storage is used' do
+      let(:store) { ObjectStoreUploader::REMOTE_STORE }
+
+      before 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)
+      end
+    end
+  end
+end

spec/uploaders/object_store_uploader_spec.rb

@@ -12,6 +12,28 @@ describe ObjectStoreUploader do
 
       uploader.object_store
     end
+
+    context 'when store is null' do
+      before do
+        expect(object).to receive(:artifacts_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(:artifacts_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
@@ -72,6 +94,14 @@ describe ObjectStoreUploader do
       end
     end
 
+    context 'when store is null' do
+      let(:store) { nil }
+
+      it "sets the store to LOCAL_STORE" do
+        expect(job.artifacts_file_store).to eq(described_class::LOCAL_STORE)
+      end
+    end
+
     describe '#use_file' do
       context 'when file is stored locally' do
         let(:store) { described_class::LOCAL_STORE }