From 09490fd0ef5717b28af3f23b85d7887c282883d7 Mon Sep 17 00:00:00 2001
From: Shinya Maeda <shinya@gitlab.com>
Date: Thu, 15 Nov 2018 11:19:04 +0000
Subject: [PATCH] Ignore environment validation failure

---
 app/models/concerns/deployable.rb             |  4 ++
 .../ignore-environment-validation-failure.yml |  5 ++
 spec/models/concerns/deployable_spec.rb       | 21 ++++++++
 .../ci/create_pipeline_service_spec.rb        | 48 +++++++++++++++++++
 4 files changed, 78 insertions(+)
 create mode 100644 changelogs/unreleased/ignore-environment-validation-failure.yml

diff --git a/app/models/concerns/deployable.rb b/app/models/concerns/deployable.rb
index 85db01af18d..bc12b06b5af 100644
--- a/app/models/concerns/deployable.rb
+++ b/app/models/concerns/deployable.rb
@@ -13,6 +13,10 @@ module Deployable
         name: expanded_environment_name
       )
 
+      # If we failed to persist envirionment record by validation error, such as name with invalid character,
+      # the job will fall back to a non-environment job.
+      return unless environment.persisted?
+
       create_deployment!(
         project_id: environment.project_id,
         environment: environment,
diff --git a/changelogs/unreleased/ignore-environment-validation-failure.yml b/changelogs/unreleased/ignore-environment-validation-failure.yml
new file mode 100644
index 00000000000..1b61cf86dc4
--- /dev/null
+++ b/changelogs/unreleased/ignore-environment-validation-failure.yml
@@ -0,0 +1,5 @@
+---
+title: Ignore environment validation failure
+merge_request: 23100
+author:
+type: fixed
diff --git a/spec/models/concerns/deployable_spec.rb b/spec/models/concerns/deployable_spec.rb
index ac79c75a55e..6951be903fe 100644
--- a/spec/models/concerns/deployable_spec.rb
+++ b/spec/models/concerns/deployable_spec.rb
@@ -49,5 +49,26 @@ describe Deployable do
         expect(environment).to be_nil
       end
     end
+
+    context 'when environment scope contains invalid character' do
+      let(:job) do
+        create(
+          :ci_build,
+          name: 'job:deploy-to-test-site',
+          environment: '$CI_JOB_NAME',
+          options: {
+            environment: {
+              name: '$CI_JOB_NAME',
+              url: 'http://staging.example.com/$CI_JOB_NAME',
+              on_stop: 'stop_review_app'
+            }
+          })
+      end
+
+      it 'does not create a deployment and environment record' do
+        expect(deployment).to be_nil
+        expect(environment).to be_nil
+      end
+    end
   end
 end
diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb
index 193148d403a..4d9c5aabbda 100644
--- a/spec/services/ci/create_pipeline_service_spec.rb
+++ b/spec/services/ci/create_pipeline_service_spec.rb
@@ -608,5 +608,53 @@ describe Ci::CreatePipelineService do
           .to eq variables_attributes.map(&:with_indifferent_access)
       end
     end
+
+    context 'when pipeline has a job with environment' do
+      let(:pipeline) { execute_service }
+
+      before do
+        stub_ci_pipeline_yaml_file(YAML.dump(config))
+      end
+
+      context 'when environment name is valid' do
+        let(:config) do
+          {
+            review_app: {
+              script: 'deploy',
+              environment: {
+                name: 'review/${CI_COMMIT_REF_NAME}',
+                url: 'http://${CI_COMMIT_REF_SLUG}-staging.example.com'
+              }
+            }
+          }
+        end
+
+        it 'has a job with environment' do
+          expect(pipeline.builds.count).to eq(1)
+          expect(pipeline.builds.first.persisted_environment.name).to eq('review/master')
+          expect(pipeline.builds.first.deployment).to be_created
+        end
+      end
+
+      context 'when environment name is invalid' do
+        let(:config) do
+          {
+            'job:deploy-to-test-site': {
+              script: 'deploy',
+              environment: {
+                name: '${CI_JOB_NAME}',
+                url: 'https://$APP_URL'
+              }
+            }
+          }
+        end
+
+        it 'has a job without environment' do
+          expect(pipeline.builds.count).to eq(1)
+          expect(pipeline.builds.first.persisted_environment).to be_nil
+          expect(pipeline.builds.first.deployment).to be_nil
+        end
+      end
+    end
   end
 end