Define the default value for only/except policies

Currently, if a job does not have only/except policies, the policy is considered as an unspecified state, and therefore the job is executed regardless of how it's executed and which branch/tags are targetted.
Ideally, this should be specified as only: ['branches', 'tags'], as it indicates that unspecified policy jobs are meant to run on any git references.

changelogs/unreleased/define-default-value-for-only-except-keys.yml

+---
+title: Define the default value for only/except policies
+merge_request: 23531
+author:
+type: changed

lib/gitlab/ci/config/entry/except_policy.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    class Config
+      module Entry
+        ##
+        # Entry that represents an only/except trigger policy for the job.
+        #
+        class ExceptPolicy < Policy
+          def self.default
+          end
+        end
+      end
+    end
+  end
+end

lib/gitlab/ci/config/entry/job.rb

@@ -65,10 +65,10 @@ module Gitlab
           entry :services, Entry::Services,
             description: 'Services that will be used to execute this job.'
 
-          entry :only, Entry::Policy,
+          entry :only, Entry::OnlyPolicy,
             description: 'Refs policy this job will be executed for.'
 
-          entry :except, Entry::Policy,
+          entry :except, Entry::ExceptPolicy,
             description: 'Refs policy this job will be executed for.'
 
           entry :variables, Entry::Variables,

lib/gitlab/ci/config/entry/only_policy.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    class Config
+      module Entry
+        ##
+        # Entry that represents an only/except trigger policy for the job.
+        #
+        class OnlyPolicy < Policy
+          def self.default
+            { refs: %w[branches tags] }
+          end
+        end
+      end
+    end
+  end
+end

lib/gitlab/ci/config/entry/policy.rb

@@ -5,12 +5,9 @@ module Gitlab
     class Config
       module Entry
         ##
-        # Entry that represents an only/except trigger policy for the job.
+        # Base class for OnlyPolicy and ExceptPolicy
         #
         class Policy < ::Gitlab::Config::Entry::Simplifiable
-          strategy :RefsPolicy, if: -> (config) { config.is_a?(Array) }
-          strategy :ComplexPolicy, if: -> (config) { config.is_a?(Hash) }
-
           class RefsPolicy < ::Gitlab::Config::Entry::Node
             include ::Gitlab::Config::Entry::Validatable
 
@@ -66,6 +63,16 @@ module Gitlab
 
           def self.default
           end
+
+          ##
+          # Class-level execution won't be inherited by subclasses by default.
+          # Therefore, we need to explicitly execute that for OnlyPolicy and ExceptPolicy
+          def self.inherited(klass)
+            super
+
+            klass.strategy :RefsPolicy, if: -> (config) { config.is_a?(Array) }
+            klass.strategy :ComplexPolicy, if: -> (config) { config.is_a?(Hash) }
+          end
         end
       end
     end

spec/lib/gitlab/ci/config/entry/except_policy_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Ci::Config::Entry::ExceptPolicy do
+  let(:entry) { described_class.new(config) }
+
+  it_behaves_like 'correct only except policy'
+
+  describe '.default' do
+    it 'does not have a default value' do
+      expect(described_class.default).to be_nil
+    end
+  end
+end

spec/lib/gitlab/ci/config/entry/global_spec.rb

@@ -160,7 +160,8 @@ describe Gitlab::Ci::Config::Entry::Global do
                        cache: { key: 'k', untracked: true, paths: ['public/'], policy: 'pull-push' },
                        variables: { 'VAR' => 'value' },
                        ignore: false,
-                       after_script: ['make clean'] },
+                       after_script: ['make clean'],
+                       only: { refs: %w[branches tags] } },
               spinach: { name: :spinach,
                          before_script: [],
                          script: %w[spinach],
@@ -171,7 +172,8 @@ describe Gitlab::Ci::Config::Entry::Global do
                          cache: { key: 'k', untracked: true, paths: ['public/'], policy: 'pull-push' },
                          variables: {},
                          ignore: false,
-                         after_script: ['make clean'] }
+                         after_script: ['make clean'],
+                         only: { refs: %w[branches tags] } }
             )
           end
         end

spec/lib/gitlab/ci/config/entry/job_spec.rb

@@ -258,7 +258,8 @@ describe Gitlab::Ci::Config::Entry::Job do
                    commands: "ls\npwd\nrspec",
                    stage: 'test',
                    ignore: false,
-                   after_script: %w[cleanup])
+                   after_script: %w[cleanup],
+                   only: { refs: %w[branches tags] })
         end
       end
     end

spec/lib/gitlab/ci/config/entry/jobs_spec.rb

@@ -67,12 +67,14 @@ describe Gitlab::Ci::Config::Entry::Jobs do
                    script: %w[rspec],
                    commands: 'rspec',
                    ignore: false,
-                   stage: 'test' },
+                   stage: 'test',
+                   only: { refs: %w[branches tags] } },
           spinach: { name: :spinach,
                      script: %w[spinach],
                      commands: 'spinach',
                      ignore: false,
-                     stage: 'test' })
+                     stage: 'test',
+                     only: { refs: %w[branches tags] } })
       end
     end
 

