Merge branch 'master' into feature/gb/variables-expressions-in-only-except

* master: (199 commits)

Conflicts:
	lib/gitlab/ci/pipeline/seed/stage.rb

12 files changed, 297 insertions, 62 deletions

diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 6081ef04fb7..6a45169c6a8 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -2175,6 +2175,35 @@ describe Ci::Build do
 
         subject.drop!
       end
+
+      context 'when retry service raises Gitlab::Access::AccessDeniedError exception' do
+        let(:retry_service) { Ci::RetryBuildService.new(subject.project, subject.user) }
+
+        before do
+          allow_any_instance_of(Ci::RetryBuildService)
+            .to receive(:execute)
+            .with(subject)
+            .and_raise(Gitlab::Access::AccessDeniedError)
+          allow(Rails.logger).to receive(:error)
+        end
+
+        it 'handles raised exception' do
+          expect { subject.drop! }.not_to raise_exception(Gitlab::Access::AccessDeniedError)
+        end
+
+        it 'logs the error' do
+          subject.drop!
+
+          expect(Rails.logger)
+            .to have_received(:error)
+            .with(a_string_matching("Unable to auto-retry job #{subject.id}"))
+        end
+
+        it 'fails the job' do
+          subject.drop!
+          expect(subject.failed?).to be_truthy
+        end
+      end
     end
 
     context 'when build is not configured to be retried' do
diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb
index 4b217df2e8f..f8874d14e3f 100644
--- a/spec/models/concerns/issuable_spec.rb
+++ b/spec/models/concerns/issuable_spec.rb
@@ -34,7 +34,7 @@ describe Issuable do
     subject { build(:issue) }
 
     before do
-      allow(subject).to receive(:set_iid).and_return(false)
+      allow(InternalId).to receive(:generate_next).and_return(nil)
     end
 
     it { is_expected.to validate_presence_of(:project) }
diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb
index abfc0896a41..d620943693c 100644
--- a/spec/models/group_spec.rb
+++ b/spec/models/group_spec.rb
@@ -240,7 +240,7 @@ describe Group do
 
     it "is false if avatar is html page" do
       group.update_attribute(:avatar, 'uploads/avatar.html')
-      expect(group.avatar_type).to eq(["only images allowed"])
+      expect(group.avatar_type).to eq(["file format is not supported. Please try one of the following supported formats: png, jpg, jpeg, gif, bmp, tiff"])
     end
   end
 
