diff options
| -rw-r--r-- | app/models/ci/variable.rb | 2 | ||||
| -rw-r--r-- | changelogs/unreleased/add-ci_variables-environment_scope-mysql.yml | 6 | ||||
| -rw-r--r-- | db/migrate/20170622135451_rename_duplicated_variable_key.rb | 35 | ||||
| -rw-r--r-- | db/migrate/20170622135628_add_environment_scope_to_ci_variables.rb | 15 | ||||
| -rw-r--r-- | db/migrate/20170622135728_add_unique_constraint_to_ci_variables.rb | 38 | ||||
| -rw-r--r-- | db/migrate/20170623080805_remove_ci_variables_project_id_index.rb | 19 | ||||
| -rw-r--r-- | db/schema.rb | 5 | ||||
| -rw-r--r-- | spec/migrations/remove_duplicated_variable_spec.rb | 34 | ||||
| -rw-r--r-- | spec/models/ci/variable_spec.rb | 14 |
9 files changed, 159 insertions, 9 deletions
diff --git a/app/models/ci/variable.rb b/app/models/ci/variable.rb index f235260208f..67444118ec5 100644 --- a/app/models/ci/variable.rb +++ b/app/models/ci/variable.rb @@ -6,7 +6,7 @@ module Ci validates :key, presence: true, - uniqueness: { scope: :project_id }, + uniqueness: { scope: [:project_id, :environment_scope] }, length: { maximum: 255 }, format: { with: /\A[a-zA-Z0-9_]+\z/, message: "can contain only letters, digits and '_'." } 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 00000000000..3414f1ed8ca --- /dev/null +++ b/changelogs/unreleased/add-ci_variables-environment_scope-mysql.yml @@ -0,0 +1,6 @@ +--- +title: Remove 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 00000000000..b88e7d7ba81 --- /dev/null +++ b/db/migrate/20170622135451_rename_duplicated_variable_key.rb @@ -0,0 +1,35 @@ +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 00000000000..17fe062d8d5 --- /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 00000000000..8b2cc40ee59 --- /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 00000000000..ddcc0292b9d --- /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 8c7440ee610..993eea1f642 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/remove_duplicated_variable_spec.rb b/spec/migrations/remove_duplicated_variable_spec.rb new file mode 100644 index 00000000000..11096564dfa --- /dev/null +++ b/spec/migrations/remove_duplicated_variable_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 83494af24ba..ad3aa660b96 100644 --- a/spec/models/ci/variable_spec.rb +++ b/spec/models/ci/variable_spec.rb @@ -5,12 +5,14 @@ describe Ci::Variable, models: true do let(:secret_value) { 'secret' } - it { is_expected.to validate_presence_of(:key) } - it { is_expected.to validate_uniqueness_of(:key).scoped_to(:project_id) } - 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) } + describe 'validations' do + it { is_expected.to validate_presence_of(:key) } + 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 } |