Skip to content

G
gitlab-ce
Commit 250fede4 authored by Pedro Pombeiro's avatar Pedro Pombeiro
Browse files
Options

Address MR review comments

parent d565ff04
Expand all Show whitespace changes
Inline Side-by-side
Showing with 510 additions and 683 deletions
lib/expand_variables.rb
View file @ 250fede4
......@@ -16,18 +16,6 @@ module ExpandVariables
end
end
# expand_variables_collection expands a Gitlab::Ci::Variables::Collection, ignoring unknown variable references.
# If a circular variable reference is found, the original Collection is returned
def expand_variables_collection(variables, project)
return Gitlab::Ci::Variables::Collection.new(variables) if
Feature.disabled?(:variable_inside_variable, project)
sorted_variables = variables.sorted_collection(project)
return sorted_variables if sorted_variables.errors
expand_sorted_variables_collection(sorted_variables)
end
def possible_var_reference?(value)
return unless value
......@@ -74,16 +62,5 @@ module ExpandVariables
variables
end
def expand_sorted_variables_collection(sorted_variables)
expanded_vars = {}
sorted_variables.each_with_object(Gitlab::Ci::Variables::Collection.new) do |item, collection|
item = item.merge(value: expand_existing(item.value, expanded_vars)) if item.depends_on
expanded_vars.store(item[:key], item)
collection.append(item)
end
end
end
end
lib/gitlab/ci/pipeline/seed/build.rb
View file @ 250fede4
......@@ -162,8 +162,8 @@ module Gitlab
end
def variable_expansion_errors
sorted_collection = evaluate_context.variables.sorted_collection(@pipeline.project)
errors = sorted_collection.errors
expanded_collection = evaluate_context.variables.expand_all(@pipeline.project)
errors = expanded_collection.errors
["#{name}: #{errors}"] if errors
end
......
lib/gitlab/ci/variables/collection.rb
View file @ 250fede4
......@@ -63,16 +63,36 @@ module Gitlab
Collection.new(@variables.reject(&block))
end
def ==(other)
return @variables == other if other.is_a?(Array)
return false unless other.class == self.class
def expand_value(value, keep_undefined: false)
value.gsub(ExpandVariables::VARIABLES_REGEXP) do
match = Regexp.last_match
result = @variables_by_key[match[1] || match[2]]&.value
result ||= match[0] if keep_undefined
result
end
end
def expand_all(project, keep_undefined: false)
return self if Feature.disabled?(:variable_inside_variable, project)
sorted = Sort.new(self)
return self.class.new(self, sorted.errors) unless sorted.valid?
new_collection = self.class.new
sorted.tsort.each do |item|
unless item.depends_on
new_collection.append(item)
next
end
@variables == other.variables
# expand variables as they are added
variable = item.to_runner_variable
variable[:value] = new_collection.expand_value(variable[:value], keep_undefined: keep_undefined)
new_collection.append(variable)
end
# Returns a sorted Collection object, and sets errors property in case of an error
def sorted_collection(project)
Sort.new(self, project).collection
new_collection
end
protected
......
lib/gitlab/ci/variables/collection/item.rb
View file @ 250fede4
......@@ -7,26 +7,23 @@ module Gitlab
class Item
include Gitlab::Utils::StrongMemoize
attr_reader :raw
def initialize(key:, value:, public: true, file: false, masked: false, raw: false)
raise ArgumentError, "`#{key}` must be of type String or nil value, while it was: #{value.class}" unless
value.is_a?(String) || value.nil?
@variable = { key: key, value: value, public: public, file: file, masked: masked }
@raw = raw
@variable = { key: key, value: value, public: public, file: file, masked: masked, raw: raw }
end
def value
@variable.fetch(:value)
end
def [](key)
@variable.fetch(key)
def raw
@variable.fetch(:raw)
end
def merge(*other_hashes)
self.class.fabricate(@variable.merge(*other_hashes))
def [](key)
@variable.fetch(key)
end
def ==(other)
......@@ -50,7 +47,7 @@ module Gitlab
#
def to_runner_variable
@variable.reject do |hash_key, hash_value|
hash_key == :file && hash_value == false
(hash_key == :file || hash_key == :raw) && hash_value == false
end
end
......
lib/gitlab/ci/variables/collection/sort.rb
View file @ 250fede4
......@@ -8,12 +8,11 @@ module Gitlab
include TSort
include Gitlab::Utils::StrongMemoize
def initialize(collection, project)
def initialize(collection)
raise(ArgumentError, "A Gitlab::Ci::Variables::Collection object was expected") unless
collection.is_a?(Collection)
@collection = collection
@project = project
end
def valid?
......@@ -23,8 +22,6 @@ module Gitlab
# errors sorts an array of variables, ignoring unknown variable references,
# and returning an error string if a circular variable reference is found
def errors
return if Feature.disabled?(:variable_inside_variable, @project)
strong_memoize(:errors) do
# Check for cyclic dependencies and build error message in that case
cyclic_vars = each_strongly_connected_component.filter_map do |component|
......@@ -35,16 +32,6 @@ module Gitlab
end
end
# collection sorts a collection of variables, ignoring unknown variable references.
# If a circular variable reference is found, a new collection with the original array and an error is returned
def collection
return @collection if Feature.disabled?(:variable_inside_variable, @project)
return Gitlab::Ci::Variables::Collection.new(@collection, errors) if errors
Gitlab::Ci::Variables::Collection.new(tsort)
end
private
def tsort_each_node(&block)
......
spec/lib/expand_variables_spec.rb
View file @ 250fede4
......@@ -268,236 +268,4 @@ RSpec.describe ExpandVariables do
end
end
end
describe '#expand_variables_collection' do
context 'when FF :variable_inside_variable is disabled' do
let_it_be(:project_with_flag_disabled) { create(:project) }
let_it_be(:project_with_flag_enabled) { create(:project) }
before do
stub_feature_flags(variable_inside_variable: [project_with_flag_enabled])
end
context 'table tests' do
using RSpec::Parameterized::TableSyntax
where do
{
"empty array": {
variables: []
},
"simple expansions": {
variables: [
{ key: 'variable', value: 'value' },
{ key: 'variable2', value: 'result' },
{ key: 'variable3', value: 'key$variable$variable2' }
]
},
"complex expansion": {
variables: [
{ key: 'variable', value: 'value' },
{ key: 'variable2', value: 'key${variable}' }
]
},
"out-of-order variable reference": {
variables: [
{ key: 'variable2', value: 'key${variable}' },
{ key: 'variable', value: 'value' }
]
},
"complex expansions with raw variable": {
variables: [
{ key: 'variable3', value: 'key_${variable}_${variable2}' },
{ key: 'variable', value: '$variable2', raw: true },
{ key: 'variable2', value: 'value2' }
]
},
"array with cyclic dependency": {
variables: [
{ key: 'variable', value: '$variable2' },
{ key: 'variable2', value: '$variable3' },
{ key: 'variable3', value: 'key$variable$variable2' }
]
}
}
end
with_them do
subject { ExpandVariables.expand_variables_collection(variables, project_with_flag_disabled) }
it 'returns Collection' do
is_expected.to be_an_instance_of(Gitlab::Ci::Variables::Collection)
end
it 'does not expand variables' do
is_expected.to eq(variables)
end
end
end
end
context 'when FF :variable_inside_variable is enabled' do
let_it_be(:project_with_flag_disabled) { create(:project) }
let_it_be(:project_with_flag_enabled) { create(:project) }
before do
stub_feature_flags(variable_inside_variable: [project_with_flag_enabled])
end
context 'table tests' do
using RSpec::Parameterized::TableSyntax
where do
{
"empty array": {
variables: [],
result: []
},
"simple expansions": {
variables: [
{ key: 'variable', value: 'value' },
{ key: 'variable2', value: 'result' },
{ key: 'variable3', value: 'key$variable$variable2' },
{ key: 'variable4', value: 'key$variable$variable3' }
],
result: [
{ key: 'variable', value: 'value' },
{ key: 'variable2', value: 'result' },
{ key: 'variable3', value: 'keyvalueresult' },
{ key: 'variable4', value: 'keyvaluekeyvalueresult' }
]
},
"complex expansion": {
variables: [
{ key: 'variable', value: 'value' },
{ key: 'variable2', value: 'key${variable}' }
],
result: [
{ key: 'variable', value: 'value' },
{ key: 'variable2', value: 'keyvalue' }
]
},
"unused variables": {
variables: [
{ key: 'variable', value: 'value' },
{ key: 'variable2', value: 'result2' },
{ key: 'variable3', value: 'result3' },
{ key: 'variable4', value: 'key$variable$variable3' }
],
result: [
{ key: 'variable', value: 'value' },
{ key: 'variable2', value: 'result2' },
{ key: 'variable3', value: 'result3' },
{ key: 'variable4', value: 'keyvalueresult3' }
]
},
"complex expansions": {
variables: [
{ key: 'variable', value: 'value' },
{ key: 'variable2', value: 'result' },
{ key: 'variable3', value: 'key${variable}${variable2}' }
],
result: [
{ key: 'variable', value: 'value' },
{ key: 'variable2', value: 'result' },
{ key: 'variable3', value: 'keyvalueresult' }
]
},
"out-of-order expansion": {
variables: [
{ key: 'variable3', value: 'key$variable2$variable' },
{ key: 'variable', value: 'value' },
{ key: 'variable2', value: 'result' }
],
result: [
{ key: 'variable2', value: 'result' },
{ key: 'variable', value: 'value' },
{ key: 'variable3', value: 'keyresultvalue' }
]
},
"out-of-order complex expansion": {
variables: [
{ key: 'variable', value: 'value' },
{ key: 'variable2', value: 'result' },
{ key: 'variable3', value: 'key${variable2}${variable}' }
],
result: [
{ key: 'variable', value: 'value' },
{ key: 'variable2', value: 'result' },
{ key: 'variable3', value: 'keyresultvalue' }
]
},
"missing variable": {
variables: [
{ key: 'variable2', value: 'key$variable' }
],
result: [
{ key: 'variable2', value: 'key$variable' }
]
},
"complex expansions with missing variable": {
variables: [
{ key: 'variable4', value: 'key${variable}${variable2}${variable3}' },
{ key: 'variable', value: 'value' },
{ key: 'variable3', value: 'value3' }
],
result: [
{ key: 'variable', value: 'value' },
{ key: 'variable3', value: 'value3' },
{ key: 'variable4', value: 'keyvalue${variable2}value3' }
]
},
"complex expansions with raw variable": {
variables: [
{ key: 'variable3', value: 'key_${variable}_${variable2}' },
{ key: 'variable', value: '$variable2', raw: true },
{ key: 'variable2', value: 'value2' }
],
result: [
{ key: 'variable', value: '$variable2', raw: true },
{ key: 'variable2', value: 'value2' },
{ key: 'variable3', value: 'key_$variable2_value2' }
]
},
"cyclic dependency causes original array to be returned": {
variables: [
{ key: 'variable', value: '$variable2' },
{ key: 'variable2', value: '$variable3' },
{ key: 'variable3', value: 'key$variable$variable2' }
],
result: [
{ key: 'variable', value: '$variable2' },
{ key: 'variable2', value: '$variable3' },
{ key: 'variable3', value: 'key$variable$variable2' }
]
}
}
end
with_them do
let!(:collection) { Gitlab::Ci::Variables::Collection.new(variables) }
subject! do
ExpandVariables.expand_variables_collection(collection, project_with_flag_enabled)
end
it 'returns Collection' do
is_expected.to be_an_instance_of(Gitlab::Ci::Variables::Collection)
end
it 'expands variables' do
is_expected.to eq(result)
end
it 'preserves raw attribute' do
collection.each do |v|
k = v[:key]
subject_item = subject.find { |sv| sv[:key] == k }
expect(subject_item.raw).to eq(v.raw)
end
end
end
end
end
end
end
spec/lib/gitlab/ci/variables/collection/item_spec.rb
View file @ 250fede4
......@@ -180,16 +180,6 @@ RSpec.describe Gitlab::Ci::Variables::Collection::Item do
end
end
describe '#merge' do
it 'behaves like hash merge' do
item = described_class.new(**variable)
subject = item.merge(value: 'another thing')
expect(subject).not_to eq item
expect(subject[:value]).to eq 'another thing'
end
end
describe '#to_runner_variable' do
context 'when variable is not a file-related' do
it 'returns a runner-compatible hash representation' do
......@@ -212,6 +202,26 @@ RSpec.describe Gitlab::Ci::Variables::Collection::Item do
end
end
context 'when variable is raw' do
it 'does not export raw value when it is false' do
runner_variable = described_class
.new(key: 'VAR', value: 'value', raw: false)
.to_runner_variable
expect(runner_variable)
.to eq(key: 'VAR', value: 'value', public: true, masked: false)
end
it 'exports raw value when it is true' do
runner_variable = described_class
.new(key: 'VAR', value: 'value', raw: true)
.to_runner_variable
expect(runner_variable)
.to eq(key: 'VAR', value: 'value', public: true, raw: true, masked: false)
end
end
context 'when referencing a variable' do
it '#depends_on contains names of dependencies' do
runner_variable = described_class.new(key: 'CI_VAR', value: '${CI_VAR_2}-123-$CI_VAR_3')
......
spec/lib/gitlab/ci/variables/collection/sort_spec.rb
View file @ 250fede4
......@@ -4,15 +4,8 @@ require 'spec_helper'
RSpec.describe Gitlab::Ci::Variables::Collection::Sort do
describe '#initialize with non-Collection value' do
let_it_be(:project_with_flag_disabled) { create(:project) }
let_it_be(:project_with_flag_enabled) { create(:project) }
before do
stub_feature_flags(variable_inside_variable: [project_with_flag_enabled])
end
context 'when FF :variable_inside_variable is disabled' do
subject { Gitlab::Ci::Variables::Collection::Sort.new([], project_with_flag_disabled) }
subject { Gitlab::Ci::Variables::Collection::Sort.new([]) }
it 'raises ArgumentError' do
expect { subject }.to raise_error(ArgumentError, /Collection object was expected/)
......@@ -20,7 +13,7 @@ RSpec.describe Gitlab::Ci::Variables::Collection::Sort do
end
context 'when FF :variable_inside_variable is enabled' do
subject { Gitlab::Ci::Variables::Collection::Sort.new([], project_with_flag_enabled) }
subject { Gitlab::Ci::Variables::Collection::Sort.new([]) }
it 'raises ArgumentError' do
expect { subject }.to raise_error(ArgumentError, /Collection object was expected/)
......@@ -29,88 +22,6 @@ RSpec.describe Gitlab::Ci::Variables::Collection::Sort do
end
describe '#errors' do
context 'when FF :variable_inside_variable is disabled' do
let_it_be(:project_with_flag_disabled) { create(:project) }
let_it_be(:project_with_flag_enabled) { create(:project) }
before do
stub_feature_flags(variable_inside_variable: [project_with_flag_enabled])
end
context 'table tests' do
using RSpec::Parameterized::TableSyntax
where do
{
"empty array": {
variables: []
},
"simple expansions": {
variables: [
{ key: 'variable', value: 'value' },
{ key: 'variable2', value: 'result' },
{ key: 'variable3', value: 'key$variable$variable2' }
]
},
"complex expansion": {
variables: [
{ key: 'variable', value: 'value' },
{ key: 'variable2', value: 'key${variable}' }
]
},
"complex expansions with missing variable for Windows": {
variables: [
{ key: 'variable', value: 'value' },
{ key: 'variable3', value: 'key%variable%%variable2%' }
]
},
"out-of-order variable reference": {
variables: [
{ key: 'variable2', value: 'key${variable}' },
{ key: 'variable', value: 'value' }
]
},
"array with cyclic dependency": {
variables: [
{ key: 'variable', value: '$variable2' },
{ key: 'variable2', value: '$variable3' },
{ key: 'variable3', value: 'key$variable$variable2' }
]
},
"array with raw variable": {
variables: [
{ key: 'variable', value: '$variable2' },
{ key: 'variable2', value: '$variable3' },
{ key: 'variable3', value: 'key$variable$variable2', raw: true }
]
}
}
end
with_them do
let(:collection) { Gitlab::Ci::Variables::Collection.new(variables) }
subject { Gitlab::Ci::Variables::Collection::Sort.new(collection, project_with_flag_disabled) }
it 'does not report error' do
expect(subject.errors).to eq(nil)
end
it 'valid? reports true' do
expect(subject.valid?).to eq(true)
end
end
end
end
context 'when FF :variable_inside_variable is enabled' do
let_it_be(:project_with_flag_disabled) { create(:project) }
let_it_be(:project_with_flag_enabled) { create(:project) }
before do
stub_feature_flags(variable_inside_variable: [project_with_flag_enabled])
end
context 'table tests' do
using RSpec::Parameterized::TableSyntax
......@@ -158,7 +69,7 @@ RSpec.describe Gitlab::Ci::Variables::Collection::Sort do
with_them do
let(:collection) { Gitlab::Ci::Variables::Collection.new(variables) }
subject { Gitlab::Ci::Variables::Collection::Sort.new(collection, project_with_flag_enabled) }
subject { Gitlab::Ci::Variables::Collection::Sort.new(collection) }
it 'errors matches expected validation result' do
expect(subject.errors).to eq(validation_result)
......@@ -167,79 +78,15 @@ RSpec.describe Gitlab::Ci::Variables::Collection::Sort do
it 'valid? matches expected validation result' do
expect(subject.valid?).to eq(validation_result.nil?)
end
end
end
end
end
describe '#collection' do
context 'when FF :variable_inside_variable is disabled' do
before do
stub_feature_flags(variable_inside_variable: false)
end
context 'table tests' do
using RSpec::Parameterized::TableSyntax
where do
{
"empty array": {
variables: []
},
"simple expansions": {
variables: [
{ key: 'variable', value: 'value' },
{ key: 'variable2', value: 'result' },
{ key: 'variable3', value: 'key$variable$variable2' }
]
},
"complex expansion": {
variables: [
{ key: 'variable', value: 'value' },
{ key: 'variable2', value: 'key${variable}' }
]
},
"complex expansions with missing variable for Windows": {
variables: [
{ key: 'variable', value: 'value' },
{ key: 'variable3', value: 'key%variable%%variable2%' }
]
},
"out-of-order variable reference": {
variables: [
{ key: 'variable2', value: 'key${variable}' },
{ key: 'variable', value: 'value' }
]
},
"array with cyclic dependency": {
variables: [
{ key: 'variable', value: '$variable2' },
{ key: 'variable2', value: '$variable3' },
{ key: 'variable3', value: 'key$variable$variable2' }
]
}
}
end
with_them do
let_it_be(:project) { create(:project) }
let(:collection) { Gitlab::Ci::Variables::Collection.new(variables) }
subject { Gitlab::Ci::Variables::Collection::Sort.new(collection, project).collection }
it 'does not expand variables' do
is_expected.to be(collection)
it 'does not raise' do
expect { subject }.not_to raise_error
end
end
end
end
context 'when FF :variable_inside_variable is enabled' do
before do
stub_licensed_features(group_saml_group_sync: true)
stub_feature_flags(variable_inside_variable: true)
end
describe '#tsort' do
context 'table tests' do
using RSpec::Parameterized::TableSyntax
......@@ -287,14 +134,6 @@ RSpec.describe Gitlab::Ci::Variables::Collection::Sort do
],
result: %w[variable variable3 variable4]
},
"cyclic dependency causes original array to be returned": {
variables: [
{ key: 'variable2', value: '$variable3' },
{ key: 'variable3', value: 'key$variable$variable2' },
{ key: 'variable', value: '$variable2' }
],
result: %w[variable2 variable3 variable]
},
"raw variable does not get resolved": {
variables: [
{ key: 'variable', value: '$variable2' },
......@@ -315,16 +154,32 @@ RSpec.describe Gitlab::Ci::Variables::Collection::Sort do
end
with_them do
let_it_be(:project) { create(:project) }
let(:collection) { Gitlab::Ci::Variables::Collection.new(variables) }
subject { Gitlab::Ci::Variables::Collection::Sort.new(collection, project).collection }
subject { Gitlab::Ci::Variables::Collection::Sort.new(collection).tsort }
it 'returns correctly sorted variables' do
expect(subject.map { |var| var[:key] }).to eq(result)
end
end
end
context 'cyclic dependency' do
let(:variables) do
[
{ key: 'variable2', value: '$variable3' },
{ key: 'variable3', value: 'key$variable$variable2' },
{ key: 'variable', value: '$variable2' }
]
end
let(:collection) { Gitlab::Ci::Variables::Collection.new(variables) }
subject { Gitlab::Ci::Variables::Collection::Sort.new(collection).tsort }
it 'raises TSort::Cyclic' do
expect { subject }.to raise_error(TSort::Cyclic)
end
end
end
end
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7