spec/lib/gitlab/ci/config/entry/only_policy_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Ci::Config::Entry::OnlyPolicy do
+  let(:entry) { described_class.new(config) }
+
+  it_behaves_like 'correct only except policy'
+
+  describe '.default' do
+    it 'haa a default value' do
+      expect(described_class.default).to eq( { refs: %w[branches tags] } )
+    end
+  end
+end

spec/lib/gitlab/ci/config/entry/policy_spec.rb

-require 'fast_spec_helper'
-require_dependency 'active_model'
+require 'spec_helper'
 
 describe Gitlab::Ci::Config::Entry::Policy do
   let(:entry) { described_class.new(config) }
 
-  context 'when using simplified policy' do
-    describe 'validations' do
-      context 'when entry config value is valid' do
-        context 'when config is a branch or tag name' do
-          let(:config) { %w[master feature/branch] }
-
-          describe '#valid?' do
-            it 'is valid' do
-              expect(entry).to be_valid
-            end
-          end
-
-          describe '#value' do
-            it 'returns refs hash' do
-              expect(entry.value).to eq(refs: config)
-            end
-          end
-        end
-
-        context 'when config is a regexp' do
-          let(:config) { ['/^issue-.*$/'] }
-
-          describe '#valid?' do
-            it 'is valid' do
-              expect(entry).to be_valid
-            end
-          end
-        end
-
-        context 'when config is a special keyword' do
-          let(:config) { %w[tags triggers branches] }
-
-          describe '#valid?' do
-            it 'is valid' do
-              expect(entry).to be_valid
-            end
-          end
-        end
-      end
-
-      context 'when entry value is not valid' do
-        let(:config) { [1] }
-
-        describe '#errors' do
-          it 'saves errors' do
-            expect(entry.errors)
-              .to include /policy config should be an array of strings or regexps/
-          end
-        end
-      end
-    end
-  end
-
-  context 'when using complex policy' do
-    context 'when specifying refs policy' do
-      let(:config) { { refs: ['master'] } }
-
-      it 'is a correct configuraton' do
-        expect(entry).to be_valid
-        expect(entry.value).to eq(refs: %w[master])
-      end
-    end
-
-    context 'when specifying kubernetes policy' do
-      let(:config) { { kubernetes: 'active' } }
-
-      it 'is a correct configuraton' do
-        expect(entry).to be_valid
-        expect(entry.value).to eq(kubernetes: 'active')
-      end
-    end
-
-    context 'when specifying invalid kubernetes policy' do
-      let(:config) { { kubernetes: 'something' } }
-
-      it 'reports an error about invalid policy' do
-        expect(entry.errors).to include /unknown value: something/
-      end
-    end
-
-    context 'when specifying valid variables expressions policy' do
-      let(:config) { { variables: ['$VAR == null'] } }
-
-      it 'is a correct configuraton' do
-        expect(entry).to be_valid
-        expect(entry.value).to eq(config)
-      end
-    end
-
-    context 'when specifying variables expressions in invalid format' do
-      let(:config) { { variables: '$MY_VAR' } }
-
-      it 'reports an error about invalid format' do
-        expect(entry.errors).to include /should be an array of strings/
-      end
-    end
-
-    context 'when specifying invalid variables expressions statement' do
-      let(:config) { { variables: ['$MY_VAR =='] } }
-
-      it 'reports an error about invalid statement' do
-        expect(entry.errors).to include /invalid expression syntax/
-      end
-    end
-
-    context 'when specifying invalid variables expressions token' do
-      let(:config) { { variables: ['$MY_VAR == 123'] } }
-
-      it 'reports an error about invalid expression' do
-        expect(entry.errors).to include /invalid expression syntax/
-      end
-    end
-
-    context 'when using invalid variables expressions regexp' do
-      let(:config) { { variables: ['$MY_VAR =~ /some ( thing/'] } }
-
-      it 'reports an error about invalid expression' do
-        expect(entry.errors).to include /invalid expression syntax/
-      end
-    end
-
-    context 'when specifying a valid changes policy' do
-      let(:config) { { changes: %w[some/* paths/**/*.rb] } }
-
-      it 'is a correct configuraton' do
-        expect(entry).to be_valid
-        expect(entry.value).to eq(config)
-      end
-    end
-
-    context 'when changes policy is invalid' do
-      let(:config) { { changes: [1, 2] } }
-
-      it 'returns errors' do
-        expect(entry.errors).to include /changes should be an array of strings/
-      end
-    end
-
-    context 'when specifying unknown policy' do
-      let(:config) { { refs: ['master'], invalid: :something } }
-
-      it 'returns error about invalid key' do
-        expect(entry.errors).to include /unknown keys: invalid/
-      end
-    end
-
-    context 'when policy is empty' do
-      let(:config) { {} }
-
-      it 'is not a valid configuration' do
-        expect(entry.errors).to include /can't be blank/
-      end
-    end
-  end
-
-  context 'when policy strategy does not match' do
-    let(:config) { 'string strategy' }
-
-    it 'returns information about errors' do
-      expect(entry.errors)
-        .to include /has to be either an array of conditions or a hash/
-    end
-  end
-
   describe '.default' do
     it 'does not have a default value' do
       expect(described_class.default).to be_nil

