Merge branch '22864-add-clean-environment-name' into 'master'

Add a slug to environments

## What does this MR do?

Adds a `slug` field to the `environments` table, populating existing rows and ensuring that new rows will get an entry.

Cleaning examples:

* `review/foo`  => `review-foo-5gghdf`
* `review-foo` => `review-foo`
* `1-foo` => `env-1-foo-e2hx12`
* `production` => `production`
* `Production` => `production-f8ddlz`

## Are there points in the code the reviewer needs to double check?

This migration requires downtime. I don't see a way to avoid it.

## Why was this MR needed?

External services often have more restrictive rules on naming than those enforced for `environments.name`. In particular, forward slashes and names longer than 24 characters causes problems on OpenShift. `slug` is designed to be an acceptable alternative to `name` in these situations. Since forward slashes are a documented part of environment names, to set environment types, we need an envionmnent slug, not just a slug for the branch name.

## Does this MR meet the acceptance criteria?

- [x] [Changelog entry](https://docs.gitlab.com/ce/development/changelog.html) added
- [x] [Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)
- [X] API support added
- Tests
  - [X] Added for this feature/bug
  - [x] All builds are passing
- [X] Conform by the [merge request performance guides](http://docs.gitlab.com/ce/development/merge_request_performance_guidelines.html)
- [X] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [X] Branch has no merge conflicts with `master` (if it does - rebase it please)
- [X] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

## What are the relevant issue numbers?

Part of #22864

See merge request !7983

app/models/ci/build.rb

@@ -9,6 +9,14 @@ module Ci
 
     has_many :deployments, as: :deployable
 
+    # The "environment" field for builds is a String, and is the unexpanded name
+    def persisted_environment
+      @persisted_environment ||= Environment.find_by(
+        name: expanded_environment_name,
+        project_id: gl_project_id
+      )
+    end
+
     serialize :options
     serialize :yaml_variables
 
@@ -143,7 +151,7 @@ module Ci
     end
 
     def expanded_environment_name
-      ExpandVariables.expand(environment, variables) if environment
+      ExpandVariables.expand(environment, simple_variables) if environment
     end
 
     def has_environment?
@@ -206,7 +214,8 @@ module Ci
       slugified.gsub(/[^a-z0-9]/, '-')[0..62]
     end
 
-    def variables
+    # Variables whose value does not depend on other variables
+    def simple_variables
       variables = predefined_variables
       variables += project.predefined_variables
       variables += pipeline.predefined_variables
@@ -219,6 +228,13 @@ module Ci
       variables
     end
 
+    # All variables, including those dependent on other variables
+    def variables
+      variables = simple_variables
+      variables += persisted_environment.predefined_variables if persisted_environment.present?
+      variables
+    end
+
     def merge_request
       merge_requests = MergeRequest.includes(:merge_request_diff)
                                    .where(source_branch: ref, source_project_id: pipeline.gl_project_id)

app/models/environment.rb

 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 },
@@ -37,6 +50,13 @@ class Environment < ActiveRecord::Base
     state :stopped
   end
 
+  def predefined_variables
+    [
+      { key: 'CI_ENVIRONMENT_NAME', value: name, public: true },
+      { key: 'CI_ENVIRONMENT_SLUG', value: slug, public: true },
+    ]
+  end
+
   def recently_updated_on_branch?(ref)
     ref.to_s == last_deployment.try(:ref)
   end
@@ -107,4 +127,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

app/services/ci/create_pipeline_builds_service.rb

@@ -10,18 +10,29 @@ module Ci
       end
     end
 
+    def project
+      pipeline.project
+    end
+
     private
 
     def create_build(build_attributes)
       build_attributes = build_attributes.merge(
         pipeline: pipeline,
-        project: pipeline.project,
+        project: project,
         ref: pipeline.ref,
         tag: pipeline.tag,
         user: current_user,
         trigger_request: trigger_request
       )
-      pipeline.builds.create(build_attributes)
+      build = pipeline.builds.create(build_attributes)
+
+      # Create the environment before the build starts. This sets its slug and
+      # makes it available as an environment variable
+      project.environments.find_or_create_by(name: build.expanded_environment_name) if
+        build.has_environment?
+
+      build
     end
 
     def new_builds

changelogs/unreleased/22864-add-environment-slug.yml

+---
+title: Add a slug to environments
+merge_request: 7983
+author:

db/migrate/20161207231620_fixup_environment_name_uniqueness.rb

+class FixupEnvironmentNameUniqueness < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = true
+  DOWNTIME_REASON = 'Renaming non-unique environments'
+
+  def up
+    environments = Arel::Table.new(:environments)
+
+    # Get all [project_id, name] pairs that occur more than once
+    finder_sql = environments.
+      group(environments[:project_id], environments[:name]).
+      having(Arel.sql("COUNT(1)").gt(1)).
+      project(environments[:project_id], environments[:name]).
+      to_sql
+
+    conflicting = connection.exec_query(finder_sql)
+
+    conflicting.rows.each do |project_id, name|
+      fix_duplicates(project_id, name)
+    end
+  end
+
+  def down
+    # Nothing to do
+  end
+
+  # Rename conflicting environments by appending "-#{id}" to all but the first
+  def fix_duplicates(project_id, name)
+    environments = Arel::Table.new(:environments)
+    finder_sql = environments.
+      where(environments[:project_id].eq(project_id)).
+      where(environments[:name].eq(name)).
+      order(environments[:id].asc).
+      project(environments[:id], environments[:name]).
+      to_sql
+
+    # Now we have the data for all the conflicting rows
+    conflicts = connection.exec_query(finder_sql).rows
+    conflicts.shift # Leave the first row alone
+
+    conflicts.each do |id, name|
+      update_sql =
+        Arel::UpdateManager.new(ActiveRecord::Base).
+        table(environments).
+        set(environments[:name] => name + "-" + id.to_s).
+        where(environments[:id].eq(id)).
+        to_sql
+
+      connection.exec_update(update_sql, self.class.name, [])
+    end
+  end
+end

db/migrate/20161207231621_create_environment_name_unique_index.rb

+class CreateEnvironmentNameUniqueIndex < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  disable_ddl_transaction!
+
+  DOWNTIME = true
+  DOWNTIME_REASON = 'Making a non-unique index into a unique index'
+
+  def up
+    remove_index :environments, [:project_id, :name]
+    add_concurrent_index :environments, [:project_id, :name], unique: true
+  end
+
+  def down
+    remove_index :environments, [:project_id, :name], unique: true
+    add_concurrent_index :environments, [:project_id, :name]
+  end
+end

db/migrate/20161207231626_add_environment_slug.rb

+# 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

db/migrate/20161209153400_add_unique_index_for_environment_slug.rb

+# 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

db/schema.rb

@@ -98,14 +98,14 @@ ActiveRecord::Schema.define(version: 20161212142807) 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: 20161212142807) 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", using: :btree
+  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: 20161212142807) 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: 20161212142807) 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
 

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"
 }
 ```

doc/ci/environments.md

@@ -86,6 +86,13 @@ will later see, is exposed in various places within GitLab. Each time a job that
 has an environment specified and succeeds, a deployment is recorded, remembering
 the Git SHA and environment name.
 
+>**Note:**
+Starting with GitLab 8.15, the environment name is exposed to the Runner in
+two forms: `$CI_ENVIRONMENT_NAME`, and `$CI_ENVIRONMENT_SLUG`. The first is
+the name given in `.gitlab-ci.yml` (with any variables expanded), while the
+second is a "cleaned-up" version of the name, suitable for use in URLs, DNS,
+etc.
+
 To sum up, with the above `.gitlab-ci.yml` we have achieved that:
 
 - All branches will run the `test` and `build` jobs.
@@ -157,7 +164,7 @@ that can be found in the deployments page
 job with the commit associated with it.
 
 >**Note:**
-Bare in mind that your mileage will vary and it's entirely up to how you define
+Bear in mind that your mileage will vary and it's entirely up to how you define
 the deployment process in the job's `script` whether the rollback succeeds or not.
 GitLab CI is just following orders.
 
@@ -248,7 +255,7 @@ deploy_review:
     - echo "Deploy a review app"
   environment:
     name: review/$CI_BUILD_REF_NAME
-    url: https://$CI_BUILD_REF_SLUG.example.com
+    url: https://$CI_BUILD_REF_SLUG.review.example.com
   only:
     - branches
   except:
@@ -266,9 +273,18 @@ ones.
 So, the first part is `review`, followed by a `/` and then `$CI_BUILD_REF_NAME`
 which takes the value of the branch name. Since `$CI_BUILD_REF_NAME` itself may
 also contain `/`, or other characters that would be invalid in a domain name or
-URL, we use `$CI_BUILD_REF_SLUG` in the `environment:url` so that the environment
-can get a specific and distinct URL for each branch. Again, the way you set up
-the webserver to serve these requests is based on your setup.
+URL, we use `$CI_ENVIRONMENT_SLUG` in the `environment:url` so that the
+environment can get a specific and distinct URL for each branch. In this case,
+given a `$CI_BUILD_REF_NAME` of `100-Do-The-Thing`, the URL will be something
+like `https://review-100-do-the-4f99a2.example.com`. Again, the way you set up
+the web server to serve these requests is based on your setup.
+
+You could also use `$CI_BUILD_REF_SLUG` in `environment:url`, e.g.:
+`https://$CI_BUILD_REF_SLUG.review.example.com`. We use `$CI_ENVIRONMENT_SLUG`
+here because it is guaranteed to be unique, but if you're using a workflow like
+[GitLab Flow][gitlab-flow], collisions are very unlikely, and you may prefer
+environment names to be more closely based on the branch name - the example
+above would give you an URL like `https://100-do-the-thing.review.example.com`
 
 Last but not least, we tell the job to run [`only`][only] on branches
 [`except`][only] master.
