From dfe6f07814cc8c2940e981658efd54fa6c177d47 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Wed, 2 Nov 2016 12:27:35 +0100
Subject: [PATCH 1/4] Add CI config logical rule for job environments

---
 lib/gitlab/ci/config/node/entry.rb            |  4 ++
 lib/gitlab/ci/config/node/environment.rb      |  4 ++
 lib/gitlab/ci/config/node/validator.rb        |  4 ++
 lib/gitlab/ci/config/rule/base.rb             | 19 ++++++
 lib/gitlab/ci/config/rule/environments.rb     | 64 +++++++++++++++++++
 .../ci/config/rule/environments_spec.rb       | 37 +++++++++++
 6 files changed, 132 insertions(+)
 create mode 100644 lib/gitlab/ci/config/rule/base.rb
 create mode 100644 lib/gitlab/ci/config/rule/environments.rb
 create mode 100644 spec/lib/gitlab/ci/config/rule/environments_spec.rb

diff --git a/lib/gitlab/ci/config/node/entry.rb b/lib/gitlab/ci/config/node/entry.rb
index 8717eabf81eb..032d756d0eb3 100644
--- a/lib/gitlab/ci/config/node/entry.rb
+++ b/lib/gitlab/ci/config/node/entry.rb
@@ -50,6 +50,10 @@ def errors
             @validator.messages + descendants.flat_map(&:errors)
           end
 
+          def add_error(attribute, message)
+            @validator.add_error(attribute, message)
+          end
+
           def value
             if leaf?
               @config
diff --git a/lib/gitlab/ci/config/node/environment.rb b/lib/gitlab/ci/config/node/environment.rb
index 9a95ef43628e..7137e209b0e2 100644
--- a/lib/gitlab/ci/config/node/environment.rb
+++ b/lib/gitlab/ci/config/node/environment.rb
@@ -68,6 +68,10 @@ def on_stop
             value[:on_stop]
           end
 
+          def has_on_stop?
+            on_stop
+          end
+
           def value
             case @config
             when String then { name: @config, action: 'start' }
diff --git a/lib/gitlab/ci/config/node/validator.rb b/lib/gitlab/ci/config/node/validator.rb
index 43c7e102b50a..0e9e2c168677 100644
--- a/lib/gitlab/ci/config/node/validator.rb
+++ b/lib/gitlab/ci/config/node/validator.rb
@@ -17,6 +17,10 @@ def messages
             end
           end
 
+          def add_error(attribute, message)
+            errors.add(attribute, message)
+          end
+
           def self.name
             'Validator'
           end
diff --git a/lib/gitlab/ci/config/rule/base.rb b/lib/gitlab/ci/config/rule/base.rb
new file mode 100644
index 000000000000..88c84f452bae
--- /dev/null
+++ b/lib/gitlab/ci/config/rule/base.rb
@@ -0,0 +1,19 @@
+module Gitlab::Ci
+  class Config
+    module Rule
+      ##
+      # Base abstract class for each logical rule in CI configuration.
+      #
+      class Base
+        def initialize(job, config)
+          @job = job
+          @config = config
+        end
+
+        def apply!
+          raise NotImplementedError
+        end
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/ci/config/rule/environments.rb b/lib/gitlab/ci/config/rule/environments.rb
new file mode 100644
index 000000000000..988104efae48
--- /dev/null
+++ b/lib/gitlab/ci/config/rule/environments.rb
@@ -0,0 +1,64 @@
+module Gitlab::Ci
+  class Config
+    module Rule
+      ##
+      # Job environment rules
+      #
+      class Environments < Rule::Base
+        def initialize(job, config)
+          @job, @config = job, config
+
+          @environment = job[:environment]
+          @on_stop_name = @environment.try(:on_stop).to_s.to_sym
+          @on_stop_job = config[:jobs][@on_stop_name]
+          @on_stop_environment = @on_stop_job[:environment]
+        end
+
+        def apply!
+          return unless has_environment_defined?
+          return unless has_on_stop_defined?
+
+          case
+          when !stop_job_defined?
+            @environment.add_error(:on_stop, 'job not defined')
+
+          when !stop_job_environment_defined?
+            @on_stop_job.add_error(:environment, 'not defined')
+
+          when !stop_job_environment_name_valid?
+            @on_stop_environment.add_error(
+              'name', "does not match environment name defined in `#{@job.name}` job")
+
+          when !stop_job_valid_action_defined?
+            @on_stop_environment.add_error(
+              'action', 'should be defined as `stop`')
+          end
+        end
+
+        def has_environment_defined?
+          @environment.specified?
+        end
+
+        def has_on_stop_defined?
+          @environment.has_on_stop?
+        end
+
+        def stop_job_defined?
+          @on_stop_job.specified?
+        end
+
+        def stop_job_environment_defined?
+          @on_stop_environment.specified?
+        end
+
+        def stop_job_environment_name_valid?
+          @environment.name == @on_stop_environment.name
+        end
+
+        def stop_job_valid_action_defined?
+          @on_stop_environment.action == 'stop'
+        end
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/ci/config/rule/environments_spec.rb b/spec/lib/gitlab/ci/config/rule/environments_spec.rb
new file mode 100644
index 000000000000..26232d953535
--- /dev/null
+++ b/spec/lib/gitlab/ci/config/rule/environments_spec.rb
@@ -0,0 +1,37 @@
+require 'spec_helper'
+
+describe Gitlab::Ci::Config::Rule::Environments do
+  let(:rule) { described_class.new(job, config) }
+
+  before do
+    config.compose!
+    rule.apply!
+  end
+
+  context 'when `on_stop` setting is defined' do
+    context 'when teardown job is not defined' do
+      let(:job) { config[:jobs][:deploy] }
+
+      let(:config) do
+        define_config(
+          deploy: {
+            script: 'rspec',
+            environment: {
+              name: 'test',
+              on_stop: 'teardown_job'
+            }
+          }
+        )
+      end
+
+      it 'invalidates environment that depends on `on_stop`' do
+        expect(config.errors)
+          .to include 'jobs:deploy:environment on stop job not defined'
+      end
+    end
+  end
+
+  def define_config(hash)
+    Gitlab::Ci::Config::Node::Global.new(hash)
+  end
+end
-- 
GitLab