spec/support/shared_examples/only_except_policy_examples.rb

+# frozen_string_literal: true
+
+shared_examples 'correct only except policy' do
+  context 'when using simplified policy' do
+    describe 'validations' do
+      context 'when entry config value is valid' do
+        context 'when config is a branch or tag name' do
+          let(:config) { %w[master feature/branch] }
+
+          describe '#valid?' do
+            it 'is valid' do
+              expect(entry).to be_valid
+            end
+          end
+
+          describe '#value' do
+            it 'returns refs hash' do
+              expect(entry.value).to eq(refs: config)
+            end
+          end
+        end
+
+        context 'when config is a regexp' do
+          let(:config) { ['/^issue-.*$/'] }
+
+          describe '#valid?' do
+            it 'is valid' do
+              expect(entry).to be_valid
+            end
+          end
+        end
+
+        context 'when config is a special keyword' do
+          let(:config) { %w[tags triggers branches] }
+
+          describe '#valid?' do
+            it 'is valid' do
+              expect(entry).to be_valid
+            end
+          end
+        end
+      end
+
+      context 'when entry value is not valid' do
+        let(:config) { [1] }
+
+        describe '#errors' do
+          it 'saves errors' do
+            expect(entry.errors)
+              .to include /policy config should be an array of strings or regexps/
+          end
+        end
+      end
+    end
+  end
+
+  context 'when using complex policy' do
+    context 'when specifying refs policy' do
+      let(:config) { { refs: ['master'] } }
+
+      it 'is a correct configuraton' do
+        expect(entry).to be_valid
+        expect(entry.value).to eq(refs: %w[master])
+      end
+    end
+
+    context 'when specifying kubernetes policy' do
+      let(:config) { { kubernetes: 'active' } }
+
+      it 'is a correct configuraton' do
+        expect(entry).to be_valid
+        expect(entry.value).to eq(kubernetes: 'active')
+      end
+    end
+
+    context 'when specifying invalid kubernetes policy' do
+      let(:config) { { kubernetes: 'something' } }
+
+      it 'reports an error about invalid policy' do
+        expect(entry.errors).to include /unknown value: something/
+      end
+    end
+
+    context 'when specifying valid variables expressions policy' do
+      let(:config) { { variables: ['$VAR == null'] } }
+
+      it 'is a correct configuraton' do
+        expect(entry).to be_valid
+        expect(entry.value).to eq(config)
+      end
+    end
+
+    context 'when specifying variables expressions in invalid format' do
+      let(:config) { { variables: '$MY_VAR' } }
+
+      it 'reports an error about invalid format' do
+        expect(entry.errors).to include /should be an array of strings/
+      end
+    end
+
+    context 'when specifying invalid variables expressions statement' do
+      let(:config) { { variables: ['$MY_VAR =='] } }
+
+      it 'reports an error about invalid statement' do
+        expect(entry.errors).to include /invalid expression syntax/
+      end
+    end
+
+    context 'when specifying invalid variables expressions token' do
+      let(:config) { { variables: ['$MY_VAR == 123'] } }
+
+      it 'reports an error about invalid expression' do
+        expect(entry.errors).to include /invalid expression syntax/
+      end
+    end
+
+    context 'when using invalid variables expressions regexp' do
+      let(:config) { { variables: ['$MY_VAR =~ /some ( thing/'] } }
+
+      it 'reports an error about invalid expression' do
+        expect(entry.errors).to include /invalid expression syntax/
+      end
+    end
+
+    context 'when specifying a valid changes policy' do
+      let(:config) { { changes: %w[some/* paths/**/*.rb] } }
+
+      it 'is a correct configuraton' do
+        expect(entry).to be_valid
+        expect(entry.value).to eq(config)
+      end
+    end
+
+    context 'when changes policy is invalid' do
+      let(:config) { { changes: [1, 2] } }
+
+      it 'returns errors' do
+        expect(entry.errors).to include /changes should be an array of strings/
+      end
+    end
+
+    context 'when specifying unknown policy' do
+      let(:config) { { refs: ['master'], invalid: :something } }
+
+      it 'returns error about invalid key' do
+        expect(entry.errors).to include /unknown keys: invalid/
+      end
+    end
+
+    context 'when policy is empty' do
+      let(:config) { {} }
+
+      it 'is not a valid configuration' do
+        expect(entry.errors).to include /can't be blank/
+      end
+    end
+  end
+
+  context 'when policy strategy does not match' do
+    let(:config) { 'string strategy' }
+
+    it 'returns information about errors' do
+      expect(entry.errors)
+        .to include /has to be either an array of conditions or a hash/
+    end
+  end
+end