Merge branch 'allow-to-fine-tune-what-to-inherit' into 'master'

Allow to fine tune what to inherit

See merge request gitlab-org/gitlab!26483

doc/ci/yaml/README.md

@@ -165,33 +165,79 @@ rspec 2.6:
 You can disable inheritance of globally defined defaults
 and variables with the `inherit:` parameter.
 
+To enable or disable the inheritance of all `variables:` or `default:` parameters, use the following format:
+
+- `default: true` or `default: false`
+- `variables: true` or `variables: false`
+
+To inherit only a subset of `default:` parameters or `variables:`, specify what
+you wish to inherit, and any not listed will **not** be inherited. Use
+one of the following formats:
+
+```yaml
+inherit:
+  default: [parameter1, parameter2]
+  variables: [VARIABLE1, VARIABLE2]
+```
+
+Or:
+
+```yaml
+inherit:
+  default:
+    - parameter1
+    - parameter2
+  variables:
+    - VARIABLE1
+    - VARIABLE2
+```
+
 In the example below:
 
-- `rubocop` **will** inherit both the `before_script` and the variable `DOMAIN`.
-- `rspec` **will not** inherit the `before_script` or the variable `DOMAIN`.
-- `capybara` **will** inherit the `before_script`, but **will not** inherit the variable `DOMAIN`.
+- `rubocop`:
+  - **will** inherit: Nothing.
+- `rspec`:
+  - **will** inherit: the default `image` and the `WEBHOOK_URL` variable.
+  - **will not** inherit: the default `before_script` and the `DOMAIN` variable.
+- `capybara`:
+  - **will** inherit: the default `before_script` and `image`.
+  - **will not** inherit: the `DOMAIN` and `WEBHOOK_URL` variables.
+- `karma`:
+  - **will** inherit: the default `image` and `before_script`, and the `DOMAIN` variable.
+  - **will not** inherit: `WEBHOOK_URL` variable.
 
 ```yaml
 default:
+  image: 'ruby:2.4'
   before_script:
     - echo Hello World
 
 variables:
   DOMAIN: example.com
+  WEBHOOK_URL: https://my-webhook.example.com
 
 rubocop:
+  inherit:
+    default: false
+    variables: false
   script: bundle exec rubocop
 
 rspec:
   inherit:
-    default: false
-    variables: false
+    default: [image]
+    variables: [WEBHOOK_URL]
   script: bundle exec rspec
 
 capybara:
   inherit:
     variables: false
   script: bundle exec capybara
+
+karma:
+  inherit:
+    default: true
+    variables: [DOMAIN]
+  script: karma
 ```
 
 ## Parameter details

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

@@ -16,11 +16,11 @@ module Gitlab
             validates :config, allowed_keys: ALLOWED_KEYS
           end
 
-          entry :default, ::Gitlab::Config::Entry::Boolean,
+          entry :default, ::Gitlab::Ci::Config::Entry::Inherit::Default,
             description: 'Indicates whether to inherit `default:`.',
             default: true
 
-          entry :variables, ::Gitlab::Config::Entry::Boolean,
+          entry :variables, ::Gitlab::Ci::Config::Entry::Inherit::Variables,
             description: 'Indicates whether to inherit `variables:`.',
             default: true
         end

lib/gitlab/ci/config/entry/inherit/default.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    class Config
+      module Entry
+        ##
+        # This class represents a default inherit entry
+        #
+        class Inherit
+          class Default < ::Gitlab::Config::Entry::Simplifiable
+            strategy :BooleanStrategy, if: -> (config) { [true, false].include?(config) }
+            strategy :ArrayStrategy, if: -> (config) { config.is_a?(Array) }
+
+            class BooleanStrategy < ::Gitlab::Config::Entry::Boolean
+              def inherit?(_key)
+                value
+              end
+            end
+
+            class ArrayStrategy < ::Gitlab::Config::Entry::Node
+              include ::Gitlab::Config::Entry::Validatable
+
+              ALLOWED_VALUES = ::Gitlab::Ci::Config::Entry::Default::ALLOWED_KEYS.map(&:to_s).freeze
+
+              validations do
+                validates :config, type: Array
+                validates :config, array_of_strings: true
+                validates :config, allowed_array_values: { in: ALLOWED_VALUES }
+              end
+
+              def inherit?(key)
+                value.include?(key.to_s)
+              end
+            end
+
+            class UnknownStrategy < ::Gitlab::Config::Entry::Node
+              def errors
+                ["#{location} should be a bool or array of strings"]
+              end
+
+              def inherit?(key)
+                false
+              end
+            end
+          end
+        end
+      end
+    end
+  end
+end

