Merge branch '30101-key-value-variables-only-for-workflow' into 'master'

Restrict the usage of value-description variables to root

See merge request gitlab-org/gitlab!48915

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

@@ -50,6 +50,7 @@ module Gitlab
 
           entry :variables, Entry::Variables,
             description: 'Environment variables that will be used.',
+            metadata: { use_value_data: true },
             reserved: true
 
           entry :stages, Entry::Stages,

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

@@ -13,7 +13,8 @@ module Gitlab
           ALLOWED_VALUE_DATA = %i[value description].freeze
 
           validations do
-            validates :config, variables: { allowed_value_data: ALLOWED_VALUE_DATA }
+            validates :config, variables: { allowed_value_data: ALLOWED_VALUE_DATA }, if: :use_value_data?
+            validates :config, variables: true, unless: :use_value_data?
           end
 
           def value
@@ -28,6 +29,10 @@ module Gitlab
             Hash[@config.map { |key, value| [key.to_s, expand_value(value)] }]
           end
 
+          def use_value_data?
+            opt(:use_value_data)
+          end
+
           private
 
           def expand_value(value)

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

@@ -32,7 +32,7 @@ RSpec.describe Gitlab::Ci::Config::Entry::Root do
           image: 'ruby:2.7',
           default: {},
           services: ['postgres:9.1', 'mysql:5.5'],
-          variables: { VAR: 'root' },
+          variables: { VAR: 'root', VAR2: { value: 'val 2', description: 'this is var 2' } },
           after_script: ['make clean'],
           stages: %w(build pages release),
           cache: { key: 'k', untracked: true, paths: ['public/'] },
@@ -80,6 +80,10 @@ RSpec.describe Gitlab::Ci::Config::Entry::Root do
             .to eq 'List of external YAML files to include.'
         end
 
+        it 'sets correct variables value' do
+          expect(root.variables_value).to eq('VAR' => 'root', 'VAR2' => 'val 2')
+        end
+
         describe '#leaf?' do
           it 'is not leaf' do
             expect(root).not_to be_leaf
@@ -128,7 +132,7 @@ RSpec.describe Gitlab::Ci::Config::Entry::Root do
                        services: [{ name: 'postgres:9.1' }, { name: 'mysql:5.5' }],
                        stage: 'test',
                        cache: { key: 'k', untracked: true, paths: ['public/'], policy: 'pull-push', when: 'on_success' },
-                       variables: { 'VAR' => 'root' },
+                       variables: { 'VAR' => 'root', 'VAR2' => 'val 2' },
                        ignore: false,
                        after_script: ['make clean'],
                        only: { refs: %w[branches tags] },
@@ -142,7 +146,7 @@ RSpec.describe Gitlab::Ci::Config::Entry::Root do
                          services: [{ name: 'postgres:9.1' }, { name: 'mysql:5.5' }],
                          stage: 'test',
                          cache: { key: 'k', untracked: true, paths: ['public/'], policy: 'pull-push', when: 'on_success' },
-                         variables: { 'VAR' => 'root' },
+                         variables: { 'VAR' => 'root', 'VAR2' => 'val 2' },
                          ignore: false,
                          after_script: ['make clean'],
                          only: { refs: %w[branches tags] },
@@ -158,7 +162,7 @@ RSpec.describe Gitlab::Ci::Config::Entry::Root do
                          services: [{ name: "postgres:9.1" }, { name: "mysql:5.5" }],
                          cache: { key: "k", untracked: true, paths: ["public/"], policy: "pull-push", when: 'on_success' },
                          only: { refs: %w(branches tags) },
-                         variables: { 'VAR' => 'job' },
+                         variables: { 'VAR' => 'job', 'VAR2' => 'val 2' },
                          after_script: [],
                          ignore: false,
                          scheduling_type: :stage }

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

@@ -3,7 +3,9 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Ci::Config::Entry::Variables do
-  subject { described_class.new(config) }
+  let(:metadata) { {} }
+
+  subject { described_class.new(config, metadata) }
 
   shared_examples 'valid config' do
     describe '#value' do
@@ -71,7 +73,13 @@ RSpec.describe Gitlab::Ci::Config::Entry::Variables do
       { 'VARIABLE_1' => 'value 1', 'VARIABLE_2' => 'value 2' }
     end
 
-    it_behaves_like 'valid config'
+    it_behaves_like 'invalid config'
+
+    context 'when metadata has use_value_data' do
+      let(:metadata) { { use_value_data: true } }
+
+      it_behaves_like 'valid config'
+    end
   end
 
   context 'when entry value is an array' do
@@ -80,32 +88,36 @@ RSpec.describe Gitlab::Ci::Config::Entry::Variables do
     it_behaves_like 'invalid config'
   end
 
-  context 'when entry value has hash with other key-pairs' do
-    let(:config) do
-      { 'VARIABLE_1' => { value: 'value 1', hello: 'variable 1' },
-        'VARIABLE_2' => 'value 2' }
-    end
+  context 'when metadata has use_value_data' do
+    let(:metadata) { { use_value_data: true } }
 
-    it_behaves_like 'invalid config'
-  end
+    context 'when entry value has hash with other key-pairs' do
+      let(:config) do
+        { 'VARIABLE_1' => { value: 'value 1', hello: 'variable 1' },
+          'VARIABLE_2' => 'value 2' }
+      end
 
-  context 'when entry config value has hash with nil description' do
-    let(:config) do
-      { 'VARIABLE_1' => { value: 'value 1', description: nil } }
+      it_behaves_like 'invalid config'
     end
 
-    it_behaves_like 'invalid config'
-  end
+    context 'when entry config value has hash with nil description' do
+      let(:config) do
+        { 'VARIABLE_1' => { value: 'value 1', description: nil } }
+      end
 
-  context 'when entry config value has hash without description' do
-    let(:config) do
-      { 'VARIABLE_1' => { value: 'value 1' } }
+      it_behaves_like 'invalid config'
     end
 
-    let(:result) do
-      { 'VARIABLE_1' => 'value 1' }
-    end
+    context 'when entry config value has hash without description' do
+      let(:config) do
+        { 'VARIABLE_1' => { value: 'value 1' } }
+      end
 
-    it_behaves_like 'valid config'
+      let(:result) do
+        { 'VARIABLE_1' => 'value 1' }
+      end
+
+      it_behaves_like 'valid config'
+    end
   end
 end