Add latest changes from gitlab-org/gitlab@master

9 files changed, 234 insertions, 72 deletions

diff --git a/spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb b/spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb
index 1f54f6ec537..216f5d0c77d 100644
--- a/spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb
@@ -6,7 +6,17 @@ require 'support/helpers/stub_feature_flags'
 require_dependency 'active_model'
 
 describe Gitlab::Ci::Config::Entry::Rules::Rule do
-  let(:entry) { described_class.new(config) }
+  let(:factory) do
+    Gitlab::Config::Entry::Factory.new(described_class)
+      .metadata(metadata)
+      .value(config)
+  end
+
+  let(:metadata) do
+    { allowed_when: %w[on_success on_failure always never manual delayed] }
+  end
+
+  let(:entry) { factory.create! }
 
   describe '.new' do
     subject { entry }
@@ -212,6 +222,112 @@ describe Gitlab::Ci::Config::Entry::Rules::Rule do
           .to include(/should be a hash/)
       end
     end
+
+    context 'when: validation' do
+      context 'with an invalid boolean when:' do
+        let(:config) do
+          { if: '$THIS == "that"', when: false }
+        end
+
+        it { is_expected.to be_a(described_class) }
+        it { is_expected.not_to be_valid }
+
+        it 'returns an error about invalid when:' do
+          expect(subject.errors).to include(/when unknown value: false/)
+        end
+
+        context 'when composed' do
+          before do
+            subject.compose!
+          end
+
+          it { is_expected.not_to be_valid }
+
+          it 'returns an error about invalid when:' do
+            expect(subject.errors).to include(/when unknown value: false/)
+          end
+        end
+      end
+
+      context 'with an invalid string when:' do
+        let(:config) do
+          { if: '$THIS == "that"', when: 'explode' }
+        end
+
+        it { is_expected.to be_a(described_class) }
+        it { is_expected.not_to be_valid }
+
+        it 'returns an error about invalid when:' do
+          expect(subject.errors).to include(/when unknown value: explode/)
+        end
+
+        context 'when composed' do
+          before do
+            subject.compose!
+          end
+
+          it { is_expected.not_to be_valid }
+
+          it 'returns an error about invalid when:' do
+            expect(subject.errors).to include(/when unknown value: explode/)
+          end
+        end
+      end
+
+      context 'with a string passed in metadata but not allowed in the class' do
+        let(:metadata) { { allowed_when: %w[explode] } }
+
+        let(:config) do
+          { if: '$THIS == "that"', when: 'explode' }
+        end
+
+        it { is_expected.to be_a(described_class) }
+        it { is_expected.not_to be_valid }
+
+        it 'returns an error about invalid when:' do
+          expect(subject.errors).to include(/when unknown value: explode/)
+        end
+
+        context 'when composed' do
+          before do
+            subject.compose!
+          end
+
+          it { is_expected.not_to be_valid }
+
+          it 'returns an error about invalid when:' do
+            expect(subject.errors).to include(/when unknown value: explode/)
+          end
+        end
+      end
+
+      context 'with a string allowed in the class but not passed in metadata' do
+        let(:metadata) { { allowed_when: %w[always never] } }
+
+        let(:config) do
+          { if: '$THIS == "that"', when: 'on_success' }
+        end
+
+        it { is_expected.to be_a(described_class) }
+        it { is_expected.not_to be_valid }
+
+        it 'returns an error about invalid when:' do
+          expect(subject.errors).to include(/when unknown value: on_success/)
+        end
+
+        context 'when composed' do
+          before do
+            subject.compose!
+          end
+
+          it { is_expected.not_to be_valid }
+
+          it 'returns an error about invalid when:' do
+            expect(subject.errors).to include(/when unknown value: on_success/)
+          end
+        end
+      end
+    end
   end
 
   describe '#value' do
diff --git a/spec/lib/gitlab/ci/config/entry/rules_spec.rb b/spec/lib/gitlab/ci/config/entry/rules_spec.rb
index 926d3fd1678..3c050801023 100644
--- a/spec/lib/gitlab/ci/config/entry/rules_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/rules_spec.rb
@@ -5,7 +5,14 @@ require 'support/helpers/stub_feature_flags'
 require_dependency 'active_model'
 
 describe Gitlab::Ci::Config::Entry::Rules do
-  let(:entry) { described_class.new(config) }
+  let(:factory) do
+    Gitlab::Config::Entry::Factory.new(described_class)
+      .metadata(metadata)
+      .value(config)
+  end
+
+  let(:metadata) { { allowed_when: %w[always never] } }
+  let(:entry)    { factory.create! }
 
   describe '.new' do
     subject { entry }