lib/gitlab/ci/config/entry/inherit/variables.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    class Config
+      module Entry
+        ##
+        # This class represents a variables inherit entry
+        #
+        class Inherit
+          class Variables < ::Gitlab::Config::Entry::Simplifiable
+            strategy :BooleanStrategy, if: -> (config) { [true, false].include?(config) }
+            strategy :ArrayStrategy, if: -> (config) { config.is_a?(Array) }
+
+            class BooleanStrategy < ::Gitlab::Config::Entry::Boolean
+              def inherit?(_key)
+                value
+              end
+            end
+
+            class ArrayStrategy < ::Gitlab::Config::Entry::Node
+              include ::Gitlab::Config::Entry::Validatable
+
+              validations do
+                validates :config, type: Array
+                validates :config, array_of_strings: true
+              end
+
+              def inherit?(key)
+                value.include?(key.to_s)
+              end
+            end
+
+            class UnknownStrategy < ::Gitlab::Config::Entry::Node
+              def errors
+                ["#{location} should be a bool or array of strings"]
+              end
+
+              def inherit?(key)
+                false
+              end
+            end
+          end
+        end
+      end
+    end
+  end
+end

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

@@ -94,7 +94,7 @@ module Gitlab
           end
 
           def overwrite_entry(deps, key, current_entry)
-            return unless inherit_entry&.default_value
+            return unless inherit_entry&.default_entry&.inherit?(key)
             return unless deps.default_entry
 
             deps.default_entry[key] unless current_entry.specified?
@@ -111,11 +111,12 @@ module Gitlab
           end
 
           def root_and_job_variables_value
-            if inherit_entry&.variables_value
-              @root_variables_value.to_h.merge(variables_value.to_h) # rubocop:disable Gitlab/ModuleWithInstanceVariables
-            else
-              variables_value.to_h
+            root_variables = @root_variables_value.to_h # rubocop:disable Gitlab/ModuleWithInstanceVariables
+            root_variables = root_variables.select do |key, _|
+              inherit_entry&.variables_entry&.inherit?(key)
             end
+
+            root_variables.merge(variables_value.to_h)
           end
         end
       end

spec/lib/gitlab/ci/config/entry/inherit/default_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ::Gitlab::Ci::Config::Entry::Inherit::Default do
+  using RSpec::Parameterized::TableSyntax
+
+  subject { described_class.new(config) }
+
+  context 'validations' do
+    where(:config, :valid) do
+      true        | true
+      false       | true
+      %w[image]   | true
+      %w[unknown] | false
+      %i[image]   | false
+      [true]      | false
+      "string"    | false
+    end
+
+    with_them do
+      it do
+        expect(subject.valid?).to eq(valid)
+      end
+    end
+  end
+
+  describe '#inherit?' do
+    where(:config, :inherit) do
+      true              | true
+      false             | false
+      %w[image]         | true
+      %w[before_script] | false
+    end
+
+    with_them do
+      it do
+        expect(subject.inherit?('image')).to eq(inherit)
+      end
+    end
+  end
+end

spec/lib/gitlab/ci/config/entry/inherit/variables_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ::Gitlab::Ci::Config::Entry::Inherit::Variables do
+  using RSpec::Parameterized::TableSyntax
+
+  subject { described_class.new(config) }
+
+  context 'validations' do
+    where(:config, :valid) do
+      true        | true
+      false       | true
+      %w[A]       | true
+      %w[A B]     | true
+      %i[image]   | true
+      [true]      | false
+      "string"    | false
+    end
+
+    with_them do
+      it do
+        expect(subject.valid?).to eq(valid)
+      end
+    end
+  end
+
+  describe '#inherit?' do
+    where(:config, :inherit) do
+      true  | true
+      false | false
+      %w[A] | true
+      %w[B] | false
+    end
+
+    with_them do
+      it do
+        expect(subject.inherit?('A')).to eq(inherit)
+      end
+    end
+  end
+end

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

@@ -18,7 +18,7 @@ describe Gitlab::Ci::Config::Entry::Job do
     end
 
     before do
-      allow(entry).to receive_message_chain(:inherit_entry, :default_value).and_return(true)
+      allow(entry).to receive_message_chain(:inherit_entry, :default_entry, :inherit?).and_return(true)
     end
   end
 

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

@@ -269,13 +269,13 @@ describe Gitlab::Ci::Config::Entry::Processable do
         context 'when root yaml variables are used' do
           let(:variables) do
             Gitlab::Ci::Config::Entry::Variables.new(
-              A: 'root', C: 'root'
+              A: 'root', C: 'root', D: 'root'
             ).value
           end
 
           it 'does return all variables and overwrite them' do
             expect(entry.value).to include(
-              variables: { 'A' => 'job', 'B' => 'job', 'C' => 'root' }
+              variables: { 'A' => 'job', 'B' => 'job', 'C' => 'root', 'D' => 'root' }
             )
           end
 
@@ -293,32 +293,61 @@ describe Gitlab::Ci::Config::Entry::Processable do
               )
             end
           end