@@ -300,7 +316,7 @@ deploy_review:
     - echo "Deploy a review app"
   environment:
     name: review/$CI_BUILD_REF_NAME
-    url: https://$CI_BUILD_REF_SLUG.example.com
+    url: https://$CI_ENVIRONMENT_SLUG.example.com
   only:
     - branches
   except:
@@ -419,7 +435,7 @@ deploy_review:
     - echo "Deploy a review app"
   environment:
     name: review/$CI_BUILD_REF_NAME
-    url: https://$CI_BUILD_REF_SLUG.example.com
+    url: https://$CI_ENVIRONMENT_SLUG.example.com
     on_stop: stop_review
   only:
     - branches
@@ -493,10 +509,6 @@ fetch = +refs/environments/*:refs/remotes/origin/environments/*
 
 ## Limitations
 
-1. `$CI_BUILD_REF_SLUG` is not *guaranteed* to be unique, so there is a small
-    chance of collisions between similarly-named branches (`fix-foo` would
-    conflict with `fix/foo`, for instance). Following a well-defined workflow
-    such as [GitLab Flow][gitlab-flow] can keep this from being a problem.
 1. You are limited to use only the [CI predefined variables][variables] in the
    `environment: name`. If you try to re-use variables defined inside `script`
    as part of the environment name, it will not work.

doc/ci/variables/README.md

@@ -52,6 +52,8 @@ version of Runner required.
 | **CI_PROJECT_PATH**     | 8.10   | 0.5    | The namespace with project name |
 | **CI_PROJECT_URL**      | 8.10   | 0.5    | The HTTP address to access project |
 | **CI_PROJECT_DIR**      | all    | all    | The full path where the repository is cloned and where the build is run |
+| **CI_ENVIRONMENT_NAME** | 8.15   | all    | The name of the environment for this build |
+| **CI_ENVIRONMENT_SLUG** | 8.15   | all    | A simplified version of the environment name, suitable for inclusion in DNS, URLs, Kubernetes labels, etc. |
 | **CI_REGISTRY**         | 8.10   | 0.5    | If the Container Registry is enabled it returns the address of GitLab's Container Registry |
 | **CI_REGISTRY_IMAGE**   | 8.10   | 0.5    | If the Container Registry is enabled for the project it returns the address of the registry tied to the specific project |
 | **CI_RUNNER_ID**        | 8.10   | 0.5    | The unique id of runner being used |

doc/ci/yaml/README.md

@@ -690,7 +690,7 @@ The `stop_review_app` job is **required** to have the following keywords defined
 #### dynamic environments
 
 > [Introduced][ce-6323] in GitLab 8.12 and GitLab Runner 1.6.
-  `$CI_BUILD_REF_SLUG` was [introduced][ce-8072] in GitLab 8.15.
+  `$CI_ENVIRONMENT_SLUG` was [introduced][ce-7983] in GitLab 8.15
 
 `environment` can also represent a configuration hash with `name` and `url`.
 These parameters can use any of the defined [CI variables](#variables)
@@ -703,15 +703,17 @@ deploy as review app:
   stage: deploy
   script: make deploy
   environment:
-    name: review-apps/$CI_BUILD_REF_NAME
-    url: https://$CI_BUILD_REF_SLUG.review.example.com/
+    name: review/$CI_BUILD_REF_NAME
+    url: https://$CI_ENVIRONMENT_SLUG.example.com/
 ```
 
 The `deploy as review app` job will be marked as deployment to dynamically
-create the `review-apps/$CI_BUILD_REF_NAME` environment, which `$CI_BUILD_REF_NAME`
-is an [environment variable][variables] set by the Runner. If for example the
-`deploy as review app` job was run in a branch named `pow`, this environment
-should be accessible under `https://pow.review.example.com/`.
+create the `review/$CI_BUILD_REF_NAME` environment, where `$CI_BUILD_REF_NAME`
+is an [environment variable][variables] set by the Runner. The
+`$CI_ENVIRONMENT_SLUG` variable is based on the environment name, but suitable
+for inclusion in URLs. In this case, if the `deploy as review app` job was run
+in a branch named `pow`, this environment would be accessible with an URL like
+`https://review-pow-aaaaaa.example.com/`.
 
 This of course implies that the underlying server which hosts the application
 is properly configured.
@@ -720,10 +722,6 @@ The common use case is to create dynamic environments for branches and use them
 as Review Apps. You can see a simple example using Review Apps at
 https://gitlab.com/gitlab-examples/review-apps-nginx/.
 
-`$CI_BUILD_REF_SLUG` is another environment variable set by the runner, based on
-`$CI_BUILD_REF_NAME` but lower-cased, and with some characters replaced with
-`-`, making it suitable for use in URLs and domain names.
-
 ### artifacts
 
 >**Notes:**
@@ -1243,5 +1241,5 @@ CI with various languages.
 [ce-6323]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6323
 [environment]: ../environments.md
 [ce-6669]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6669
-[ce-8072]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/xxxx
 [variables]: ../variables/README.md
+[ce-7983]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7983

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

lib/api/environments.rb

 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

lib/api/helpers/custom_validators.rb

+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)

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

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

spec/models/build_spec.rb

@@ -87,6 +87,26 @@ describe Ci::Build, models: true do
     end
   end
 
+  describe '#persisted_environment' do
+    before do
+      @environment = create(:environment, project: project, name: "foo-#{project.default_branch}")
+    end
+
+    subject { build.persisted_environment }
+
+    context 'referenced literally' do
+      let(:build) { create(:ci_build, pipeline: pipeline, environment: "foo-#{project.default_branch}") }
+
+      it { is_expected.to eq(@environment) }
+    end
+
+    context 'referenced with a variable' do
+      let(:build) { create(:ci_build, pipeline: pipeline, environment: "foo-$CI_BUILD_REF_NAME") }
+
+      it { is_expected.to eq(@environment) }
+    end
+  end
+
   describe '#trace' do
     it { expect(build.trace).to be_nil }
 
@@ -328,6 +348,22 @@ describe Ci::Build, models: true do
       it { user_variables.each { |v| is_expected.to include(v) } }
     end
 
+    context 'when build has an environment' do
+      before do
+        build.update(environment: 'production')
+        create(:environment, project: build.project, name: 'production', slug: 'prod-slug')
+      end
+
+      let(:environment_variables) do
+        [
+          { key: 'CI_ENVIRONMENT_NAME', value: 'production', public: true },
+          { key: 'CI_ENVIRONMENT_SLUG', value: 'prod-slug',  public: true }
+        ]
+      end
+
+      it { environment_variables.each { |v| is_expected.to include(v) } }
+    end
+
     context 'when build started manually' do
       before do
         build.update_attributes(when: :manual)

spec/models/environment_spec.rb

 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_slug
+
+        expect(slug).to match(/\A#{matcher}\z/)
+      end
+    end
+  end
 end

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),

spec/services/ci/create_pipeline_service_spec.rb

@@ -210,5 +210,22 @@ describe Ci::CreatePipelineService, services: true do
         expect(result.manual_actions).not_to be_empty
       end
     end
+
+    context 'with environment' do
+      before do
+        config = YAML.dump(deploy: { environment: { name: "review/$CI_BUILD_REF_NAME" }, script: 'ls' })
+        stub_ci_pipeline_yaml_file(config)
+      end
+
+      it 'creates the environment' do
+        result = execute(ref: 'refs/heads/master',
+                         before: '00000000',
+                         after: project.commit.id,
+                         commits: [{ message: 'some msg' }])
+
+        expect(result).to be_persisted
+        expect(Environment.find_by(name: "review/master")).not_to be_nil
+      end
+    end
   end
 end