diff --git a/spec/models/internal_id_spec.rb b/spec/models/internal_id_spec.rb
new file mode 100644
index 00000000000..581fd0293cc
--- /dev/null
+++ b/spec/models/internal_id_spec.rb
@@ -0,0 +1,106 @@
+require 'spec_helper'
+
+describe InternalId do
+  let(:project) { create(:project) }
+  let(:usage) { :issues }
+  let(:issue) { build(:issue, project: project) }
+  let(:scope) { { project: project } }
+  let(:init) { ->(s) { s.project.issues.size } }
+
+  context 'validations' do
+    it { is_expected.to validate_presence_of(:usage) }
+  end
+
+  describe '.generate_next' do
+    subject { described_class.generate_next(issue, scope, usage, init) }
+
+    context 'in the absence of a record' do
+      it 'creates a record if not yet present' do
+        expect { subject }.to change { described_class.count }.from(0).to(1)
+      end
+
+      it 'stores record attributes' do
+        subject
+
+        described_class.first.tap do |record|
+          expect(record.project).to eq(project)
+          expect(record.usage).to eq(usage.to_s)
+        end
+      end
+
+      context 'with existing issues' do
+        before do
+          rand(1..10).times { create(:issue, project: project) }
+          described_class.delete_all
+        end
+
+        it 'calculates last_value values automatically' do
+          expect(subject).to eq(project.issues.size + 1)
+        end
+      end
+
+      context 'with concurrent inserts on table' do
+        it 'looks up the record if it was created concurrently' do
+          args = { **scope, usage: described_class.usages[usage.to_s] }
+          record = double
+          expect(described_class).to receive(:find_by).with(args).and_return(nil)    # first call, record not present
+          expect(described_class).to receive(:find_by).with(args).and_return(record) # second call, record was created by another process
+          expect(described_class).to receive(:create!).and_raise(ActiveRecord::RecordNotUnique, 'record not unique')
+          expect(record).to receive(:increment_and_save!)
+
+          subject
+        end
+      end
+    end
+
+    it 'generates a strictly monotone, gapless sequence' do
+      seq = (0..rand(100)).map do
+        described_class.generate_next(issue, scope, usage, init)
+      end
+      normalized = seq.map { |i| i - seq.min }
+
+      expect(normalized).to eq((0..seq.size - 1).to_a)
+    end
+
+    context 'with an insufficient schema version' do
+      before do
+        described_class.reset_column_information
+        expect(ActiveRecord::Migrator).to receive(:current_version).and_return(InternalId::REQUIRED_SCHEMA_VERSION - 1)
+      end
+
+      let(:init) { double('block') }
+
+      it 'calculates next internal ids on the fly' do
+        val = rand(1..100)
+
+        expect(init).to receive(:call).with(issue).and_return(val)
+        expect(subject).to eq(val + 1)
+      end
+    end
+  end
+
+  describe '#increment_and_save!' do
+    let(:id) { create(:internal_id) }
+    subject { id.increment_and_save! }
+
+    it 'returns incremented iid' do
+      value = id.last_value
+
+      expect(subject).to eq(value + 1)
+    end
+
+    it 'saves the record' do
+      subject
+
+      expect(id.changed?).to be_falsey
+    end
+
+    context 'with last_value=nil' do
+      let(:id) { build(:internal_id, last_value: nil) }
+
+      it 'returns 1' do
+        expect(subject).to eq(1)
+      end
+    end
+  end
+end
diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb
index feed7968f09..11154291368 100644
--- a/spec/models/issue_spec.rb
+++ b/spec/models/issue_spec.rb
@@ -9,11 +9,17 @@ describe Issue do
   describe 'modules' do
     subject { described_class }
 
-    it { is_expected.to include_module(InternalId) }
     it { is_expected.to include_module(Issuable) }
     it { is_expected.to include_module(Referable) }
     it { is_expected.to include_module(Sortable) }
     it { is_expected.to include_module(Taskable) }
+
+    it_behaves_like 'AtomicInternalId' do
+      let(:internal_id_attribute) { :iid }
+      let(:instance) { build(:issue) }
+      let(:scope_attrs) { { project: instance.project } }
+      let(:usage) { :issues }
+    end
   end
 
   subject { create(:issue) }
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 4e783acbd8b..ff5a6f63010 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -17,7 +17,7 @@ describe MergeRequest do
   describe 'modules' do
     subject { described_class }
 
-    it { is_expected.to include_module(InternalId) }
+    it { is_expected.to include_module(NonatomicInternalId) }
     it { is_expected.to include_module(Issuable) }
     it { is_expected.to include_module(Referable) }
     it { is_expected.to include_module(Sortable) }
diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb
index ee142718f7e..62e95a622eb 100644
--- a/spec/models/namespace_spec.rb
+++ b/spec/models/namespace_spec.rb
@@ -305,7 +305,7 @@ describe Namespace do
   end
 
   describe '#rm_dir', 'callback' do
-    let(:repository_storage_path) { Gitlab.config.repositories.storages.default['path'] }
+    let(:repository_storage_path) { Gitlab.config.repositories.storages.default.legacy_disk_path }
     let(:path_in_dir) { File.join(repository_storage_path, namespace.full_path) }
     let(:deleted_path) { namespace.full_path.gsub(namespace.path, "#{namespace.full_path}+#{namespace.id}+deleted") }
     let(:deleted_path_in_dir) { File.join(repository_storage_path, deleted_path) }
diff --git a/spec/models/pages_domain_spec.rb b/spec/models/pages_domain_spec.rb
index 95713d8b85b..4b85c5e8720 100644
--- a/spec/models/pages_domain_spec.rb
+++ b/spec/models/pages_domain_spec.rb
@@ -18,24 +18,63 @@ describe PagesDomain do
       it { is_expected.to validate_uniqueness_of(:domain).case_insensitive }
     end
 