+
+          context 'when inherit of only specific variable is enabled' do
+            let(:config) do
+              {
+                variables: { A: 'job', B: 'job' },
+                inherit: { variables: ['D'] }
+              }
+            end
+
+            it 'does return only job variables' do
+              expect(entry.value).to include(
+                variables: { 'A' => 'job', 'B' => 'job', 'D' => 'root' }
+              )
+            end
+          end
         end
       end
 
       context 'of default:tags' do
         using RSpec::Parameterized::TableSyntax
 
-        where(:default_tags, :tags, :inherit_default, :result) do
-          nil | %w[a b] | nil | %w[a b]
-          nil | %w[a b] | true | %w[a b]
-          nil | %w[a b] | false | %w[a b]
-          %w[b c] | %w[a b] | nil | %w[a b]
-          %w[b c] | %w[a b] | true | %w[a b]
-          %w[b c] | %w[a b] | false | %w[a b]
-          %w[b c] | nil | nil | %w[b c]
-          %w[b c] | nil | true | %w[b c]
-          %w[b c] | nil | false | nil
+        where(:name, :default_tags, :tags, :inherit_default, :result) do
+          "only local tags"       | nil     | %w[a b] | nil       | %w[a b]
+          "only local tags"       | nil     | %w[a b] | true      | %w[a b]
+          "only local tags"       | nil     | %w[a b] | false     | %w[a b]
+          "global and local tags" | %w[b c] | %w[a b] | nil       | %w[a b]
+          "global and local tags" | %w[b c] | %w[a b] | true      | %w[a b]
+          "global and local tags" | %w[b c] | %w[a b] | false     | %w[a b]
+          "only global tags"      | %w[b c] | nil     | nil       | %w[b c]
+          "only global tags"      | %w[b c] | nil     | true      | %w[b c]
+          "only global tags"      | %w[b c] | nil     | false     | nil
+          "only global tags"      | %w[b c] | nil     | %w[image] | nil
+          "only global tags"      | %w[b c] | nil     | %w[tags]  | %w[b c]
         end
 
         with_them do
-          let(:config) { { tags: tags, inherit: { default: inherit_default } } }
-          let(:default_specified_tags) { double('tags', 'specified?' => true, 'valid?' => true, 'value' => default_tags) }
+          let(:config) do
+            { tags: tags,
+              inherit: { default: inherit_default } }
+          end
+
+          let(:default_specified_tags) do
+            double('tags',
+              'specified?' => true,
+              'valid?' => true,
+              'value' => default_tags,
+              'errors' => [])
+          end
 
           before do
             allow(default).to receive('[]').with(:tags).and_return(default_specified_tags)
 
             entry.compose!(deps)
+
+            expect(entry).to be_valid
           end
 
           it { expect(entry.tags_value).to eq(result) }

spec/lib/gitlab/ci/yaml_processor_spec.rb

@@ -515,6 +515,8 @@ module Gitlab
               nil | ["global script"]
               { default: false } | nil
               { default: true } | ["global script"]
+              { default: %w[before_script] } | ["global script"]
+              { default: %w[image] } | nil
             end
 
             with_them do
@@ -527,26 +529,28 @@ module Gitlab
 
               it { expect(subject[:options][:before_script]).to eq(result) }
             end
-          end
 
-          context "in default context" do
-            using RSpec::Parameterized::TableSyntax
+            context "in default context" do
+              using RSpec::Parameterized::TableSyntax
 
-            where(:inherit, :result) do
-              nil | ["global script"]
-              { default: false } | nil
-              { default: true } | ["global script"]
-            end
-
-            with_them do
-              let(:config) do
-                {
-                  default: { before_script: ["global script"] },
-                  test: { script: ["script"], inherit: inherit }
-                }
+              where(:inherit, :result) do
+                nil | ["global script"]
+                { default: false } | nil
+                { default: true } | ["global script"]
+                { default: %w[before_script] } | ["global script"]
+                { default: %w[image] } | nil
               end
 
-              it { expect(subject[:options][:before_script]).to eq(result) }
+              with_them do
+                let(:config) do
+                  {
+                    default: { before_script: ["global script"] },
+                    test: { script: ["script"], inherit: inherit }
+                  }
+                end
+
+                it { expect(subject[:options][:before_script]).to eq(result) }
+              end
             end
           end
 
@@ -845,6 +849,18 @@ module Gitlab
               )
             end
           end
+
+          context 'when specific variables are to inherited' do
+            let(:inherit) { { variables: %w[VAR1 VAR4] } }
+
+            it 'returns all unique variables and inherits only specified variables' do
+              expect(subject).to contain_exactly(
+                { key: 'VAR4', value: 'global4', public: true },
+                { key: 'VAR1', value: 'value1', public: true },
+                { key: 'VAR2', value: 'value2', public: true }
+              )
+            end
+          end
         end
 
         context 'when job variables are defined' do