diff options
author | Marin Jankovski <marin@gitlab.com> | 2018-02-15 12:46:27 +0000 |
---|---|---|
committer | Marin Jankovski <marin@gitlab.com> | 2018-02-15 12:46:27 +0000 |
commit | 3eaabc0a24566e02db3bd40141caef59e2139ec7 (patch) | |
tree | 65e04099e84ee8b39735e82350858bb2cac0c693 | |
parent | 80daee8fa82c24c957e7f56987d0b2503e5dc672 (diff) | |
parent | 424684554758a24914098b20300d5908235d7949 (diff) | |
download | gitlab-ce-3eaabc0a24566e02db3bd40141caef59e2139ec7.tar.gz |
Merge branch '10-5-stable-prepare-rc7' into '10-5-stable'
Prepare 10.5 RC7 release
See merge request gitlab-org/gitlab-ce!17131
-rw-r--r-- | app/models/project.rb | 2 | ||||
-rw-r--r-- | app/models/repository.rb | 10 | ||||
-rw-r--r-- | app/validators/variable_duplicates_validator.rb | 21 | ||||
-rw-r--r-- | changelogs/unreleased/fix-validation-of-environment-scope-for-variables.yml | 5 | ||||
-rw-r--r-- | spec/fixtures/api/schemas/variable.json | 3 | ||||
-rw-r--r-- | spec/models/repository_spec.rb | 12 | ||||
-rw-r--r-- | spec/support/features/variable_list_shared_examples.rb | 2 | ||||
-rw-r--r-- | spec/validators/variable_duplicates_validator_spec.rb | 67 |
8 files changed, 115 insertions, 7 deletions
diff --git a/app/models/project.rb b/app/models/project.rb index 3893b1818f3..2ba6a863500 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -261,7 +261,7 @@ class Project < ActiveRecord::Base validates :repository_storage, presence: true, inclusion: { in: ->(_object) { Gitlab.config.repositories.storages.keys } } - validates :variables, variable_duplicates: true + validates :variables, variable_duplicates: { scope: :environment_scope } has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent diff --git a/app/models/repository.rb b/app/models/repository.rb index 1cf55fd4332..4f754b11da4 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -593,7 +593,15 @@ class Repository def license_key return unless exists? - Licensee.license(path).try(:key) + # The licensee gem creates a Rugged object from the path: + # https://github.com/benbalter/licensee/blob/v8.7.0/lib/licensee/projects/git_project.rb + begin + Licensee.license(path).try(:key) + # Normally we would rescue Rugged::Error, but that is banned by lint-rugged + # and we need to migrate this endpoint to Gitaly: + # https://gitlab.com/gitlab-org/gitaly/issues/1026 + rescue + end end cache_method :license_key diff --git a/app/validators/variable_duplicates_validator.rb b/app/validators/variable_duplicates_validator.rb index 8a9d8892e9b..4bfa3c45303 100644 --- a/app/validators/variable_duplicates_validator.rb +++ b/app/validators/variable_duplicates_validator.rb @@ -1,13 +1,28 @@ # VariableDuplicatesValidator # -# This validtor is designed for especially the following condition +# This validator is designed for especially the following condition # - Use `accepts_nested_attributes_for :xxx` in a parent model # - Use `validates :xxx, uniqueness: { scope: :xxx_id }` in a child model class VariableDuplicatesValidator < ActiveModel::EachValidator def validate_each(record, attribute, value) - duplicates = value.reject(&:marked_for_destruction?).group_by(&:key).select { |_, v| v.many? }.map(&:first) + if options[:scope] + scoped = value.group_by do |variable| + Array(options[:scope]).map { |attr| variable.send(attr) } # rubocop:disable GitlabSecurity/PublicSend + end + scoped.each_value { |scope| validate_duplicates(record, attribute, scope) } + else + validate_duplicates(record, attribute, value) + end + end + + private + + def validate_duplicates(record, attribute, values) + duplicates = values.reject(&:marked_for_destruction?).group_by(&:key).select { |_, v| v.many? }.map(&:first) if duplicates.any? - record.errors.add(attribute, "Duplicate variables: #{duplicates.join(", ")}") + error_message = "have duplicate values (#{duplicates.join(", ")})" + error_message += " for #{values.first.send(options[:scope])} scope" if options[:scope] # rubocop:disable GitlabSecurity/PublicSend + record.errors.add(attribute, error_message) end end end diff --git a/changelogs/unreleased/fix-validation-of-environment-scope-for-variables.yml b/changelogs/unreleased/fix-validation-of-environment-scope-for-variables.yml new file mode 100644 index 00000000000..5424c15a8ae --- /dev/null +++ b/changelogs/unreleased/fix-validation-of-environment-scope-for-variables.yml @@ -0,0 +1,5 @@ +--- +title: Fix validation of environment scope of variables +merge_request: +author: +type: fixed diff --git a/spec/fixtures/api/schemas/variable.json b/spec/fixtures/api/schemas/variable.json index 78977118b0a..6f6b044115b 100644 --- a/spec/fixtures/api/schemas/variable.json +++ b/spec/fixtures/api/schemas/variable.json @@ -10,7 +10,8 @@ "id": { "type": "integer" }, "key": { "type": "string" }, "value": { "type": "string" }, - "protected": { "type": "boolean" } + "protected": { "type": "boolean" }, + "environment_scope": { "type": "string", "optional": true } }, "additionalProperties": false } diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index a6d48e369ac..0bc07dc7a85 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -873,6 +873,18 @@ describe Repository do expect(repository.license_key).to be_nil end + it 'returns nil when the commit SHA does not exist' do + allow(repository.head_commit).to receive(:sha).and_return('1' * 40) + + expect(repository.license_key).to be_nil + end + + it 'returns nil when master does not exist' do + repository.rm_branch(user, 'master') + + expect(repository.license_key).to be_nil + end + it 'returns the license key' do repository.create_file(user, 'LICENSE', Licensee::License.new('mit').content, diff --git a/spec/support/features/variable_list_shared_examples.rb b/spec/support/features/variable_list_shared_examples.rb index 4315bf5d037..0d8f7a7aae6 100644 --- a/spec/support/features/variable_list_shared_examples.rb +++ b/spec/support/features/variable_list_shared_examples.rb @@ -263,7 +263,7 @@ shared_examples 'variable list' do # We check the first row because it re-sorts to alphabetical order on refresh page.within('.js-ci-variable-list-section') do - expect(find('.js-ci-variable-error-box')).to have_content('Validation failed Variables Duplicate variables: samekey') + expect(find('.js-ci-variable-error-box')).to have_content(/Validation failed Variables have duplicate values \(.+\)/) end end end diff --git a/spec/validators/variable_duplicates_validator_spec.rb b/spec/validators/variable_duplicates_validator_spec.rb new file mode 100644 index 00000000000..0b71a67f94d --- /dev/null +++ b/spec/validators/variable_duplicates_validator_spec.rb @@ -0,0 +1,67 @@ +require 'spec_helper' + +describe VariableDuplicatesValidator do + let(:validator) { described_class.new(attributes: [:variables], **options) } + + describe '#validate_each' do + let(:project) { build(:project) } + + subject { validator.validate_each(project, :variables, project.variables) } + + context 'with no scope' do + let(:options) { {} } + let(:variables) { build_list(:ci_variable, 2, project: project) } + + before do + project.variables << variables + end + + it 'does not have any errors' do + subject + + expect(project.errors.empty?).to be true + end + + context 'with duplicates' do + before do + project.variables.build(key: variables.first.key, value: 'dummy_value') + end + + it 'has a duplicate key error' do + subject + + expect(project.errors).to have_key(:variables) + end + end + end + + context 'with a scope attribute' do + let(:options) { { scope: :environment_scope } } + let(:first_variable) { build(:ci_variable, key: 'test_key', environment_scope: '*', project: project) } + let(:second_variable) { build(:ci_variable, key: 'test_key', environment_scope: 'prod', project: project) } + + before do + project.variables << first_variable + project.variables << second_variable + end + + it 'does not have any errors' do + subject + + expect(project.errors.empty?).to be true + end + + context 'with duplicates' do + before do + project.variables.build(key: second_variable.key, value: 'dummy_value', environment_scope: second_variable.environment_scope) + end + + it 'has a duplicate key error' do + subject + + expect(project.errors).to have_key(:variables) + end + end + end + end +end |