@@ -18,7 +25,7 @@ describe Gitlab::Ci::Config::Entry::Rules do
       it { is_expected.to be_a(described_class) }
       it { is_expected.to be_valid }
 
-      context 'after #compose!' do
+      context 'when composed' do
         before do
           subject.compose!
         end
@@ -38,7 +45,7 @@ describe Gitlab::Ci::Config::Entry::Rules do
       it { is_expected.to be_a(described_class) }
       it { is_expected.to be_valid }
 
-      context 'after #compose!' do
+      context 'when composed' do
         before do
           subject.compose!
         end
@@ -54,48 +61,6 @@ describe Gitlab::Ci::Config::Entry::Rules do
 
       it { is_expected.not_to be_valid }
     end
-
-    context 'with an invalid boolean when:' do
-      let(:config) do
-        [{ if: '$THIS == "that"', when: false }]
-      end
-
-      it { is_expected.to be_a(described_class) }
-      it { is_expected.to be_valid }
-
-      context 'after #compose!' do
-        before do
-          subject.compose!
-        end
-
-        it { is_expected.not_to be_valid }
-
-        it 'returns an error about invalid when:' do
-          expect(subject.errors).to include(/when unknown value: false/)
-        end
-      end
-    end
-
-    context 'with an invalid string when:' do
-      let(:config) do
-        [{ if: '$THIS == "that"', when: 'explode' }]
-      end
-
-      it { is_expected.to be_a(described_class) }
-      it { is_expected.to be_valid }
-
-      context 'after #compose!' do
-        before do
-          subject.compose!
-        end
-
-        it { is_expected.not_to be_valid }
-
-        it 'returns an error about invalid when:' do
-          expect(subject.errors).to include(/when unknown value: explode/)
-        end
-      end
-    end
   end
 
   describe '#value' do
diff --git a/spec/services/projects/hashed_storage/base_attachment_service_spec.rb b/spec/services/projects/hashed_storage/base_attachment_service_spec.rb
new file mode 100644
index 00000000000..34c37be6703
--- /dev/null
+++ b/spec/services/projects/hashed_storage/base_attachment_service_spec.rb
@@ -0,0 +1,56 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Projects::HashedStorage::BaseAttachmentService do
+  let(:project) { create(:project, :repository, storage_version: 0, skip_disk_validation: true) }
+
+  subject(:service) { described_class.new(project: project, old_disk_path: project.full_path, logger: nil) }
+
+  describe '#old_disk_path' do
+    it { is_expected.to respond_to :old_disk_path }
+  end
+
+  describe '#new_disk_path' do
+    it { is_expected.to respond_to :new_disk_path }
+  end
+
+  describe '#skipped?' do
+    it { is_expected.to respond_to :skipped? }
+  end
+
+  describe '#target_path_discardable?' do
+    it 'returns false' do
+      expect(subject.target_path_discardable?('something/something')).to be_falsey
+    end
+  end
+
+  describe '#discard_path!' do
+    it 'renames target path adding a timestamp at the end' do
+      target_path = Dir.mktmpdir
+      expect(Dir.exist?(target_path)).to be_truthy
+
+      Timecop.freeze do
+        suffix = Time.now.utc.to_i
+        subject.send(:discard_path!, target_path)
+
+        expected_renamed_path = "#{target_path}-#{suffix}"
+
+        expect(Dir.exist?(target_path)).to be_falsey
+        expect(Dir.exist?(expected_renamed_path)).to be_truthy
+      end
+    end
+  end
+
+  describe '#move_folder!' do
+    context 'when old_path is not a directory' do
+      it 'adds information to the logger and returns true' do
+        Tempfile.create do |old_path|
+          new_path = "#{old_path}-new"
+
+          expect(subject.send(:move_folder!, old_path, new_path)).to be_truthy
+        end
+      end
+    end
+  end
+end
diff --git a/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb b/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb
index 32ebec318f2..ab9d2bdba8f 100644
--- a/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb
+++ b/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb
@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 describe Projects::HashedStorage::MigrateAttachmentsService do
-  subject(:service) { described_class.new(project, project.full_path, logger: nil) }
+  subject(:service) { described_class.new(project: project, old_disk_path: project.full_path, logger: nil) }
 
   let(:project) { create(:project, :repository, storage_version: 1, skip_disk_validation: true) }
   let(:legacy_storage) { Storage::LegacyProject.new(project) }
