Merge branch '10-5-stable-prepare-rc7' into '10-5-stable'

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
 

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
 

app/validators/variable_duplicates_validator.rb

 # 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

changelogs/unreleased/fix-validation-of-environment-scope-for-variables.yml

+---
+title: Fix validation of environment scope of variables
+merge_request:
+author:
+type: fixed

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
 }

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,

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

spec/validators/variable_duplicates_validator_spec.rb

+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