From 4c7ee928ef0a43a2a7cac601525026e139c7b5af Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Fri, 9 Dec 2016 13:41:14 +0100
Subject: [PATCH 2/4] Refine CI configuration rules for environments

---
 lib/gitlab/ci/config/node/entry.rb            |  2 +-
 lib/gitlab/ci/config/node/environment.rb      |  4 +-
 lib/gitlab/ci/config/rule/environments.rb     | 92 +++++++++----------
 .../ci/config/rule/environments_spec.rb       | 27 ++++--
 4 files changed, 64 insertions(+), 61 deletions(-)

diff --git a/lib/gitlab/ci/config/node/entry.rb b/lib/gitlab/ci/config/node/entry.rb
index 032d756d0eb3..e796ace57106 100644
--- a/lib/gitlab/ci/config/node/entry.rb
+++ b/lib/gitlab/ci/config/node/entry.rb
@@ -50,7 +50,7 @@ def errors
             @validator.messages + descendants.flat_map(&:errors)
           end
 
-          def add_error(attribute, message)
+          def error(attribute, message)
             @validator.add_error(attribute, message)
           end
 
diff --git a/lib/gitlab/ci/config/node/environment.rb b/lib/gitlab/ci/config/node/environment.rb
index 7137e209b0e2..52d21bccc208 100644
--- a/lib/gitlab/ci/config/node/environment.rb
+++ b/lib/gitlab/ci/config/node/environment.rb
@@ -68,8 +68,8 @@ def on_stop
             value[:on_stop]
           end
 
-          def has_on_stop?
-            on_stop
+          def stoppable?
+            on_stop.present?
           end
 
           def value
diff --git a/lib/gitlab/ci/config/rule/environments.rb b/lib/gitlab/ci/config/rule/environments.rb
index 988104efae48..1a81fecdbc08 100644
--- a/lib/gitlab/ci/config/rule/environments.rb
+++ b/lib/gitlab/ci/config/rule/environments.rb
@@ -1,62 +1,56 @@
-module Gitlab::Ci
-  class Config
-    module Rule
-      ##
-      # Job environment rules
-      #
-      class Environments < Rule::Base
-        def initialize(job, config)
-          @job, @config = job, config
-
-          @environment = job[:environment]
-          @on_stop_name = @environment.try(:on_stop).to_s.to_sym
-          @on_stop_job = config[:jobs][@on_stop_name]
-          @on_stop_environment = @on_stop_job[:environment]
-        end
+module Gitlab
+  module Ci
+    class Config
+      module Rule
+        ##
+        # Job environment rules
+        #
+        class Environments < Rule::Base
+          def initialize(job, config)
+            @job = job
+            @job_environment = job[:environment]
+
+            @stop_job_name = @environment.try(:on_stop).to_s
+            @stop_job = config[:jobs][@stop_job_name.to_sym]
+            @stop_job_environment = @stop_job[:environment]
+          end
 
-        def apply!
-          return unless has_environment_defined?
-          return unless has_on_stop_defined?
+          def apply!
+            return unless @job_environment.specified?
+            return unless @job_environment.stoppable?
 
