diff options
author | Kamil Trzciński <ayufan@ayufan.eu> | 2016-12-15 16:18:50 +0000 |
---|---|---|
committer | Kamil Trzciński <ayufan@ayufan.eu> | 2016-12-15 16:18:50 +0000 |
commit | dfdce39ad6860b54f7f0be7d91ef809d8da24c18 (patch) | |
tree | 315f45bc57dc2586941fd8766005f3d2813e740f | |
parent | ada8b026ef55733a94821525249ed67a094d5521 (diff) | |
parent | 80513a129592583ed100e7a90fc9ea144eb62ea9 (diff) | |
download | gitlab-ce-dfdce39ad6860b54f7f0be7d91ef809d8da24c18.tar.gz |
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
22 files changed, 440 insertions, 46 deletions
diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 45a416a4c41..fdbf28a1d68 100644 --- a/app/models/ci/build.rb +++ b/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) diff --git a/app/models/environment.rb b/app/models/environment.rb index 96700143ddd..8ef1c841ea3 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 }, @@ -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 diff --git a/app/services/ci/create_pipeline_builds_service.rb b/app/services/ci/create_pipeline_builds_service.rb index 005014fa1de..b7da3f8e7eb 100644 --- a/app/services/ci/create_pipeline_builds_service.rb +++ b/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 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/20161207231620_fixup_environment_name_uniqueness.rb b/db/migrate/20161207231620_fixup_environment_name_uniqueness.rb new file mode 100644 index 00000000000..b74552e762d --- /dev/null +++ b/db/migrate/20161207231620_fixup_environment_name_uniqueness.rb @@ -0,0 +1,53 @@ +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 diff --git a/db/migrate/20161207231621_create_environment_name_unique_index.rb b/db/migrate/20161207231621_create_environment_name_unique_index.rb new file mode 100644 index 00000000000..ac680c8d10f --- /dev/null +++ b/db/migrate/20161207231621_create_environment_name_unique_index.rb @@ -0,0 +1,18 @@ +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 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 4711b7873af..67ff83d96d9 100644 --- a/db/schema.rb +++ b/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 @@ -1290,4 +1292,4 @@ ActiveRecord::Schema.define(version: 20161212142807) do add_foreign_key "subscriptions", "projects", on_delete: :cascade add_foreign_key "trending_projects", "projects", on_delete: :cascade add_foreign_key "u2f_registrations", "users" -end
\ No newline at end of file +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/doc/ci/environments.md b/doc/ci/environments.md index 705bca6cc1f..bad0233a9ce 100644 --- a/doc/ci/environments.md +++ b/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. diff --git a/doc/ci/variables/README.md b/doc/ci/variables/README.md index e0ff9756868..eb540a50606 100644 --- a/doc/ci/variables/README.md +++ b/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 | diff --git a/doc/ci/yaml/README.md b/doc/ci/yaml/README.md index 5e8d888e555..a62193700fc 100644 --- a/doc/ci/yaml/README.md +++ b/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 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/build_spec.rb b/spec/models/build_spec.rb index ea60c17a58a..d5f2ffcff59 100644 --- a/spec/models/build_spec.rb +++ b/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) diff --git a/spec/models/environment_spec.rb b/spec/models/environment_spec.rb index c8170164898..97cbb093ed2 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_slug + + 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), diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index 4aadd009f3e..ceaca96e25b 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/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 |