diff --git a/app/models/ci/variable.rb b/app/models/ci/variable.rb
index 96d6e120998077f720495cce6ad05701081b2003..0b8d0ff881a2b252f4bdc888153fde376431b74e 100644
--- a/app/models/ci/variable.rb
+++ b/app/models/ci/variable.rb
@@ -5,7 +5,7 @@ module Ci
belongs_to :project
- validates :key, uniqueness: { scope: :project_id }
+ validates :key, uniqueness: { scope: [:project_id, :environment_scope] }
scope :unprotected, -> { where(protected: false) }
end
diff --git a/changelogs/unreleased/add-ci_variables-environment_scope-mysql.yml b/changelogs/unreleased/add-ci_variables-environment_scope-mysql.yml
new file mode 100644
index 0000000000000000000000000000000000000000..4948d415bed5001ce4c0c7509c6d01e1c06f94e2
--- /dev/null
+++ b/changelogs/unreleased/add-ci_variables-environment_scope-mysql.yml
@@ -0,0 +1,6 @@
+---
+title: Rename duplicated variables with the same key for projects. Add environment_scope
+ column to variables and add unique constraint to make sure that no variables could
+ be created with the same key within a project
+merge_request: 12363
+author:
diff --git a/db/migrate/20170622135451_rename_duplicated_variable_key.rb b/db/migrate/20170622135451_rename_duplicated_variable_key.rb
new file mode 100644
index 0000000000000000000000000000000000000000..368718ab0ce787d95f66894ea053e04006745156
--- /dev/null
+++ b/db/migrate/20170622135451_rename_duplicated_variable_key.rb
@@ -0,0 +1,38 @@
+class RenameDuplicatedVariableKey < ActiveRecord::Migration
+ include Gitlab::Database::MigrationHelpers
+
+ DOWNTIME = false
+
+ disable_ddl_transaction!
+
+ def up
+ execute(<<~SQL)
+ UPDATE ci_variables
+ SET #{key} = CONCAT(#{key}, #{underscore}, id)
+ WHERE id IN (
+ SELECT *
+ FROM ( -- MySQL requires an extra layer
+ SELECT dup.id
+ FROM ci_variables dup
+ INNER JOIN (SELECT max(id) AS id, #{key}, project_id
+ FROM ci_variables tmp
+ GROUP BY #{key}, project_id) var
+ USING (#{key}, project_id) where dup.id <> var.id
+ ) dummy
+ )
+ SQL
+ end
+
+ def down
+ # noop
+ end
+
+ def key
+ # key needs to be quoted in MySQL
+ quote_column_name('key')
+ end
+
+ def underscore
+ quote('_')
+ end
+end
diff --git a/db/migrate/20170622135628_add_environment_scope_to_ci_variables.rb b/db/migrate/20170622135628_add_environment_scope_to_ci_variables.rb
new file mode 100644
index 0000000000000000000000000000000000000000..17fe062d8d59272a3ebeb6857541183d65bb1029
--- /dev/null
+++ b/db/migrate/20170622135628_add_environment_scope_to_ci_variables.rb
@@ -0,0 +1,15 @@
+class AddEnvironmentScopeToCiVariables < ActiveRecord::Migration
+ include Gitlab::Database::MigrationHelpers
+
+ DOWNTIME = false
+
+ disable_ddl_transaction!
+
+ def up
+ add_column_with_default(:ci_variables, :environment_scope, :string, default: '*')
+ end
+
+ def down
+ remove_column(:ci_variables, :environment_scope)
+ end
+end
diff --git a/db/migrate/20170622135728_add_unique_constraint_to_ci_variables.rb b/db/migrate/20170622135728_add_unique_constraint_to_ci_variables.rb
new file mode 100644
index 0000000000000000000000000000000000000000..8b2cc40ee5929655e7020358d956ce2c62459d1b
--- /dev/null
+++ b/db/migrate/20170622135728_add_unique_constraint_to_ci_variables.rb
@@ -0,0 +1,38 @@
+class AddUniqueConstraintToCiVariables < ActiveRecord::Migration
+ include Gitlab::Database::MigrationHelpers
+
+ DOWNTIME = false
+
+ disable_ddl_transaction!
+
+ def up
+ unless this_index_exists?
+ add_concurrent_index(:ci_variables, columns, name: index_name, unique: true)
+ end
+ end
+
+ def down
+ if this_index_exists?
+ if Gitlab::Database.mysql? && !index_exists?(:ci_variables, :project_id)
+ # Need to add this index for MySQL project_id foreign key constraint
+ add_concurrent_index(:ci_variables, :project_id)
+ end
+
+ remove_concurrent_index(:ci_variables, columns, name: index_name)
+ end
+ end
+
+ private
+
+ def this_index_exists?
+ index_exists?(:ci_variables, columns, name: index_name)
+ end
+
+ def columns
+ @columns ||= [:project_id, :key, :environment_scope]
+ end
+
+ def index_name
+ 'index_ci_variables_on_project_id_and_key_and_environment_scope'
+ end
+end
diff --git a/db/migrate/20170623080805_remove_ci_variables_project_id_index.rb b/db/migrate/20170623080805_remove_ci_variables_project_id_index.rb
new file mode 100644
index 0000000000000000000000000000000000000000..ddcc0292b9d66dad359a5ef1efe53d7e2fc7cf46
--- /dev/null
+++ b/db/migrate/20170623080805_remove_ci_variables_project_id_index.rb
@@ -0,0 +1,19 @@
+class RemoveCiVariablesProjectIdIndex < ActiveRecord::Migration
+ include Gitlab::Database::MigrationHelpers
+
+ DOWNTIME = false
+
+ disable_ddl_transaction!
+
+ def up
+ if index_exists?(:ci_variables, :project_id)
+ remove_concurrent_index(:ci_variables, :project_id)
+ end
+ end
+
+ def down
+ unless index_exists?(:ci_variables, :project_id)
+ add_concurrent_index(:ci_variables, :project_id)
+ end
+ end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 8c7440ee6102f035b7db57f52d331a69b08dcc33..993eea1f642e1957b71fb10009d8a7978d067d17 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -11,7 +11,7 @@
#
# It's strongly recommended that you check this file into your version control system.
-ActiveRecord::Schema.define(version: 20170622162730) do
+ActiveRecord::Schema.define(version: 20170623080805) do
# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"
@@ -374,9 +374,10 @@ ActiveRecord::Schema.define(version: 20170622162730) do
t.string "encrypted_value_iv"
t.integer "project_id", null: false
t.boolean "protected", default: false, null: false
+ t.string "environment_scope", default: "*", null: false
end
- add_index "ci_variables", ["project_id"], name: "index_ci_variables_on_project_id", using: :btree
+ add_index "ci_variables", ["project_id", "key", "environment_scope"], name: "index_ci_variables_on_project_id_and_key_and_environment_scope", unique: true, using: :btree
create_table "container_repositories", force: :cascade do |t|
t.integer "project_id", null: false
diff --git a/spec/migrations/rename_duplicated_variable_key_spec.rb b/spec/migrations/rename_duplicated_variable_key_spec.rb
new file mode 100644
index 0000000000000000000000000000000000000000..11096564dfa7d5763afc9f917f172942ef0686ee
--- /dev/null
+++ b/spec/migrations/rename_duplicated_variable_key_spec.rb
@@ -0,0 +1,34 @@
+require 'spec_helper'
+require Rails.root.join('db', 'migrate', '20170622135451_rename_duplicated_variable_key.rb')
+
+describe RenameDuplicatedVariableKey, :migration do
+ let(:variables) { table(:ci_variables) }
+ let(:projects) { table(:projects) }
+
+ before do
+ projects.create!(id: 1)
+ variables.create!(id: 1, key: 'key1', project_id: 1)
+ variables.create!(id: 2, key: 'key2', project_id: 1)
+ variables.create!(id: 3, key: 'keyX', project_id: 1)
+ variables.create!(id: 4, key: 'keyX', project_id: 1)
+ variables.create!(id: 5, key: 'keyY', project_id: 1)
+ variables.create!(id: 6, key: 'keyX', project_id: 1)
+ variables.create!(id: 7, key: 'key7', project_id: 1)
+ variables.create!(id: 8, key: 'keyY', project_id: 1)
+ end
+
+ it 'correctly remove duplicated records with smaller id' do
+ migrate!
+
+ expect(variables.pluck(:id, :key)).to contain_exactly(
+ [1, 'key1'],
+ [2, 'key2'],
+ [3, 'keyX_3'],
+ [4, 'keyX_4'],
+ [5, 'keyY_5'],
+ [6, 'keyX'],
+ [7, 'key7'],
+ [8, 'keyY']
+ )
+ end
+end
diff --git a/spec/models/ci/variable_spec.rb b/spec/models/ci/variable_spec.rb
index 329682a0771f05fde56277506fe248cf533f9468..50f7c029af8902e54c847204f04d6c39c510c857 100644
--- a/spec/models/ci/variable_spec.rb
+++ b/spec/models/ci/variable_spec.rb
@@ -3,8 +3,16 @@ require 'spec_helper'
describe Ci::Variable, models: true do
subject { build(:ci_variable) }
- it { is_expected.to include_module(HasVariable) }
- it { is_expected.to validate_uniqueness_of(:key).scoped_to(:project_id) }
+ let(:secret_value) { 'secret' }
+
+ describe 'validations' do
+ it { is_expected.to include_module(HasVariable) }
+ it { is_expected.to validate_uniqueness_of(:key).scoped_to(:project_id, :environment_scope) }
+ it { is_expected.to validate_length_of(:key).is_at_most(255) }
+ it { is_expected.to allow_value('foo').for(:key) }
+ it { is_expected.not_to allow_value('foo bar').for(:key) }
+ it { is_expected.not_to allow_value('foo/bar').for(:key) }
+ end
describe '.unprotected' do
subject { described_class.unprotected }