-          case
-          when !stop_job_defined?
-            @environment.add_error(:on_stop, 'job not defined')
+            if stop_job_undefined?
+              @job_environment.error('on_stop', 'job not defined')
 
-          when !stop_job_environment_defined?
-            @on_stop_job.add_error(:environment, 'not defined')
+            elsif stop_job_environment_undefined?
+              @stop_job.error('environment', 'not defined')
 
-          when !stop_job_environment_name_valid?
-            @on_stop_environment.add_error(
-              'name', "does not match environment name defined in `#{@job.name}` job")
+            elsif stop_job_environment_name_invalid?
+              @stop_job_environment
+                .error('name', "does not match environment name " \
+                               "defined in `#{@job.name}` job")
 
-          when !stop_job_valid_action_defined?
-            @on_stop_environment.add_error(
-              'action', 'should be defined as `stop`')
+            elsif stop_job_action_invalid?
+              @stop_job_environment
+                .error('action', 'should be defined as `stop`')
+            end
           end
-        end
-
-        def has_environment_defined?
-          @environment.specified?
-        end
-
-        def has_on_stop_defined?
-          @environment.has_on_stop?
-        end
 
-        def stop_job_defined?
-          @on_stop_job.specified?
-        end
+          def stop_job_undefined?
+            !@stop_job.specified?
+          end
 
-        def stop_job_environment_defined?
-          @on_stop_environment.specified?
-        end
+          def stop_job_environment_undefined?
+            !@stop_job_environment.specified?
+          end
 
-        def stop_job_environment_name_valid?
-          @environment.name == @on_stop_environment.name
-        end
+          def stop_job_environment_name_invalid?
+            @environment.name != @stop_job_environment.name
+          end
 
-        def stop_job_valid_action_defined?
-          @on_stop_environment.action == 'stop'
+          def stop_job_action_invalid?
+            @stop_job_environment.action != 'stop'
+          end
         end
       end
     end
diff --git a/spec/lib/gitlab/ci/config/rule/environments_spec.rb b/spec/lib/gitlab/ci/config/rule/environments_spec.rb
index 26232d953535..71f30313b508 100644
--- a/spec/lib/gitlab/ci/config/rule/environments_spec.rb
+++ b/spec/lib/gitlab/ci/config/rule/environments_spec.rb
@@ -1,34 +1,43 @@
 require 'spec_helper'
 
 describe Gitlab::Ci::Config::Rule::Environments do
-  let(:rule) { described_class.new(job, config) }
+  let(:rule) { described_class.new(job, global) }
+  let(:job) { global[:jobs][:deploy] }
+
+  let(:global) do
+    # We may need factory for that
+    #
+    Gitlab::Ci::Config::Node::Global.new(config)
+  end
 
   before do
-    config.compose!
+    # We will phase public `.compose!` out
+    #
+    global.compose!
     rule.apply!
   end
 
-  context 'when `on_stop` setting is defined' do
+  context 'when environment is stoppable' do
     context 'when teardown job is not defined' do
-      let(:job) { config[:jobs][:deploy] }
-
       let(:config) do
-        define_config(
-          deploy: {
+        { deploy: {
             script: 'rspec',
             environment: {
               name: 'test',
               on_stop: 'teardown_job'
             }
           }
-        )
+        }
       end
 
       it 'invalidates environment that depends on `on_stop`' do
-        expect(config.errors)
+        expect(global.errors)
           .to include 'jobs:deploy:environment on stop job not defined'
       end
     end
+
+    context 'when teardown job is defined' do
+    end
   end
 
   def define_config(hash)
-- 
GitLab


From a4ae474195950c6784159c3e948382d6baac99d3 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Fri, 9 Dec 2016 13:54:35 +0100
Subject: [PATCH 3/4] Add test example for environment with teardown job

---
 lib/gitlab/ci/config/rule/environments.rb     |  2 +-
 .../ci/config/rule/environments_spec.rb       | 26 ++++++++++++++++++-
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/lib/gitlab/ci/config/rule/environments.rb b/lib/gitlab/ci/config/rule/environments.rb
index 1a81fecdbc08..b01359cf338b 100644
--- a/lib/gitlab/ci/config/rule/environments.rb
+++ b/lib/gitlab/ci/config/rule/environments.rb
@@ -10,7 +10,7 @@ def initialize(job, config)
             @job = job
             @job_environment = job[:environment]
 
-            @stop_job_name = @environment.try(:on_stop).to_s
+            @stop_job_name = @job_environment.try(:on_stop).to_s
             @stop_job = config[:jobs][@stop_job_name.to_sym]
             @stop_job_environment = @stop_job[:environment]
           end
diff --git a/spec/lib/gitlab/ci/config/rule/environments_spec.rb b/spec/lib/gitlab/ci/config/rule/environments_spec.rb
index 71f30313b508..0cdbabd319d6 100644
--- a/spec/lib/gitlab/ci/config/rule/environments_spec.rb
+++ b/spec/lib/gitlab/ci/config/rule/environments_spec.rb
@@ -30,13 +30,37 @@
         }
       end
 