-    {
-      'my.domain.com'    => true,
-      '123.456.789'      => true,
-      '0x12345.com'      => true,
-      '0123123'          => true,
-      '_foo.com'         => false,
-      'reserved.com'     => false,
-      'a.reserved.com'   => false,
-      nil                => false
-    }.each do |value, validity|
-      context "domain #{value.inspect} validity" do
-        before do
-          allow(Settings.pages).to receive(:host).and_return('reserved.com')
+    describe "hostname" do
+      {
+        'my.domain.com'    => true,
+        '123.456.789'      => true,
+        '0x12345.com'      => true,
+        '0123123'          => true,
+        '_foo.com'         => false,
+        'reserved.com'     => false,
+        'a.reserved.com'   => false,
+        nil                => false
+      }.each do |value, validity|
+        context "domain #{value.inspect} validity" do
+          before do
+            allow(Settings.pages).to receive(:host).and_return('reserved.com')
+          end
+
+          let(:domain) { value }
+
+          it { expect(pages_domain.valid?).to eq(validity) }
+        end
+      end
+    end
+
+    describe "HTTPS-only" do
+      using RSpec::Parameterized::TableSyntax
+
+      let(:domain) { 'my.domain.com' }
+
+      let(:project) do
+        instance_double(Project, pages_https_only?: pages_https_only)
+      end
+
+      let(:pages_domain) do
+        build(:pages_domain, certificate: certificate, key: key).tap do |pd|
+          allow(pd).to receive(:project).and_return(project)
+          pd.valid?
         end
+      end
 
-        let(:domain) { value }
+      where(:pages_https_only, :certificate, :key, :errors_on) do
+        attributes = attributes_for(:pages_domain)
+        cert, key = attributes.fetch_values(:certificate, :key)
+
+        true  | nil  | nil | %i(certificate key)
+        true  | cert | nil | %i(key)
+        true  | nil  | key | %i(certificate key)
+        true  | cert | key | []
+        false | nil  | nil | []
+        false | cert | nil | %i(key)
+        false | nil  | key | %i(key)
+        false | cert | key | []
+      end
 
-        it { expect(pages_domain.valid?).to eq(validity) }
+      with_them do
+        it "is adds the expected errors" do
+          expect(pages_domain.errors.keys).to eq errors_on
+        end
       end
     end
   end
@@ -43,26 +82,26 @@ describe PagesDomain do
   describe 'validate certificate' do
     subject { domain }
 
-    context 'when only certificate is specified' do
-      let(:domain) { build(:pages_domain, :with_certificate) }
+    context 'with matching key' do
+      let(:domain) { build(:pages_domain) }
 
-      it { is_expected.not_to be_valid }
+      it { is_expected.to be_valid }
     end
 
-    context 'when only key is specified' do
-      let(:domain) { build(:pages_domain, :with_key) }
+    context 'when no certificate is specified' do
+      let(:domain) { build(:pages_domain, :without_certificate) }
 
       it { is_expected.not_to be_valid }
     end
 
-    context 'with matching key' do
-      let(:domain) { build(:pages_domain, :with_certificate, :with_key) }
+    context 'when no key is specified' do
+      let(:domain) { build(:pages_domain, :without_key) }
 
-      it { is_expected.to be_valid }
+      it { is_expected.not_to be_valid }
     end
 
     context 'for not matching key' do
-      let(:domain) { build(:pages_domain, :with_missing_chain, :with_key) }
+      let(:domain) { build(:pages_domain, :with_missing_chain) }
 
       it { is_expected.not_to be_valid }
     end
@@ -103,30 +142,26 @@ describe PagesDomain do
   describe '#url' do
     subject { domain.url }
 
-    context 'without the certificate' do
-      let(:domain) { build(:pages_domain, certificate: '') }
+    let(:domain) { build(:pages_domain) }
 
-      it { is_expected.to eq("http://#{domain.domain}") }
-    end
+    it { is_expected.to eq("https://#{domain.domain}") }
 
-    context 'with a certificate' do
-      let(:domain) { build(:pages_domain, :with_certificate) }
+    context 'without the certificate' do
+      let(:domain) { build(:pages_domain, :without_certificate) }
 