@@ -72,7 +72,23 @@ describe Projects::HashedStorage::MigrateAttachmentsService do
         FileUtils.mkdir_p(base_path(hashed_storage))
       end
 
-      it 'raises AttachmentCannotMoveError' do
+      it 'succeed when target is empty' do
+        expect { service.execute }.not_to raise_error
+      end
+
+      it 'succeed when target include only discardable items' do
+        Projects::HashedStorage::MigrateAttachmentsService::DISCARDABLE_PATHS.each do |path_fragment|
+          discardable_path = File.join(base_path(hashed_storage), path_fragment)
+          FileUtils.mkdir_p(discardable_path)
+        end
+
+        expect { service.execute }.not_to raise_error
+      end
+
+      it 'raises AttachmentCannotMoveError when there are non discardable items on target path' do
+        not_discardable_path = File.join(base_path(hashed_storage), 'something')
+        FileUtils.mkdir_p(not_discardable_path)
+
         expect(FileUtils).not_to receive(:mv).with(base_path(legacy_storage), base_path(hashed_storage))
 
         expect { service.execute }.to raise_error(Projects::HashedStorage::AttachmentCannotMoveError)
@@ -100,6 +116,18 @@ describe Projects::HashedStorage::MigrateAttachmentsService do
     end
   end
 
+  context '#target_path_discardable?' do
+    it 'returns true when it include only items on the discardable list' do
+      hashed_attachments_path = File.join(base_path(hashed_storage))
+      Projects::HashedStorage::MigrateAttachmentsService::DISCARDABLE_PATHS.each do |path_fragment|
+        discardable_path = File.join(hashed_attachments_path, path_fragment)
+        FileUtils.mkdir_p(discardable_path)
+      end
+
+      expect(service.target_path_discardable?(hashed_attachments_path)).to be_truthy
+    end
+  end
+
   def base_path(storage)
     File.join(FileUploader.root, storage.disk_path)
   end
diff --git a/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb b/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb
index 70785c606a5..132b895fc35 100644
--- a/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb
+++ b/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb
@@ -10,7 +10,7 @@ describe Projects::HashedStorage::MigrateRepositoryService do
   let(:legacy_storage) { Storage::LegacyProject.new(project) }
   let(:hashed_storage) { Storage::HashedProject.new(project) }
 
-  subject(:service) { described_class.new(project, project.disk_path) }
+  subject(:service) { described_class.new(project: project, old_disk_path: project.disk_path) }
 
   describe '#execute' do
     let(:old_disk_path) { legacy_storage.disk_path }
diff --git a/spec/services/projects/hashed_storage/migration_service_spec.rb b/spec/services/projects/hashed_storage/migration_service_spec.rb
index e3191cd7ebc..f3ac26e7761 100644
--- a/spec/services/projects/hashed_storage/migration_service_spec.rb
+++ b/spec/services/projects/hashed_storage/migration_service_spec.rb
@@ -10,13 +10,14 @@ describe Projects::HashedStorage::MigrationService do
 
   describe '#execute' do
     context 'repository migration' do
-      let(:repository_service) { Projects::HashedStorage::MigrateRepositoryService.new(project, project.full_path, logger: logger) }
+      let(:repository_service) do
+        Projects::HashedStorage::MigrateRepositoryService.new(project: project,
+                                                              old_disk_path: project.full_path,
+                                                              logger: logger)
+      end
 
       it 'delegates migration to Projects::HashedStorage::MigrateRepositoryService' do
-        expect(Projects::HashedStorage::MigrateRepositoryService)
-          .to receive(:new)
-                .with(project, project.full_path, logger: logger)
-                .and_return(repository_service)
+        expect(service).to receive(:migrate_repository_service).and_return(repository_service)
         expect(repository_service).to receive(:execute)
 
         service.execute
@@ -31,13 +32,14 @@ describe Projects::HashedStorage::MigrationService do
     end
 
     context 'attachments migration' do
-      let(:attachments_service) { Projects::HashedStorage::MigrateAttachmentsService.new(project, project.full_path, logger: logger) }
+      let(:attachments_service) do
+        Projects::HashedStorage::MigrateAttachmentsService.new(project: project,
+                                                               old_disk_path: project.full_path,
+                                                               logger: logger)
+      end
 
       it 'delegates migration to Projects::HashedStorage::MigrateRepositoryService' do