-      it 'invalidates environment that depends on `on_stop`' do
+      it 'adds error about missing on stop job' do
         expect(global.errors)
           .to include 'jobs:deploy:environment on stop job not defined'
       end
     end
 
     context 'when teardown job is defined' do
+      context 'when teardown job does not have environment defined' do
+        let(:config) do
+          { deploy: {
+              script: 'rspec',
+              environment: {
+                name: 'test',
+                on_stop: 'teardown'
+              }
+            },
+
+            teardown: {
+              script: 'echo teardown'
+            }
+          }
+        end
+
+        it 'adds error about incomplete teardown job' do
+          expect(global.errors)
+            .to include 'jobs:teardown environment not defined'
+        end
+      end
+
+      context 'when teardown job is defined' do
+      end
     end
   end
 
-- 
GitLab


From 56baeca3622ff4a05b5281002ec9440d41b57225 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Fri, 9 Dec 2016 14:07:26 +0100
Subject: [PATCH 4/4] Add remaining tests for ci environment config rules

---
 lib/gitlab/ci/config/rule/environments.rb     |  4 +-
 .../ci/config/rule/environments_spec.rb       | 83 +++++++++++++++++--
 2 files changed, 81 insertions(+), 6 deletions(-)

diff --git a/lib/gitlab/ci/config/rule/environments.rb b/lib/gitlab/ci/config/rule/environments.rb
index b01359cf338b..7c2f88d0691c 100644
--- a/lib/gitlab/ci/config/rule/environments.rb
+++ b/lib/gitlab/ci/config/rule/environments.rb
@@ -36,6 +36,8 @@ def apply!
             end
           end
 
+          private
+
           def stop_job_undefined?
             !@stop_job.specified?
           end
@@ -45,7 +47,7 @@ def stop_job_environment_undefined?
           end
 
           def stop_job_environment_name_invalid?
-            @environment.name != @stop_job_environment.name
+            @job_environment.name != @stop_job_environment.name
           end
 
           def stop_job_action_invalid?
diff --git a/spec/lib/gitlab/ci/config/rule/environments_spec.rb b/spec/lib/gitlab/ci/config/rule/environments_spec.rb
index 0cdbabd319d6..857653c89b88 100644
--- a/spec/lib/gitlab/ci/config/rule/environments_spec.rb
+++ b/spec/lib/gitlab/ci/config/rule/environments_spec.rb
@@ -59,12 +59,85 @@
         end
       end
 
-      context 'when teardown job is defined' do
+      context 'when teardown job has environment defined' do
+        context 'when teardown job has invalid environment name' do
+          let(:config) do
+            { deploy: {
+                script: 'rspec',
+                environment: {
+                  name: 'test',
+                  on_stop: 'teardown'
+                }
+              },
+
+              teardown: {
+                script: 'echo teardown',
+                environment: 'staging'
+              }
+            }
+          end
+
+          it 'adds errors about invalid environment name' do
+            expect(global.errors)
+              .to include 'jobs:teardown:environment name does not match ' \
+                          'environment name defined in `deploy` job'
+          end
+        end
+
+        context 'when teardown job has valid environment name' do
+          context 'when teardown has invalid action name' do
+            let(:config) do
+              { deploy: {
+                  script: 'rspec',
+                  environment: {
+                    name: 'test',
+                    on_stop: 'teardown'
+                  }
+                },
+
+                teardown: {
+                  script: 'echo teardown',
+                  environment: {
+                    name: 'test',
+                    action: 'start'
+                  }
+                }
+              }
+            end
+
+            it 'adds error about invalid action name' do
+              expect(global.errors)
+                .to include 'jobs:teardown:environment action should be ' \
+                            'defined as `stop`'
+            end
+          end
+
+          context 'when teardown job has valid action name' do
+            let(:config) do
+              { deploy: {
+                  script: 'rspec',
+                  environment: {
+                    name: 'test',
+                    on_stop: 'teardown'
+                  }
+                },
+
+                teardown: {
+                  script: 'echo teardown',
+                  environment: {
+                    name: 'test',
+                    action: 'stop'
+                  }
+                }
+              }
+            end
+
+            it 'does not invalidate configuration' do
+              expect(global).to be_valid
+            end
+          end
+        end
       end
     end
   end
-
-  def define_config(hash)
-    Gitlab::Ci::Config::Node::Global.new(hash)
-  end
 end
-- 
GitLab