diff options
| -rw-r--r-- | app/models/environment.rb | 50 | ||||
| -rw-r--r-- | changelogs/unreleased/22864-add-environment-slug.yml | 4 | ||||
| -rw-r--r-- | db/migrate/20161207231626_add_environment_slug.rb | 60 | ||||
| -rw-r--r-- | db/migrate/20161209153400_add_unique_index_for_environment_slug.rb | 15 | ||||
| -rw-r--r-- | db/schema.rb | 14 | ||||
| -rw-r--r-- | doc/api/enviroments.md | 8 | ||||
| -rw-r--r-- | lib/api/entities.rb | 2 | ||||
| -rw-r--r-- | lib/api/environments.rb | 3 | ||||
| -rw-r--r-- | lib/api/helpers/custom_validators.rb | 14 | ||||
| -rw-r--r-- | lib/gitlab/regex.rb | 9 | ||||
| -rw-r--r-- | spec/lib/gitlab/regex_spec.rb | 16 | ||||
| -rw-r--r-- | spec/models/environment_spec.rb | 48 | ||||
| -rw-r--r-- | spec/requests/api/environments_spec.rb | 17 |
13 files changed, 242 insertions, 18 deletions
diff --git a/app/models/environment.rb b/app/models/environment.rb index 96700143ddd..0abbf674b9d 100644 --- a/app/models/environment.rb +++ b/app/models/environment.rb @@ -1,9 +1,15 @@ class Environment < ActiveRecord::Base + # Used to generate random suffixes for the slug + NUMBERS = '0'..'9' + SUFFIX_CHARS = ('a'..'z').to_a + NUMBERS.to_a + belongs_to :project, required: true, validate: true has_many :deployments before_validation :nullify_external_url + before_validation :generate_slug, if: ->(env) { env.slug.blank? } + before_save :set_environment_type validates :name, @@ -13,6 +19,13 @@ class Environment < ActiveRecord::Base format: { with: Gitlab::Regex.environment_name_regex, message: Gitlab::Regex.environment_name_regex_message } + validates :slug, + presence: true, + uniqueness: { scope: :project_id }, + length: { maximum: 24 }, + format: { with: Gitlab::Regex.environment_slug_regex, + message: Gitlab::Regex.environment_slug_regex_message } + validates :external_url, uniqueness: { scope: :project_id }, length: { maximum: 255 }, @@ -107,4 +120,41 @@ class Environment < ActiveRecord::Base action.expanded_environment_name == environment end end + + # An environment name is not necessarily suitable for use in URLs, DNS + # or other third-party contexts, so provide a slugified version. A slug has + # the following properties: + # * contains only lowercase letters (a-z), numbers (0-9), and '-' + # * begins with a letter + # * has a maximum length of 24 bytes (OpenShift limitation) + # * cannot end with `-` + def generate_slug + # Lowercase letters and numbers only + slugified = name.to_s.downcase.gsub(/[^a-z0-9]/, '-') + + # Must start with a letter + slugified = "env-" + slugified if NUMBERS.cover?(slugified[0]) + + # Maximum length: 24 characters (OpenShift limitation) + slugified = slugified[0..23] + + # Cannot end with a "-" character (Kubernetes label limitation) + slugified = slugified[0..-2] if slugified[-1] == "-" + + # Add a random suffix, shortening the current string if necessary, if it + # has been slugified. This ensures uniqueness. + slugified = slugified[0..16] + "-" + random_suffix if slugified != name + + self.slug = slugified + end + + private + + # Slugifying a name may remove the uniqueness guarantee afforded by it being + # based on name (which must be unique). To compensate, we add a random + # 6-byte suffix in those circumstances. This is not *guaranteed* uniqueness, + # but the chance of collisions is vanishingly small + def random_suffix + (0..5).map { SUFFIX_CHARS.sample }.join + end end diff --git a/changelogs/unreleased/22864-add-environment-slug.yml b/changelogs/unreleased/22864-add-environment-slug.yml new file mode 100644 index 00000000000..f90f79337d5 --- /dev/null +++ b/changelogs/unreleased/22864-add-environment-slug.yml @@ -0,0 +1,4 @@ +--- +title: Add a slug to environments +merge_request: 7983 +author: diff --git a/db/migrate/20161207231626_add_environment_slug.rb b/db/migrate/20161207231626_add_environment_slug.rb new file mode 100644 index 00000000000..7153e6a32b1 --- /dev/null +++ b/db/migrate/20161207231626_add_environment_slug.rb @@ -0,0 +1,60 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddEnvironmentSlug < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = true + DOWNTIME_REASON = 'Adding NOT NULL column environments.slug with dependent data' + + # Used to generate random suffixes for the slug + NUMBERS = '0'..'9' + SUFFIX_CHARS = ('a'..'z').to_a + NUMBERS.to_a + + def up + environments = Arel::Table.new(:environments) + + add_column :environments, :slug, :string + finder = environments.project(:id, :name) + + connection.exec_query(finder.to_sql).rows.each do |id, name| + updater = Arel::UpdateManager.new(ActiveRecord::Base). + table(environments). + set(environments[:slug] => generate_slug(name)). + where(environments[:id].eq(id)) + + connection.exec_update(updater.to_sql, self.class.name, []) + end + + change_column_null :environments, :slug, false + end + + def down + remove_column :environments, :slug + end + + # Copy of the Environment#generate_slug implementation + def generate_slug(name) + # Lowercase letters and numbers only + slugified = name.to_s.downcase.gsub(/[^a-z0-9]/, '-') + + # Must start with a letter + slugified = "env-" + slugified if NUMBERS.cover?(slugified[0]) + + # Maximum length: 24 characters (OpenShift limitation) + slugified = slugified[0..23] + + # Cannot end with a "-" character (Kubernetes label limitation) + slugified = slugified[0..-2] if slugified[-1] == "-" + + # Add a random suffix, shortening the current string if necessary, if it + # has been slugified. This ensures uniqueness. + slugified = slugified[0..16] + "-" + random_suffix if slugified != name + + slugified + end + + def random_suffix + (0..5).map { SUFFIX_CHARS.sample }.join + end +end diff --git a/db/migrate/20161209153400_add_unique_index_for_environment_slug.rb b/db/migrate/20161209153400_add_unique_index_for_environment_slug.rb new file mode 100644 index 00000000000..e9fcef1cd45 --- /dev/null +++ b/db/migrate/20161209153400_add_unique_index_for_environment_slug.rb @@ -0,0 +1,15 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddUniqueIndexForEnvironmentSlug < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = true + DOWNTIME_REASON = 'Adding a *unique* index to environments.slug' + + disable_ddl_transaction! + + def change + add_concurrent_index :environments, [:project_id, :slug], unique: true + end +end diff --git a/db/schema.rb b/db/schema.rb index 83c8ad48537..67ff83d96d9 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: 20161207231621) do +ActiveRecord::Schema.define(version: 20161212142807) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -98,14 +98,14 @@ ActiveRecord::Schema.define(version: 20161207231621) do t.text "help_page_text_html" t.text "shared_runners_text_html" t.text "after_sign_up_text_html" + t.boolean "sidekiq_throttling_enabled", default: false + t.string "sidekiq_throttling_queues" + t.decimal "sidekiq_throttling_factor" t.boolean "housekeeping_enabled", default: true, null: false t.boolean "housekeeping_bitmaps_enabled", default: true, null: false t.integer "housekeeping_incremental_repack_period", default: 10, null: false t.integer "housekeeping_full_repack_period", default: 50, null: false t.integer "housekeeping_gc_period", default: 200, null: false - t.boolean "sidekiq_throttling_enabled", default: false - t.string "sidekiq_throttling_queues" - t.decimal "sidekiq_throttling_factor" t.boolean "html_emails_enabled", default: true end @@ -428,9 +428,11 @@ ActiveRecord::Schema.define(version: 20161207231621) do t.string "external_url" t.string "environment_type" t.string "state", default: "available", null: false + t.string "slug", null: false end add_index "environments", ["project_id", "name"], name: "index_environments_on_project_id_and_name", unique: true, using: :btree + add_index "environments", ["project_id", "slug"], name: "index_environments_on_project_id_and_slug", unique: true, using: :btree create_table "events", force: :cascade do |t| t.string "target_type" @@ -737,8 +739,8 @@ ActiveRecord::Schema.define(version: 20161207231621) do t.integer "visibility_level", default: 20, null: false t.boolean "request_access_enabled", default: false, null: false t.datetime "deleted_at" - t.text "description_html" t.boolean "lfs_enabled" + t.text "description_html" t.integer "parent_id" end @@ -1219,8 +1221,8 @@ ActiveRecord::Schema.define(version: 20161207231621) do t.datetime "otp_grace_period_started_at" t.boolean "ldap_email", default: false, null: false t.boolean "external", default: false - t.string "incoming_email_token" t.string "organization" + t.string "incoming_email_token" t.boolean "authorized_projects_populated" end diff --git a/doc/api/enviroments.md b/doc/api/enviroments.md index 87a5fa67124..1299aca8c45 100644 --- a/doc/api/enviroments.md +++ b/doc/api/enviroments.md @@ -22,8 +22,9 @@ Example response: [ { "id": 1, - "name": "Env1", - "external_url": "https://env1.example.gitlab.com" + "name": "review/fix-foo", + "slug": "review-fix-foo-dfjre3", + "external_url": "https://review-fix-foo-dfjre3.example.gitlab.com" } ] ``` @@ -54,6 +55,7 @@ Example response: { "id": 1, "name": "deploy", + "slug": "deploy", "external_url": "https://deploy.example.gitlab.com" } ``` @@ -85,6 +87,7 @@ Example response: { "id": 1, "name": "staging", + "slug": "staging", "external_url": "https://staging.example.gitlab.com" } ``` @@ -112,6 +115,7 @@ Example response: { "id": 1, "name": "deploy", + "slug": "deploy", "external_url": "https://deploy.example.gitlab.com" } ``` diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 01c0f5072ba..dfbb3ab86dd 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -629,7 +629,7 @@ module API end class EnvironmentBasic < Grape::Entity - expose :id, :name, :external_url + expose :id, :name, :slug, :external_url end class Environment < EnvironmentBasic diff --git a/lib/api/environments.rb b/lib/api/environments.rb index 80bbd9bb6e4..1a7e68f0528 100644 --- a/lib/api/environments.rb +++ b/lib/api/environments.rb @@ -1,6 +1,7 @@ module API # Environments RESTfull API endpoints class Environments < Grape::API + include ::API::Helpers::CustomValidators include PaginationParams before { authenticate! } @@ -29,6 +30,7 @@ module API params do requires :name, type: String, desc: 'The name of the environment to be created' optional :external_url, type: String, desc: 'URL on which this deployment is viewable' + optional :slug, absence: { message: "is automatically generated and cannot be changed" } end post ':id/environments' do authorize! :create_environment, user_project @@ -50,6 +52,7 @@ module API requires :environment_id, type: Integer, desc: 'The environment ID' optional :name, type: String, desc: 'The new environment name' optional :external_url, type: String, desc: 'The new URL on which this deployment is viewable' + optional :slug, absence: { message: "is automatically generated and cannot be changed" } end put ':id/environments/:environment_id' do authorize! :update_environment, user_project diff --git a/lib/api/helpers/custom_validators.rb b/lib/api/helpers/custom_validators.rb new file mode 100644 index 00000000000..0a8f3073a50 --- /dev/null +++ b/lib/api/helpers/custom_validators.rb @@ -0,0 +1,14 @@ +module API + module Helpers + module CustomValidators + class Absence < Grape::Validations::Base + def validate_param!(attr_name, params) + return if params.respond_to?(:key?) && !params.key?(attr_name) + raise Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)], message: message(:absence) + end + end + end + end +end + +Grape::Validations.register_validator(:absence, ::API::Helpers::CustomValidators::Absence) diff --git a/lib/gitlab/regex.rb b/lib/gitlab/regex.rb index 7c711d581e8..9e0b0e5ea98 100644 --- a/lib/gitlab/regex.rb +++ b/lib/gitlab/regex.rb @@ -131,5 +131,14 @@ module Gitlab def kubernetes_namespace_regex_message "can contain only letters, digits or '-', and cannot start or end with '-'" end + + def environment_slug_regex + @environment_slug_regex ||= /\A[a-z]([a-z0-9-]*[a-z0-9])?\z/.freeze + end + + def environment_slug_regex_message + "can contain only lowercase letters, digits, and '-'. " \ + "Must start with a letter, and cannot end with '-'" + end end end diff --git a/spec/lib/gitlab/regex_spec.rb b/spec/lib/gitlab/regex_spec.rb index c51b10bdc69..c78cd30157e 100644 --- a/spec/lib/gitlab/regex_spec.rb +++ b/spec/lib/gitlab/regex_spec.rb @@ -29,4 +29,20 @@ describe Gitlab::Regex, lib: true do describe 'file path regex' do it { expect('foo@/bar').to match(Gitlab::Regex.file_path_regex) } end + + describe 'environment slug regex' do + def be_matched + match(Gitlab::Regex.environment_slug_regex) + end + + it { expect('foo').to be_matched } + it { expect('foo-1').to be_matched } + + it { expect('FOO').not_to be_matched } + it { expect('foo/1').not_to be_matched } + it { expect('foo.1').not_to be_matched } + it { expect('foo*1').not_to be_matched } + it { expect('9foo').not_to be_matched } + it { expect('foo-').not_to be_matched } + end end diff --git a/spec/models/environment_spec.rb b/spec/models/environment_spec.rb index c8170164898..706f1a5cd1c 100644 --- a/spec/models/environment_spec.rb +++ b/spec/models/environment_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe Environment, models: true do - let(:environment) { create(:environment) } + subject(:environment) { create(:environment) } it { is_expected.to belong_to(:project) } it { is_expected.to have_many(:deployments) } @@ -15,15 +15,11 @@ describe Environment, models: true do it { is_expected.to validate_uniqueness_of(:name).scoped_to(:project_id) } it { is_expected.to validate_length_of(:name).is_at_most(255) } - it { is_expected.to validate_length_of(:external_url).is_at_most(255) } - - # To circumvent a not null violation of the name column: - # https://github.com/thoughtbot/shoulda-matchers/issues/336 - it 'validates uniqueness of :external_url' do - create(:environment) + it { is_expected.to validate_uniqueness_of(:slug).scoped_to(:project_id) } + it { is_expected.to validate_length_of(:slug).is_at_most(24) } - is_expected.to validate_uniqueness_of(:external_url).scoped_to(:project_id) - end + it { is_expected.to validate_length_of(:external_url).is_at_most(255) } + it { is_expected.to validate_uniqueness_of(:external_url).scoped_to(:project_id) } describe '#nullify_external_url' do it 'replaces a blank url with nil' do @@ -199,4 +195,38 @@ describe Environment, models: true do expect(environment.actions_for('review/master')).to contain_exactly(review_action) end end + + describe '#slug' do + it "is automatically generated" do + expect(environment.slug).not_to be_nil + end + + it "is not regenerated if name changes" do + original_slug = environment.slug + environment.update_attributes!(name: environment.name.reverse) + + expect(environment.slug).to eq(original_slug) + end + end + + describe '#generate_slug' do + SUFFIX = "-[a-z0-9]{6}" + { + "staging-12345678901234567" => "staging-123456789" + SUFFIX, + "9-staging-123456789012345" => "env-9-staging-123" + SUFFIX, + "staging-1234567890123456" => "staging-1234567890123456", + "production" => "production", + "PRODUCTION" => "production" + SUFFIX, + "review/1-foo" => "review-1-foo" + SUFFIX, + "1-foo" => "env-1-foo" + SUFFIX, + "1/foo" => "env-1-foo" + SUFFIX, + "foo-" => "foo" + SUFFIX, + }.each do |name, matcher| + it "returns a slug matching #{matcher}, given #{name}" do + slug = described_class.new(name: name).generate_clean_name + + expect(slug).to match(/\A#{matcher}\z/) + end + end + end end diff --git a/spec/requests/api/environments_spec.rb b/spec/requests/api/environments_spec.rb index 126496c43a5..b9d535bc314 100644 --- a/spec/requests/api/environments_spec.rb +++ b/spec/requests/api/environments_spec.rb @@ -46,6 +46,7 @@ describe API::Environments, api: true do expect(response).to have_http_status(201) expect(json_response['name']).to eq('mepmep') + expect(json_response['slug']).to eq('mepmep') expect(json_response['external']).to be nil end @@ -60,6 +61,13 @@ describe API::Environments, api: true do expect(response).to have_http_status(400) end + + it 'returns a 400 if slug is specified' do + post api("/projects/#{project.id}/environments", user), name: "foo", slug: "foo" + + expect(response).to have_http_status(400) + expect(json_response["error"]).to eq("slug is automatically generated and cannot be changed") + end end context 'a non member' do @@ -86,6 +94,15 @@ describe API::Environments, api: true do expect(json_response['external_url']).to eq(url) end + it "won't allow slug to be changed" do + slug = environment.slug + api_url = api("/projects/#{project.id}/environments/#{environment.id}", user) + put api_url, slug: slug + "-foo" + + expect(response).to have_http_status(400) + expect(json_response["error"]).to eq("slug is automatically generated and cannot be changed") + end + it "won't update the external_url if only the name is passed" do url = environment.external_url put api("/projects/#{project.id}/environments/#{environment.id}", user), |