-      it { is_expected.to eq("https://#{domain.domain}") }
+      it { is_expected.to eq("http://#{domain.domain}") }
     end
   end
 
   describe '#has_matching_key?' do
     subject { domain.has_matching_key? }
 
-    context 'for matching key' do
-      let(:domain) { build(:pages_domain, :with_certificate, :with_key) }
+    let(:domain) { build(:pages_domain) }
 
-      it { is_expected.to be_truthy }
-    end
+    it { is_expected.to be_truthy }
 
     context 'for invalid key' do
-      let(:domain) { build(:pages_domain, :with_missing_chain, :with_key) }
+      let(:domain) { build(:pages_domain, :with_missing_chain) }
 
       it { is_expected.to be_falsey }
     end
@@ -136,7 +171,7 @@ describe PagesDomain do
     subject { domain.has_intermediates? }
 
     context 'for self signed' do
-      let(:domain) { build(:pages_domain, :with_certificate) }
+      let(:domain) { build(:pages_domain) }
 
       it { is_expected.to be_truthy }
     end
@@ -162,7 +197,7 @@ describe PagesDomain do
     subject { domain.expired? }
 
     context 'for valid' do
-      let(:domain) { build(:pages_domain, :with_certificate) }
+      let(:domain) { build(:pages_domain) }
 
       it { is_expected.to be_falsey }
     end
@@ -175,7 +210,7 @@ describe PagesDomain do
   end
 
   describe '#subject' do
-    let(:domain) { build(:pages_domain, :with_certificate) }
+    let(:domain) { build(:pages_domain) }
 
     subject { domain.subject }
 
@@ -183,7 +218,7 @@ describe PagesDomain do
   end
 
   describe '#certificate_text' do
-    let(:domain) { build(:pages_domain, :with_certificate) }
+    let(:domain) { build(:pages_domain) }
 
     subject { domain.certificate_text }
 
@@ -191,6 +226,18 @@ describe PagesDomain do
     it { is_expected.not_to be_empty }
   end
 
+  describe "#https?" do
+    context "when a certificate is present" do
+      subject { build(:pages_domain) }
+      it { is_expected.to be_https }
+    end
+
+    context "when no certificate is present" do
+      subject { build(:pages_domain, :without_certificate) }
+      it { is_expected.not_to be_https }
+    end
+  end
+
   describe '#update_daemon' do
     it 'runs when the domain is created' do
       domain = build(:pages_domain)
@@ -267,29 +314,30 @@ describe PagesDomain do
       end
 
       context 'TLS configuration' do
-        set(:domain_with_tls) { create(:pages_domain, :with_key, :with_certificate) }
+        set(:domain_without_tls) { create(:pages_domain, :without_certificate, :without_key) }
+        set(:domain) { create(:pages_domain) }
 
-        let(:cert1) { domain_with_tls.certificate }
+        let(:cert1) { domain.certificate }
         let(:cert2) { cert1 + ' ' }
-        let(:key1) { domain_with_tls.key }
+        let(:key1) { domain.key }
         let(:key2) { key1 + ' ' }
 
         it 'updates when added' do
-          expect(domain).to receive(:update_daemon)
+          expect(domain_without_tls).to receive(:update_daemon)
 
-          domain.update!(key: key1, certificate: cert1)
+          domain_without_tls.update!(key: key1, certificate: cert1)
         end
 
         it 'updates when changed' do
-          expect(domain_with_tls).to receive(:update_daemon)
+          expect(domain).to receive(:update_daemon)
 
-          domain_with_tls.update!(key: key2, certificate: cert2)
+          domain.update!(key: key2, certificate: cert2)
         end
 
         it 'updates when removed' do
-          expect(domain_with_tls).to receive(:update_daemon)
+          expect(domain).to receive(:update_daemon)
 
-          domain_with_tls.update!(key: nil, certificate: nil)
+          domain.update!(key: nil, certificate: nil)
         end
       end
     end
diff --git a/spec/models/project_services/mattermost_slash_commands_service_spec.rb b/spec/models/project_services/mattermost_slash_commands_service_spec.rb
index a5bdf9a9337..05d33cd3874 100644
--- a/spec/models/project_services/mattermost_slash_commands_service_spec.rb
+++ b/spec/models/project_services/mattermost_slash_commands_service_spec.rb
@@ -9,10 +9,11 @@ describe MattermostSlashCommandsService do
     let(:user) { create(:user) }
 
     before do
