diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 791bae172718e0c54f8c517adb95cf4d7af49e07..77704ac02d293f5692bb886db46a17414ec563d7 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -1152,6 +1152,10 @@ def modified_paths end end + def modified_paths_since(compare_to_sha) + project.repository.diff_stats(project.repository.merge_base(compare_to_sha, sha), sha).paths + end + def all_worktree_paths strong_memoize(:all_worktree_paths) do project.repository.ls_files(sha) diff --git a/config/feature_flags/development/ci_rules_changes_compare.yml b/config/feature_flags/development/ci_rules_changes_compare.yml new file mode 100644 index 0000000000000000000000000000000000000000..b405c2ab919a4ef192f3ab54f2c79f092e9a55a3 --- /dev/null +++ b/config/feature_flags/development/ci_rules_changes_compare.yml @@ -0,0 +1,8 @@ +--- +name: ci_rules_changes_compare +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/90968 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/366412 +milestone: '15.3' +type: development +group: group::pipeline authoring +default_enabled: false diff --git a/doc/ci/yaml/index.md b/doc/ci/yaml/index.md index 6d8ca7fe8cf0591c119af1527092f79ae8d1a548..3fbc6a40a4b9fdb45835816582b635e7d2bb3d69 100644 --- a/doc/ci/yaml/index.md +++ b/doc/ci/yaml/index.md @@ -3244,6 +3244,64 @@ docker build: - [Jobs or pipelines can run unexpectedly when using `rules: changes`](../jobs/job_control.md#jobs-or-pipelines-run-unexpectedly-when-using-changes). +##### `rules:changes:paths` + +> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/90171) in GitLab 15.2. + +`rules:changes:paths` is an alias for `rules:changes`. + +**Keyword type**: Job keyword. You can use it only as part of a job. + +**Possible inputs**: + +- An array of file paths. + +**Example of `rules:changes:paths`**: + +```yaml +docker build: + script: docker build -t my-image:$CI_COMMIT_REF_SLUG . + rules: + - if: $CI_PIPELINE_SOURCE == "merge_request_event" + changes: + paths: + - Dockerfile +``` + +In this example, the `docker build` job is only included when the `Dockerfile` has changed +and the pipeline source is a merge request event. + +##### `rules:changes:compare_to` + +> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/293645) in GitLab 15.3 [with a flag](../../administration/feature_flags.md) named `ci_rules_changes_compare`. Disabled by default. + +Use `rules:changes:compare_to` to specify which ref to compare against for changes to the files +listed under [`rules:changes:paths`](#ruleschangespaths). + +**Keyword type**: Job keyword. You can use it only as part of a job, and it must be combined with `rules:changes:paths`. + +**Possible inputs**: + +- A branch name, like `main`, `branch1`, or `refs/heads/branch1`. +- A tag name, like `tag1` or `refs/tags/tag1`. +- A commit SHA, like `2fg31ga14b`. + +**Example of `rules:changes:compare_to`**: + +```yaml +docker build: + script: docker build -t my-image:$CI_COMMIT_REF_SLUG . + rules: + - if: $CI_PIPELINE_SOURCE == "merge_request_event" + changes: + paths: + - Dockerfile + compare_to: 'refs/heads/branch1' +``` + +In this example, the `docker build` job is only included when the `Dockerfile` has changed +relative to `refs/heads/branch1` and the pipeline source is a merge request event. + #### `rules:exists` > [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/24021) in GitLab 12.4. diff --git a/lib/gitlab/ci/build/rules.rb b/lib/gitlab/ci/build/rules.rb index 2d4f9cf635b53c62569f3070b9e363d4bc7f7c7d..dee95534b0755416240e67e25d0b6660beb76959 100644 --- a/lib/gitlab/ci/build/rules.rb +++ b/lib/gitlab/ci/build/rules.rb @@ -6,7 +6,7 @@ module Build class Rules include ::Gitlab::Utils::StrongMemoize - Result = Struct.new(:when, :start_in, :allow_failure, :variables) do + Result = Struct.new(:when, :start_in, :allow_failure, :variables, :errors) do def build_attributes { when: self.when, @@ -38,6 +38,8 @@ def evaluate(pipeline, context) else Result.new('never') end + rescue Rule::Clause::ParseError => e + Result.new('never', nil, nil, nil, [e.message]) end private diff --git a/lib/gitlab/ci/build/rules/rule/clause.rb b/lib/gitlab/ci/build/rules/rule/clause.rb index 6d4bbbb8c21c94115b876889343851b3b16a8d82..503f2a8736114c8f46195e081761987a87cd9f00 100644 --- a/lib/gitlab/ci/build/rules/rule/clause.rb +++ b/lib/gitlab/ci/build/rules/rule/clause.rb @@ -11,6 +11,7 @@ class Rules::Rule::Clause # Used for job's inclusion rules configuration. # UnknownClauseError = Class.new(StandardError) + ParseError = Class.new(StandardError) def self.fabricate(type, value) "#{self}::#{type.to_s.camelize}".safe_constantize&.new(value) diff --git a/lib/gitlab/ci/build/rules/rule/clause/changes.rb b/lib/gitlab/ci/build/rules/rule/clause/changes.rb index 1bcd87c9d93f4ae4237498ba32f463c59e3fff71..29e43198635a930944a6b22b39d839c861b36a0e 100644 --- a/lib/gitlab/ci/build/rules/rule/clause/changes.rb +++ b/lib/gitlab/ci/build/rules/rule/clause/changes.rb @@ -11,10 +11,12 @@ def initialize(globs) end def satisfied_by?(pipeline, context) - return true unless pipeline&.modified_paths + modified_paths = find_modified_paths(pipeline) + + return true unless modified_paths expanded_globs = expand_globs(context) - pipeline.modified_paths.any? do |path| + modified_paths.any? do |path| expanded_globs.any? do |glob| File.fnmatch?(glob, path, File::FNM_PATHNAME | File::FNM_DOTMATCH | File::FNM_EXTGLOB) end @@ -40,6 +42,28 @@ def paths end end end + + def find_modified_paths(pipeline) + return unless pipeline + return pipeline.modified_paths unless ::Feature.enabled?(:ci_rules_changes_compare, pipeline.project) + + compare_to_sha = find_compare_to_sha(pipeline) + + if compare_to_sha + pipeline.modified_paths_since(compare_to_sha) + else + pipeline.modified_paths + end + end + + def find_compare_to_sha(pipeline) + return unless @globs.include?(:compare_to) + + commit = pipeline.project.commit(@globs[:compare_to]) + raise Rules::Rule::Clause::ParseError, 'rules:changes:compare_to is not a valid ref' unless commit + + commit.sha + 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 a56b928450abca62a2c87c12f35a6408d8d3f28e..107e7c228af0eda4fe007fe8641ec1de4ff5e8db 100644 --- a/lib/gitlab/ci/config/entry/rules/rule/changes.rb +++ b/lib/gitlab/ci/config/entry/rules/rule/changes.rb @@ -30,7 +30,7 @@ class ComplexChanges < ::Gitlab::Config::Entry::Node include ::Gitlab::Config::Entry::Validatable include ::Gitlab::Config::Entry::Attributable - ALLOWED_KEYS = %i[paths].freeze + ALLOWED_KEYS = %i[paths compare_to].freeze REQUIRED_KEYS = %i[paths].freeze attributes ALLOWED_KEYS @@ -43,6 +43,7 @@ class ComplexChanges < ::Gitlab::Config::Entry::Node validates :paths, array_of_strings: true, length: { maximum: 50, too_long: "has too many entries (maximum %{count})" } + validates :compare_to, type: String, allow_nil: true end end end diff --git a/lib/gitlab/ci/pipeline/seed/build.rb b/lib/gitlab/ci/pipeline/seed/build.rb index 901208f325a68ae67d929b7dbb8c0cbb383a57e8..93106b96af20a723942fa97082d85f28880ed867 100644 --- a/lib/gitlab/ci/pipeline/seed/build.rb +++ b/lib/gitlab/ci/pipeline/seed/build.rb @@ -54,9 +54,11 @@ def included? end def errors - return unless included? - strong_memoize(:errors) do + # We check rules errors before checking "included?" because rules affects its inclusion status. + next rules_errors if rules_errors + next unless included? + [needs_errors, variable_expansion_errors].compact.flatten end end @@ -168,6 +170,12 @@ def rules_result end end + def rules_errors + strong_memoize(:rules_errors) do + ["Failed to parse rule for #{name}: #{rules_result.errors.join(', ')}"] if rules_result.errors.present? + end + end + def evaluate_context strong_memoize(:evaluate_context) do Gitlab::Ci::Build::Context::Build.new(@pipeline, @seed_attributes) diff --git a/lib/gitlab/ci/pipeline/seed/stage.rb b/lib/gitlab/ci/pipeline/seed/stage.rb index bc56fe9bef93bbed1faf18c14124537420922fec..7cf6466cf4b25f5660ccc5c1465e5450b5d843b3 100644 --- a/lib/gitlab/ci/pipeline/seed/stage.rb +++ b/lib/gitlab/ci/pipeline/seed/stage.rb @@ -36,7 +36,7 @@ def seeds def errors strong_memoize(:errors) do - seeds.flat_map(&:errors).compact + @builds.flat_map(&:errors).compact 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 3892b88598a5e8300c3a8cc5afae6339330c2e73..fa92d1a830d94021a17a4950dbdbe54e70e46e5e 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 @@ -4,7 +4,9 @@ RSpec.describe Gitlab::Ci::Build::Rules::Rule::Clause::Changes do describe '#satisfied_by?' do - subject { described_class.new(globs).satisfied_by?(pipeline, context) } + let(:context) { instance_double(Gitlab::Ci::Build::Context::Base) } + + subject(:satisfied_by) { described_class.new(globs).satisfied_by?(pipeline, context) } context 'a glob matching rule' do using RSpec::Parameterized::TableSyntax @@ -92,5 +94,97 @@ it { is_expected.to be_truthy } end end + + context 'when using compare_to' do + let_it_be(:project) do + create(:project, :custom_repo, + files: { 'README.md' => 'readme' }) + end + + let_it_be(:user) { project.owner } + + before_all do + project.repository.add_branch(user, 'feature_1', 'master') + + project.repository.create_file( + user, 'file1.txt', 'file 1', message: 'Create file1.txt', branch_name: 'feature_1' + ) + project.repository.add_tag(user, 'tag_1', 'feature_1') + + project.repository.create_file( + user, 'file2.txt', 'file 2', message: 'Create file2.txt', branch_name: 'feature_1' + ) + project.repository.add_branch(user, 'feature_2', 'feature_1') + + project.repository.update_file( + user, 'file2.txt', 'file 2 updated', message: 'Update file2.txt', branch_name: 'feature_2' + ) + end + + context 'when compare_to is branch or tag' do + using RSpec::Parameterized::TableSyntax + + where(:pipeline_ref, :compare_to, :paths, :ff, :result) do + 'feature_1' | 'master' | ['file1.txt'] | true | true + 'feature_1' | 'master' | ['README.md'] | true | false + 'feature_1' | 'master' | ['xyz.md'] | true | false + 'feature_2' | 'master' | ['file1.txt'] | true | true + 'feature_2' | 'master' | ['file2.txt'] | true | true + 'feature_2' | 'feature_1' | ['file1.txt'] | true | false + 'feature_2' | 'feature_1' | ['file1.txt'] | false | true + 'feature_2' | 'feature_1' | ['file2.txt'] | true | true + 'feature_1' | 'tag_1' | ['file1.txt'] | true | false + 'feature_1' | 'tag_1' | ['file1.txt'] | false | true + 'feature_1' | 'tag_1' | ['file2.txt'] | true | true + 'feature_2' | 'tag_1' | ['file2.txt'] | true | true + end + + with_them do + let(:globs) { { paths: paths, compare_to: compare_to } } + + let(:pipeline) do + build(:ci_pipeline, project: project, ref: pipeline_ref, sha: project.commit(pipeline_ref).sha) + end + + before do + stub_feature_flags(ci_rules_changes_compare: ff) + end + + it { is_expected.to eq(result) } + end + end + + context 'when compare_to is a sha' do + let(:globs) { { paths: ['file2.txt'], compare_to: project.commit('tag_1').sha } } + + let(:pipeline) do + build(:ci_pipeline, project: project, ref: 'feature_2', sha: project.commit('feature_2').sha) + end + + it { is_expected.to be_truthy } + end + + context 'when compare_to is not a valid ref' do + let(:globs) { { paths: ['file1.txt'], compare_to: 'xyz' } } + + let(:pipeline) do + build(:ci_pipeline, project: project, ref: 'feature_2', sha: project.commit('feature_2').sha) + end + + it 'raises ParseError' do + expect { satisfied_by }.to raise_error( + ::Gitlab::Ci::Build::Rules::Rule::Clause::ParseError, 'rules:changes:compare_to is not a valid ref' + ) + end + + context 'when the FF ci_rules_changes_compare is disabled' do + before do + stub_feature_flags(ci_rules_changes_compare: false) + end + + it { is_expected.to be_truthy } + end + end + end end end 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 295561b3c4d26ac7648969586f07bcbd6d853d72..64f0a64074c215727af6282f1738609c7c936c0e 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 @@ -1,6 +1,6 @@ # frozen_string_literal: true -require 'fast_spec_helper' +require 'spec_helper' RSpec.describe Gitlab::Ci::Config::Entry::Rules::Rule::Changes do let(:factory) do @@ -119,6 +119,23 @@ end end end + + context 'with paths and compare_to' do + let(:config) { { paths: %w[app/ lib/], compare_to: 'branch1' } } + + it { is_expected.to be_valid } + + context 'when compare_to is not a string' do + let(:config) { { paths: %w[app/ lib/], compare_to: 1 } } + + it { is_expected.not_to be_valid } + + it 'returns information about errors' do + expect(entry.errors) + .to include(/should be a string/) + end + end + end end describe '#value' do @@ -137,5 +154,13 @@ it { is_expected.to eq(config) } end + + context 'with paths and compare_to' do + let(:config) do + { paths: ['app/', 'lib/'], compare_to: 'branch1' } + end + + it { is_expected.to eq(config) } + end 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 93f4a66bfb6c53c931aa23e8136a6de518fe18c4..10b4cba3bbb13f99bf43b60cc80e5143821618ac 100644 --- a/spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb @@ -418,6 +418,12 @@ it { is_expected.to eq(config) } end + + context 'when using changes with paths and compare_to' do + let(:config) { { changes: { paths: %w[app/ lib/ spec/ other/* paths/**/*.rb], compare_to: 'branch1' } } } + + it { is_expected.to eq(config) } + end end context 'when default value has been provided' do diff --git a/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb b/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb index 040f3ab5830f18265244a7c07bcc4e6fc18f1040..890ba51157a43a2e58fafb21cae306adf551b596 100644 --- a/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb @@ -769,7 +769,7 @@ with_them do it { is_expected.not_to be_included } - it 'correctly populates when:' do + it 'still correctly populates when:' do expect(seed_build.attributes).to include(when: 'never') end end @@ -958,6 +958,26 @@ expect(seed_build.attributes).to include(when: 'never') end end + + context 'with invalid rules raising error' do + let(:rule_set) do + [ + { changes: { paths: ['README.md'], compare_to: 'invalid-ref' }, when: 'never' } + ] + end + + it { is_expected.not_to be_included } + + it 'correctly populates when:' do + expect(seed_build.attributes).to include(when: 'never') + end + + it 'returns an error' do + expect(seed_build.errors).to contain_exactly( + 'Failed to parse rule for rspec: rules:changes:compare_to is not a valid ref' + ) + end + end end end diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 081fa6cbbaeb31c7eceb2a1590e476adbd01ef63..b1ddc26b1dc11b56e86b8c7dc1b55631b267c0fc 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -2162,6 +2162,60 @@ def create_build(name, *traits, queued_at: current, started_from: 0, **opts) end end + describe '#modified_paths_since' do + let(:project) do + create(:project, :custom_repo, + files: { 'file1.txt' => 'file 1' }) + end + + let(:user) { project.owner } + let(:main_branch) { project.default_branch } + let(:new_branch) { 'feature_x' } + let(:pipeline) { build(:ci_pipeline, project: project, sha: new_branch) } + + subject(:modified_paths_since) { pipeline.modified_paths_since(main_branch) } + + before do + project.repository.add_branch(user, new_branch, main_branch) + end + + context 'when no change in the new branch' do + it 'returns an empty array' do + expect(modified_paths_since).to be_empty + end + end + + context 'when adding a new file' do + before do + project.repository.create_file(user, 'file2.txt', 'file 2', message: 'Create file2.txt', branch_name: new_branch) + end + + it 'returns the new file path' do + expect(modified_paths_since).to eq(['file2.txt']) + end + + context 'and when updating an existing file' do + before do + project.repository.update_file(user, 'file1.txt', 'file 1 updated', message: 'Update file1.txt', branch_name: new_branch) + end + + it 'returns the new and updated file paths' do + expect(modified_paths_since).to eq(['file1.txt', 'file2.txt']) + end + end + end + + context 'when updating an existing file' do + before do + project.repository.update_file(user, 'file1.txt', 'file 1 updated', message: 'Update file1.txt', branch_name: new_branch) + end + + it 'returns the updated file path' do + expect(modified_paths_since).to eq(['file1.txt']) + end + end + end + describe '#all_worktree_paths' do let(:files) { { 'main.go' => '', 'mocks/mocks.go' => '' } } let(:project) { create(:project, :custom_repo, files: files) } diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index 9cef7f7dadb1113f9bed23bd2d947cbd4ae65cd8..9e1fb46558e5bd0a4a827f44b4b9e1cdb5f17f90 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -18,6 +18,7 @@ # rubocop:disable Metrics/ParameterLists def execute_service( source: :push, + before: '00000000', after: project.commit.id, ref: ref_name, trigger_request: nil, @@ -29,7 +30,7 @@ def execute_service( target_sha: nil, save_on_errors: true) params = { ref: ref, - before: '00000000', + before: before, after: after, variables_attributes: variables_attributes, push_options: push_options, @@ -2152,6 +2153,145 @@ def find_job(name) end end + context 'with changes: paths and compare_to' do + before_all do + project.repository.add_branch(user, 'feature_1', 'master') + + project.repository.create_file( + user, 'file1.txt', 'file 1', message: 'Create file1.txt', branch_name: 'feature_1' + ) + + project.repository.add_branch(user, 'feature_2', 'feature_1') + + project.repository.create_file( + user, 'file2.txt', 'file 2', message: 'Create file2.txt', branch_name: 'feature_2' + ) + end + + let(:changed_file) { 'file2.txt' } + let(:ref_name) { 'feature_2' } + + let(:response) { execute_service(ref: ref_name, before: nil, after: project.commit(ref_name).sha) } + + context 'for jobs rules' do + let(:config) do + <<-EOY + job1: + script: exit 0 + rules: + - changes: + paths: [#{changed_file}] + compare_to: #{compare_to} + + job2: + script: exit 0 + EOY + end + + context 'when there is no such compare_to ref' do + let(:compare_to) { 'invalid-branch' } + + it 'returns an error' do + expect(pipeline.errors.full_messages).to eq([ + 'Failed to parse rule for job1: rules:changes:compare_to is not a valid ref' + ]) + end + + context 'when the FF ci_rules_changes_compare is not enabled' do + before do + stub_feature_flags(ci_rules_changes_compare: false) + end + + it 'ignores compare_to and changes is always true' do + expect(build_names).to contain_exactly('job1', 'job2') + end + end + end + + context 'when the compare_to ref exists' do + let(:compare_to) { 'feature_1'} + + context 'when the rule matches' do + it 'creates job1 and job2' do + expect(build_names).to contain_exactly('job1', 'job2') + end + + context 'when the FF ci_rules_changes_compare is not enabled' do + before do + stub_feature_flags(ci_rules_changes_compare: false) + end + + it 'ignores compare_to and changes is always true' do + expect(build_names).to contain_exactly('job1', 'job2') + end + end + end + + context 'when the rule does not match' do + let(:changed_file) { 'file1.txt' } + + it 'does not create job1' do + expect(build_names).to contain_exactly('job2') + end + + context 'when the FF ci_rules_changes_compare is not enabled' do + before do + stub_feature_flags(ci_rules_changes_compare: false) + end + + it 'ignores compare_to and changes is always true' do + expect(build_names).to contain_exactly('job1', 'job2') + end + end + end + end + end + + context 'for workflow rules' do + let(:config) do + <<-EOY + workflow: + rules: + - changes: + paths: [#{changed_file}] + compare_to: #{compare_to} + + job1: + script: exit 0 + EOY + end + + let(:compare_to) { 'feature_1'} + + context 'when the rule matches' do + it 'creates job1' do + expect(pipeline).to be_created_successfully + expect(build_names).to contain_exactly('job1') + end + + context 'when the FF ci_rules_changes_compare is not enabled' do + before do + stub_feature_flags(ci_rules_changes_compare: false) + end + + it 'ignores compare_to and changes is always true' do + expect(pipeline).to be_created_successfully + expect(build_names).to contain_exactly('job1') + end + end + end + + context 'when the rule does not match' do + let(:changed_file) { 'file1.txt' } + + it 'does not create job1' do + expect(pipeline).not_to be_created_successfully + expect(build_names).to be_empty + end + end + end + end + context 'with mixed if: and changes: rules' do let(:config) do <<-EOY