diff --git a/.rubocop_todo/rspec/verified_doubles.yml b/.rubocop_todo/rspec/verified_doubles.yml
index df5e30e94ed67e6e62f4ad0f2416b2234f291a5a..420b51316c334782d8632d72eba3af50f51a20e7 100644
--- a/.rubocop_todo/rspec/verified_doubles.yml
+++ b/.rubocop_todo/rspec/verified_doubles.yml
@@ -442,7 +442,6 @@ RSpec/VerifiedDoubles:
- spec/lib/gitlab/ci/build/policy/variables_spec.rb
- spec/lib/gitlab/ci/build/policy_spec.rb
- spec/lib/gitlab/ci/build/prerequisite/kubernetes_namespace_spec.rb
- - spec/lib/gitlab/ci/build/rules/rule/clause/changes_spec.rb
- spec/lib/gitlab/ci/build/rules/rule_spec.rb
- spec/lib/gitlab/ci/build/rules_spec.rb
- spec/lib/gitlab/ci/build/status/reason_spec.rb
diff --git a/lib/gitlab/ci/build/rules/rule/clause/changes.rb b/lib/gitlab/ci/build/rules/rule/clause/changes.rb
index 4c5f02b4f7b56e8a7ae6e149bf1487d70da45f2e..6eba3160249df1f7e9fdbb993c7e1946ab95486a 100644
--- a/lib/gitlab/ci/build/rules/rule/clause/changes.rb
+++ b/lib/gitlab/ci/build/rules/rule/clause/changes.rb
@@ -4,8 +4,10 @@ module Gitlab
module Ci
module Build
class Rules::Rule::Clause::Changes < Rules::Rule::Clause
+ include Gitlab::Utils::StrongMemoize
+
def initialize(globs)
- @globs = Array(globs)
+ @globs = globs
end
def satisfied_by?(pipeline, context)
@@ -19,13 +21,21 @@ def satisfied_by?(pipeline, context)
end
end
+ private
+
def expand_globs(context)
- return @globs unless context
+ return paths unless context
- @globs.map do |glob|
+ paths.map do |glob|
ExpandVariables.expand_existing(glob, -> { context.variables_hash })
end
end
+
+ def paths
+ strong_memoize(:paths) do
+ Array(@globs[:paths])
+ end
+ end
end
end
end
diff --git a/lib/gitlab/ci/config/entry/rules/rule/changes.rb b/lib/gitlab/ci/config/entry/rules/rule/changes.rb
index be57e089f34c9da8ae7b48032b3ae1b4aeb711e5..a56b928450abca62a2c87c12f35a6408d8d3f28e 100644
--- a/lib/gitlab/ci/config/entry/rules/rule/changes.rb
+++ b/lib/gitlab/ci/config/entry/rules/rule/changes.rb
@@ -6,13 +6,51 @@ class Config
module Entry
class Rules
class Rule
- class Changes < ::Gitlab::Config::Entry::Node
- include ::Gitlab::Config::Entry::Validatable
+ class Changes < ::Gitlab::Config::Entry::Simplifiable
+ strategy :SimpleChanges, if: -> (config) { config.is_a?(Array) }
+ strategy :ComplexChanges, if: -> (config) { config.is_a?(Hash) }
- validations do
- validates :config,
- array_of_strings: true,
- length: { maximum: 50, too_long: "has too many entries (maximum %{count})" }
+ class SimpleChanges < ::Gitlab::Config::Entry::Node
+ include ::Gitlab::Config::Entry::Validatable
+
+ validations do
+ validates :config,
+ array_of_strings: true,
+ length: { maximum: 50, too_long: "has too many entries (maximum %{count})" }
+ end
+
+ def value
+ {
+ paths: config
+ }.compact
+ end
+ end
+
+ class ComplexChanges < ::Gitlab::Config::Entry::Node
+ include ::Gitlab::Config::Entry::Validatable
+ include ::Gitlab::Config::Entry::Attributable
+
+ ALLOWED_KEYS = %i[paths].freeze
+ REQUIRED_KEYS = %i[paths].freeze
+
+ attributes ALLOWED_KEYS
+
+ validations do
+ validates :config, allowed_keys: ALLOWED_KEYS
+ validates :config, required_keys: REQUIRED_KEYS
+
+ with_options allow_nil: false do
+ validates :paths,
+ array_of_strings: true,
+ length: { maximum: 50, too_long: "has too many entries (maximum %{count})" }
+ end
+ end
+ end
+
+ class UnknownStrategy < ::Gitlab::Config::Entry::Node
+ def errors
+ ["#{location} should be an array or a hash"]
+ end
end
end
end
diff --git a/spec/lib/gitlab/ci/build/rules/rule/clause/changes_spec.rb b/spec/lib/gitlab/ci/build/rules/rule/clause/changes_spec.rb
index 4ac8bf617380e1e3f543cb53e768d04cef6c3e8c..da9d3fab869007d47802c8d98d0d99555d1b10f7 100644
--- a/spec/lib/gitlab/ci/build/rules/rule/clause/changes_spec.rb
+++ b/spec/lib/gitlab/ci/build/rules/rule/clause/changes_spec.rb
@@ -6,19 +6,41 @@
describe '#satisfied_by?' do
subject { described_class.new(globs).satisfied_by?(pipeline, context) }
- it_behaves_like 'a glob matching rule' do
+ context 'a glob matching rule' do
+ using RSpec::Parameterized::TableSyntax
+
let(:pipeline) { build(:ci_pipeline) }
let(:context) {}
before do
allow(pipeline).to receive(:modified_paths).and_return(files.keys)
end
+
+ # rubocop:disable Layout/LineLength
+ where(:case_name, :globs, :files, :satisfied) do
+ 'exact top-level match' | { paths: ['Dockerfile'] } | { 'Dockerfile' => '', 'Gemfile' => '' } | true
+ 'exact top-level no match' | { paths: ['Dockerfile'] } | { 'Gemfile' => '' } | false
+ 'pattern top-level match' | { paths: ['Docker*'] } | { 'Dockerfile' => '', 'Gemfile' => '' } | true
+ 'pattern top-level no match' | { paths: ['Docker*'] } | { 'Gemfile' => '' } | false
+ 'exact nested match' | { paths: ['project/build.properties'] } | { 'project/build.properties' => '' } | true
+ 'exact nested no match' | { paths: ['project/build.properties'] } | { 'project/README.md' => '' } | false
+ 'pattern nested match' | { paths: ['src/**/*.go'] } | { 'src/gitlab.com/goproject/goproject.go' => '' } | true
+ 'pattern nested no match' | { paths: ['src/**/*.go'] } | { 'src/gitlab.com/goproject/README.md' => '' } | false
+ 'ext top-level match' | { paths: ['*.go'] } | { 'main.go' => '', 'cmd/goproject/main.go' => '' } | true
+ 'ext nested no match' | { paths: ['*.go'] } | { 'cmd/goproject/main.go' => '' } | false
+ 'ext slash no match' | { paths: ['/*.go'] } | { 'main.go' => '', 'cmd/goproject/main.go' => '' } | false
+ end
+ # rubocop:enable Layout/LineLength
+
+ with_them do
+ it { is_expected.to eq(satisfied) }
+ end
end
context 'when pipeline is nil' do
let(:pipeline) {}
let(:context) {}
- let(:globs) { [] }
+ let(:globs) { { paths: [] } }
it { is_expected.to be_truthy }
end
@@ -26,8 +48,8 @@
context 'when using variable expansion' do
let(:pipeline) { build(:ci_pipeline) }
let(:modified_paths) { ['helm/test.txt'] }
- let(:globs) { ['$HELM_DIR/**/*'] }
- let(:context) { double('context') }
+ let(:globs) { { paths: ['$HELM_DIR/**/*'] } }
+ let(:context) { instance_double(Gitlab::Ci::Build::Context::Base) }
before do
allow(pipeline).to receive(:modified_paths).and_return(modified_paths)
@@ -58,7 +80,7 @@
end
context 'when variable expansion does not match' do
- let(:globs) { ['path/with/$in/it/*'] }
+ let(:globs) { { paths: ['path/with/$in/it/*'] } }
let(:modified_paths) { ['path/with/$in/it/file.txt'] }
before do
diff --git a/spec/lib/gitlab/ci/build/rules/rule_spec.rb b/spec/lib/gitlab/ci/build/rules/rule_spec.rb
index f905e229415abc1bc0e27fe54b104a0114cc0a57..ac73b665f3ac02d90fc5632f38309237bd731fb1 100644
--- a/spec/lib/gitlab/ci/build/rules/rule_spec.rb
+++ b/spec/lib/gitlab/ci/build/rules/rule_spec.rb
@@ -14,10 +14,14 @@
let(:ci_build) { build(:ci_build, pipeline: pipeline) }
let(:rule) { described_class.new(rule_hash) }
+ before do
+ allow(pipeline).to receive(:modified_paths).and_return(['file.rb'])
+ end
+
describe '#matches?' do
subject { rule.matches?(pipeline, seed) }
- context 'with one matching clause' do
+ context 'with one matching clause if' do
let(:rule_hash) do
{ if: '$VAR == null', when: 'always' }
end
@@ -25,9 +29,17 @@
it { is_expected.to eq(true) }
end
+ context 'with one matching clause changes' do
+ let(:rule_hash) do
+ { changes: { paths: ['**/*'] }, when: 'always' }
+ end
+
+ it { is_expected.to eq(true) }
+ end
+
context 'with two matching clauses' do
let(:rule_hash) do
- { if: '$VAR == null', changes: '**/*', when: 'always' }
+ { if: '$VAR == null', changes: { paths: ['**/*'] }, when: 'always' }
end
it { is_expected.to eq(true) }
@@ -35,7 +47,7 @@
context 'with a matching and non-matching clause' do
let(:rule_hash) do
- { if: '$VAR != null', changes: '$VAR == null', when: 'always' }
+ { if: '$VAR != null', changes: { paths: ['invalid.xyz'] }, when: 'always' }
end
it { is_expected.to eq(false) }
@@ -43,7 +55,7 @@
context 'with two non-matching clauses' do
let(:rule_hash) do
- { if: '$VAR != null', changes: 'README', when: 'always' }
+ { if: '$VAR != null', changes: { paths: ['README'] }, when: 'always' }
end
it { is_expected.to eq(false) }
diff --git a/spec/lib/gitlab/ci/config/entry/rules/rule/changes_spec.rb b/spec/lib/gitlab/ci/config/entry/rules/rule/changes_spec.rb
index 3ed4a9f263f1d861dc274d781a57b28d3670615d..295561b3c4d26ac7648969586f07bcbd6d853d72 100644
--- a/spec/lib/gitlab/ci/config/entry/rules/rule/changes_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/rules/rule/changes_spec.rb
@@ -37,7 +37,7 @@
it { is_expected.not_to be_valid }
it 'reports an error about invalid policy' do
- expect(entry.errors).to include(/should be an array of strings/)
+ expect(entry.errors).to include(/should be an array or a hash/)
end
end
@@ -64,7 +64,59 @@
it 'returns information about errors' do
expect(entry.errors)
- .to include(/should be an array of strings/)
+ .to include(/should be an array or a hash/)
+ end
+ end
+
+ context 'with paths' do
+ context 'when paths is an array of strings' do
+ let(:config) { { paths: %w[app/ lib/] } }
+
+ it { is_expected.to be_valid }
+ end
+
+ context 'when paths is not an array' do
+ let(:config) { { paths: 'string' } }
+
+ it { is_expected.not_to be_valid }
+
+ it 'returns information about errors' do
+ expect(entry.errors)
+ .to include(/should be an array of strings/)
+ end
+ end
+
+ context 'when paths is an array of integers' do
+ let(:config) { { paths: [1, 2] } }
+
+ it { is_expected.not_to be_valid }
+
+ it 'returns information about errors' do
+ expect(entry.errors)
+ .to include(/should be an array of strings/)
+ end
+ end
+
+ context 'when paths is an array of long strings' do
+ let(:config) { { paths: ['a'] * 51 } }
+
+ it { is_expected.not_to be_valid }
+
+ it 'returns information about errors' do
+ expect(entry.errors)
+ .to include(/has too many entries \(maximum 50\)/)
+ end
+ end
+
+ context 'when paths is nil' do
+ let(:config) { { paths: nil } }
+
+ it { is_expected.not_to be_valid }
+
+ it 'returns information about errors' do
+ expect(entry.errors)
+ .to include(/should be an array of strings/)
+ end
end
end
end
@@ -75,6 +127,14 @@
context 'when using a string array' do
let(:config) { %w[app/ lib/ spec/ other/* paths/**/*.rb] }
+ it { is_expected.to eq(paths: config) }
+ end
+
+ context 'with paths' do
+ let(:config) do
+ { paths: ['app/', 'lib/'] }
+ end
+
it { is_expected.to eq(config) }
end
end
diff --git a/spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb b/spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb
index 89d349efe8f5cd4d0770ec549f0f7d2f74bdcfc2..93f4a66bfb6c53c931aa23e8136a6de518fe18c4 100644
--- a/spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb
@@ -115,7 +115,7 @@
it { is_expected.not_to be_valid }
it 'reports an error about invalid policy' do
- expect(subject.errors).to include(/should be an array of strings/)
+ expect(subject.errors).to include(/should be an array or a hash/)
end
end
@@ -411,7 +411,13 @@
context 'when using a changes: clause' do
let(:config) { { changes: %w[app/ lib/ spec/ other/* paths/**/*.rb] } }
- it { is_expected.to eq(config) }
+ it { is_expected.to eq(changes: { paths: %w[app/ lib/ spec/ other/* paths/**/*.rb] }) }
+
+ context 'when using changes with paths' do
+ let(:config) { { changes: { paths: %w[app/ lib/ spec/ other/* paths/**/*.rb] } } }
+
+ it { is_expected.to eq(config) }
+ end
end
context 'when default value has been provided' do
@@ -426,7 +432,7 @@
end
it 'does not add to provided configuration' do
- expect(entry.value).to eq(config)
+ expect(entry.value).to eq(changes: { paths: %w[app/**/*.rb] })
end
end
diff --git a/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb b/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb
index 49505d397c2c4b333e03fc97923ca8e090b87f23..040f3ab5830f18265244a7c07bcc4e6fc18f1040 100644
--- a/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb
+++ b/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb
@@ -858,14 +858,14 @@
context 'with an explicit `when: never`' do
where(:rule_set) do
[
- [[{ changes: %w[*/**/*.rb], when: 'never' }, { changes: %w[*/**/*.rb], when: 'always' }]],
- [[{ changes: %w[app/models/ci/pipeline.rb], when: 'never' }, { changes: %w[app/models/ci/pipeline.rb], when: 'always' }]],
- [[{ changes: %w[spec/**/*.rb], when: 'never' }, { changes: %w[spec/**/*.rb], when: 'always' }]],
- [[{ changes: %w[*.yml], when: 'never' }, { changes: %w[*.yml], when: 'always' }]],
- [[{ changes: %w[.*.yml], when: 'never' }, { changes: %w[.*.yml], when: 'always' }]],
- [[{ changes: %w[**/*], when: 'never' }, { changes: %w[**/*], when: 'always' }]],
- [[{ changes: %w[*/**/*.rb *.yml], when: 'never' }, { changes: %w[*/**/*.rb *.yml], when: 'always' }]],
- [[{ changes: %w[.*.yml **/*], when: 'never' }, { changes: %w[.*.yml **/*], when: 'always' }]]
+ [[{ changes: { paths: %w[*/**/*.rb] }, when: 'never' }, { changes: { paths: %w[*/**/*.rb] }, when: 'always' }]],
+ [[{ changes: { paths: %w[app/models/ci/pipeline.rb] }, when: 'never' }, { changes: { paths: %w[app/models/ci/pipeline.rb] }, when: 'always' }]],
+ [[{ changes: { paths: %w[spec/**/*.rb] }, when: 'never' }, { changes: { paths: %w[spec/**/*.rb] }, when: 'always' }]],
+ [[{ changes: { paths: %w[*.yml] }, when: 'never' }, { changes: { paths: %w[*.yml] }, when: 'always' }]],
+ [[{ changes: { paths: %w[.*.yml] }, when: 'never' }, { changes: { paths: %w[.*.yml] }, when: 'always' }]],
+ [[{ changes: { paths: %w[**/*] }, when: 'never' }, { changes: { paths: %w[**/*] }, when: 'always' }]],
+ [[{ changes: { paths: %w[*/**/*.rb *.yml] }, when: 'never' }, { changes: { paths: %w[*/**/*.rb *.yml] }, when: 'always' }]],
+ [[{ changes: { paths: %w[.*.yml **/*] }, when: 'never' }, { changes: { paths: %w[.*.yml **/*] }, when: 'always' }]]
]
end
@@ -881,14 +881,14 @@
context 'with an explicit `when: always`' do
where(:rule_set) do
[
- [[{ changes: %w[*/**/*.rb], when: 'always' }, { changes: %w[*/**/*.rb], when: 'never' }]],
- [[{ changes: %w[app/models/ci/pipeline.rb], when: 'always' }, { changes: %w[app/models/ci/pipeline.rb], when: 'never' }]],
- [[{ changes: %w[spec/**/*.rb], when: 'always' }, { changes: %w[spec/**/*.rb], when: 'never' }]],
- [[{ changes: %w[*.yml], when: 'always' }, { changes: %w[*.yml], when: 'never' }]],
- [[{ changes: %w[.*.yml], when: 'always' }, { changes: %w[.*.yml], when: 'never' }]],
- [[{ changes: %w[**/*], when: 'always' }, { changes: %w[**/*], when: 'never' }]],
- [[{ changes: %w[*/**/*.rb *.yml], when: 'always' }, { changes: %w[*/**/*.rb *.yml], when: 'never' }]],
- [[{ changes: %w[.*.yml **/*], when: 'always' }, { changes: %w[.*.yml **/*], when: 'never' }]]
+ [[{ changes: { paths: %w[*/**/*.rb] }, when: 'always' }, { changes: { paths: %w[*/**/*.rb] }, when: 'never' }]],
+ [[{ changes: { paths: %w[app/models/ci/pipeline.rb] }, when: 'always' }, { changes: { paths: %w[app/models/ci/pipeline.rb] }, when: 'never' }]],
+ [[{ changes: { paths: %w[spec/**/*.rb] }, when: 'always' }, { changes: { paths: %w[spec/**/*.rb] }, when: 'never' }]],
+ [[{ changes: { paths: %w[*.yml] }, when: 'always' }, { changes: { paths: %w[*.yml] }, when: 'never' }]],
+ [[{ changes: { paths: %w[.*.yml] }, when: 'always' }, { changes: { paths: %w[.*.yml] }, when: 'never' }]],
+ [[{ changes: { paths: %w[**/*] }, when: 'always' }, { changes: { paths: %w[**/*] }, when: 'never' }]],
+ [[{ changes: { paths: %w[*/**/*.rb *.yml] }, when: 'always' }, { changes: { paths: %w[*/**/*.rb *.yml] }, when: 'never' }]],
+ [[{ changes: { paths: %w[.*.yml **/*] }, when: 'always' }, { changes: { paths: %w[.*.yml **/*] }, when: 'never' }]]
]
end
@@ -904,14 +904,14 @@
context 'without an explicit when: value' do
where(:rule_set) do
[
- [[{ changes: %w[*/**/*.rb] }]],
- [[{ changes: %w[app/models/ci/pipeline.rb] }]],
- [[{ changes: %w[spec/**/*.rb] }]],
- [[{ changes: %w[*.yml] }]],
- [[{ changes: %w[.*.yml] }]],
- [[{ changes: %w[**/*] }]],
- [[{ changes: %w[*/**/*.rb *.yml] }]],
- [[{ changes: %w[.*.yml **/*] }]]
+ [[{ changes: { paths: %w[*/**/*.rb] } }]],
+ [[{ changes: { paths: %w[app/models/ci/pipeline.rb] } }]],
+ [[{ changes: { paths: %w[spec/**/*.rb] } }]],
+ [[{ changes: { paths: %w[*.yml] } }]],
+ [[{ changes: { paths: %w[.*.yml] } }]],
+ [[{ changes: { paths: %w[**/*] } }]],
+ [[{ changes: { paths: %w[*/**/*.rb *.yml] } }]],
+ [[{ changes: { paths: %w[.*.yml **/*] } }]]
]
end
diff --git a/spec/lib/gitlab/ci/yaml_processor_spec.rb b/spec/lib/gitlab/ci/yaml_processor_spec.rb
index 15567b3667330a3e6ea598806454de5c76fdfd20..22bc6b0db5974d1fe6fe5d8627242dbf3420561d 100644
--- a/spec/lib/gitlab/ci/yaml_processor_spec.rb
+++ b/spec/lib/gitlab/ci/yaml_processor_spec.rb
@@ -70,7 +70,7 @@ module Ci
options: { script: ['rspec'] },
rules: [
{ if: '$CI_COMMIT_REF_NAME == "master"' },
- { changes: %w[README.md] }
+ { changes: { paths: %w[README.md] } }
],
allow_failure: false,
when: 'on_success',
@@ -2893,6 +2893,51 @@ module Ci
end
end
+ describe 'Rules' do
+ context 'changes' do
+ let(:config) do
+ <<~YAML
+ rspec:
+ script: exit 0
+ rules:
+ - changes: [README.md]
+ YAML
+ end
+
+ it 'returns builds with correct rules' do
+ expect(processor.builds.size).to eq(1)
+ expect(processor.builds[0]).to match(
+ hash_including(
+ name: "rspec",
+ rules: [{ changes: { paths: ["README.md"] } }]
+ )
+ )
+ end
+
+ context 'with paths' do
+ let(:config) do
+ <<~YAML
+ rspec:
+ script: exit 0
+ rules:
+ - changes:
+ paths: [README.md]
+ YAML
+ end
+
+ it 'returns builds with correct rules' do
+ expect(processor.builds.size).to eq(1)
+ expect(processor.builds[0]).to match(
+ hash_including(
+ name: "rspec",
+ rules: [{ changes: { paths: ["README.md"] } }]
+ )
+ )
+ end
+ end
+ end
+ end
+
describe '#execute' do
subject { Gitlab::Ci::YamlProcessor.new(content).execute }
diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb
index aac059f2104437e3fce23bae6362c1b8cea18a35..9cef7f7dadb1113f9bed23bd2d947cbd4ae65cd8 100644
--- a/spec/services/ci/create_pipeline_service_spec.rb
+++ b/spec/services/ci/create_pipeline_service_spec.rb
@@ -2087,6 +2087,12 @@ def find_job(name)
rules:
- changes:
- $CI_JOB_NAME*
+
+ changes-paths:
+ script: "I am using a new syntax!"
+ rules:
+ - changes:
+ paths: [README.md]
EOY
end
@@ -2098,8 +2104,9 @@ def find_job(name)
it 'creates five jobs' do
expect(pipeline).to be_persisted
- expect(build_names)
- .to contain_exactly('regular-job', 'rules-job', 'delayed-job', 'negligible-job', 'README')
+ expect(build_names).to contain_exactly(
+ 'regular-job', 'rules-job', 'delayed-job', 'negligible-job', 'README', 'changes-paths'
+ )
end
it 'sets when: for all jobs' do