-      Mattermost::Session.base_uri("http://mattermost.example.com")
+      session = Mattermost::Session.new(nil)
+      session.base_uri = 'http://mattermost.example.com'
 
       allow_any_instance_of(Mattermost::Client).to receive(:with_session)
-        .and_yield(Mattermost::Session.new(nil))
+        .and_yield(session)
     end
 
     describe '#configure' do
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 4cf8d861595..1a7a6e035ea 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -922,7 +922,7 @@ describe Project do
 
     it 'is false if avatar is html page' do
       project.update_attribute(:avatar, 'uploads/avatar.html')
-      expect(project.avatar_type).to eq(['only images allowed'])
+      expect(project.avatar_type).to eq(['file format is not supported. Please try one of the following supported formats: png, jpg, jpeg, gif, bmp, tiff'])
     end
   end
 
@@ -1101,8 +1101,8 @@ describe Project do
 
     before do
       storages = {
-        'default' => { 'path' => 'tmp/tests/repositories' },
-        'picked'  => { 'path' => 'tmp/tests/repositories' }
+        'default' => Gitlab::GitalyClient::StorageSettings.new('path' => 'tmp/tests/repositories'),
+        'picked'  => Gitlab::GitalyClient::StorageSettings.new('path' => 'tmp/tests/repositories')
       }
       allow(Gitlab.config.repositories).to receive(:storages).and_return(storages)
     end
@@ -3479,4 +3479,49 @@ describe Project do
       end
     end
   end
+
+  describe "#pages_https_only?" do
+    subject { build(:project) }
+
+    context "when HTTPS pages are disabled" do
+      it { is_expected.not_to be_pages_https_only }
+    end
+
+    context "when HTTPS pages are enabled", :https_pages_enabled do
+      it { is_expected.to be_pages_https_only }
+    end
+  end
+
+  describe "#pages_https_only? validation", :https_pages_enabled do
+    subject(:project) do
+      # set-up dirty object:
+      create(:project, pages_https_only: false).tap do |p|
+        p.pages_https_only = true
+      end
+    end
+
+    context "when no domains are associated" do
+      it { is_expected.to be_valid }
+    end
+
+    context "when domains including keys and certificates are associated" do
+      before do
+        allow(project)
+          .to receive(:pages_domains)
+          .and_return([instance_double(PagesDomain, https?: true)])
+      end
+
+      it { is_expected.to be_valid }
+    end
+
+    context "when domains including no keys or certificates are associated" do
+      before do
+        allow(project)
+          .to receive(:pages_domains)
+          .and_return([instance_double(PagesDomain, https?: false)])
+      end
+
+      it { is_expected.not_to be_valid }
+    end
+  end
 end
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index 5bc972bca14..e506c932d58 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -895,7 +895,7 @@ describe Repository do
     end
 
     it 'returns nil when the content is not recognizable' do
-      repository.create_file(user, 'LICENSE', 'Copyright!',
+      repository.create_file(user, 'LICENSE', 'Gitlab B.V.',
         message: 'Add LICENSE', branch_name: 'master')
 
       expect(repository.license_key).to be_nil
@@ -939,7 +939,7 @@ describe Repository do
     end
 
     it 'returns nil when the content is not recognizable' do
-      repository.create_file(user, 'LICENSE', 'Copyright!',
+      repository.create_file(user, 'LICENSE', 'Gitlab B.V.',
         message: 'Add LICENSE', branch_name: 'master')
 
       expect(repository.license).to be_nil
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 5680eb24985..c61674fff13 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -1222,7 +1222,7 @@ describe User do
     it 'is false if avatar is html page' do
       user.update_attribute(:avatar, 'uploads/avatar.html')
 
-      expect(user.avatar_type).to eq(['only images allowed'])
+      expect(user.avatar_type).to eq(['file format is not supported. Please try one of the following supported formats: png, jpg, jpeg, gif, bmp, tiff'])
     end
   end