-        expect(Projects::HashedStorage::MigrateAttachmentsService)
-          .to receive(:new)
-                .with(project, project.full_path, logger: logger)
-                .and_return(attachments_service)
+        expect(service).to receive(:migrate_attachments_service).and_return(attachments_service)
         expect(attachments_service).to receive(:execute)
 
         service.execute
diff --git a/spec/services/projects/hashed_storage/rollback_attachments_service_spec.rb b/spec/services/projects/hashed_storage/rollback_attachments_service_spec.rb
index 815c85e0866..c2ba9626f41 100644
--- a/spec/services/projects/hashed_storage/rollback_attachments_service_spec.rb
+++ b/spec/services/projects/hashed_storage/rollback_attachments_service_spec.rb
@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 describe Projects::HashedStorage::RollbackAttachmentsService do
-  subject(:service) { described_class.new(project, logger: nil) }
+  subject(:service) { described_class.new(project: project, old_disk_path: project.disk_path, logger: nil) }
 
   let(:project) { create(:project, :repository, skip_disk_validation: true) }
   let(:legacy_storage) { Storage::LegacyProject.new(project) }
diff --git a/spec/services/projects/hashed_storage/rollback_repository_service_spec.rb b/spec/services/projects/hashed_storage/rollback_repository_service_spec.rb
index 3ca9ee5bee5..97c7c0af946 100644
--- a/spec/services/projects/hashed_storage/rollback_repository_service_spec.rb
+++ b/spec/services/projects/hashed_storage/rollback_repository_service_spec.rb
@@ -10,7 +10,7 @@ describe Projects::HashedStorage::RollbackRepositoryService, :clean_gitlab_redis
   let(:legacy_storage) { Storage::LegacyProject.new(project) }
   let(:hashed_storage) { Storage::HashedProject.new(project) }
 
-  subject(:service) { described_class.new(project, project.disk_path) }
+  subject(:service) { described_class.new(project: project, old_disk_path: project.disk_path) }
 
   describe '#execute' do
     let(:old_disk_path) { hashed_storage.disk_path }
diff --git a/spec/services/projects/hashed_storage/rollback_service_spec.rb b/spec/services/projects/hashed_storage/rollback_service_spec.rb
index 427d1535559..48d4eac9eb7 100644
--- a/spec/services/projects/hashed_storage/rollback_service_spec.rb
+++ b/spec/services/projects/hashed_storage/rollback_service_spec.rb
@@ -6,17 +6,15 @@ describe Projects::HashedStorage::RollbackService do
   let(:project) { create(:project, :empty_repo, :wiki_repo) }
   let(:logger) { double }
 
-  subject(:service) { described_class.new(project, project.full_path, logger: logger) }
+  subject(:service) { described_class.new(project, project.disk_path, logger: logger) }
 
   describe '#execute' do
     context 'attachments rollback' do
       let(:attachments_service_class) { Projects::HashedStorage::RollbackAttachmentsService }
-      let(:attachments_service) { attachments_service_class.new(project, logger: logger) }
+      let(:attachments_service) { attachments_service_class.new(project: project, old_disk_path: project.disk_path, logger: logger) }
 
       it 'delegates rollback to Projects::HashedStorage::RollbackAttachmentsService' do
-        expect(attachments_service_class).to receive(:new)
-            .with(project, logger: logger)
-            .and_return(attachments_service)
+        expect(service).to receive(:rollback_attachments_service).and_return(attachments_service)
         expect(attachments_service).to receive(:execute)
 
         service.execute
@@ -31,15 +29,12 @@ describe Projects::HashedStorage::RollbackService do
     end
 
     context 'repository rollback' do
+      let(:project) { create(:project, :empty_repo, :wiki_repo, storage_version: ::Project::HASHED_STORAGE_FEATURES[:repository]) }
       let(:repository_service_class) { Projects::HashedStorage::RollbackRepositoryService }
-      let(:repository_service) { repository_service_class.new(project, project.full_path, logger: logger) }
+      let(:repository_service) { repository_service_class.new(project: project, old_disk_path: project.disk_path, logger: logger) }
 
       it 'delegates rollback to RollbackRepositoryService' do
-        project.storage_version = ::Project::HASHED_STORAGE_FEATURES[:repository]
-
-        expect(repository_service_class).to receive(:new)
-            .with(project, project.full_path, logger: logger)
-            .and_return(repository_service)
+        expect(service).to receive(:rollback_repository_service).and_return(repository_service)
         expect(repository_service).to receive(:execute)
 
         service.execute