From 3c2d6b4870afa9cae25007a2ccf90ebf92c37d42 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 28 Sep 2018 11:00:30 +0200 Subject: [PATCH 01/40] Add a class that represents a git push operation --- app/services/merge_requests/base_service.rb | 6 ++--- lib/gitlab/git/push.rb | 25 +++++++++++++++++++++ 2 files changed, 28 insertions(+), 3 deletions(-) create mode 100644 lib/gitlab/git/push.rb diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb index aa5d8406d0f..28c3219b37b 100644 --- a/app/services/merge_requests/base_service.rb +++ b/app/services/merge_requests/base_service.rb @@ -57,10 +57,10 @@ module MergeRequests # Returns all origin and fork merge requests from `@project` satisfying passed arguments. # rubocop: disable CodeReuse/ActiveRecord def merge_requests_for(source_branch, mr_states: [:opened]) - MergeRequest + @project.source_of_merge_requests .with_state(mr_states) - .where(source_branch: source_branch, source_project_id: @project.id) - .preload(:source_project) # we don't need a #includes since we're just preloading for the #select + .where(source_branch: source_branch) + .preload(:source_project) # we don't need #includes since we're just preloading for the #select .select(&:source_project) end # rubocop: enable CodeReuse/ActiveRecord diff --git a/lib/gitlab/git/push.rb b/lib/gitlab/git/push.rb new file mode 100644 index 00000000000..d4fe3c623c1 --- /dev/null +++ b/lib/gitlab/git/push.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +module Gitlab + module Git + class Push + def initialize(project, oldrev, newrev, ref) + @project, @oldrev, @newrev = project, oldrev, newrev + @repository = project.repository + @branch_name = Gitlab::Git.ref_name(ref) + end + + def branch_added? + Gitlab::Git.blank_ref?(@oldrev) + end + + def branch_removed? + Gitlab::Git.blank_ref?(@newrev) + end + + def force_push? + Gitlab::Checks::ForcePush.force_push?(@project, @oldrev, @newrev) + end + end + end +end From 76d9e29a65e488a316347e054920924a3c5f8b3d Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 28 Sep 2018 12:08:11 +0200 Subject: [PATCH 02/40] Extract git push from merge request refresh service --- .../merge_requests/refresh_service.rb | 59 ++++++++----------- lib/gitlab/git/push.rb | 17 +++++- 2 files changed, 38 insertions(+), 38 deletions(-) diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb index bcdd752ddc4..d3e4f3def23 100644 --- a/app/services/merge_requests/refresh_service.rb +++ b/app/services/merge_requests/refresh_service.rb @@ -3,17 +3,16 @@ module MergeRequests class RefreshService < MergeRequests::BaseService def execute(oldrev, newrev, ref) - return true unless Gitlab::Git.branch_ref?(ref) + @push = Gitlab::Git::Push.new(@project, oldrev, newrev, ref) - do_execute(oldrev, newrev, ref) + return true unless @push.branch_push? + + refresh_merge_requests! end private - def do_execute(oldrev, newrev, ref) - @oldrev, @newrev = oldrev, newrev - @branch_name = Gitlab::Git.ref_name(ref) - + def refresh_merge_requests! Gitlab::GitalyClient.allow_n_plus_1_calls(&method(:find_new_commits)) # Be sure to close outstanding MRs before reloading them to avoid generating an # empty diff during a manual merge @@ -25,7 +24,7 @@ module MergeRequests cache_merge_requests_closing_issues # Leave a system note if a branch was deleted/added - if branch_added? || branch_removed? + if @push.branch_added? || @push.branch_removed? comment_mr_branch_presence_changed end @@ -54,8 +53,10 @@ module MergeRequests # rubocop: disable CodeReuse/ActiveRecord def post_merge_manually_merged commit_ids = @commits.map(&:id) - merge_requests = @project.merge_requests.preload(:latest_merge_request_diff).opened.where(target_branch: @branch_name).to_a - merge_requests = merge_requests.select(&:diff_head_commit) + merge_requests = @project.merge_requests.opened + .preload(:latest_merge_request_diff) + .where(target_branch: @push.branch_name).to_a + .select(&:diff_head_commit) merge_requests = merge_requests.select do |merge_request| commit_ids.include?(merge_request.diff_head_sha) && @@ -70,24 +71,20 @@ module MergeRequests end # rubocop: enable CodeReuse/ActiveRecord - def force_push? - Gitlab::Checks::ForcePush.force_push?(@project, @oldrev, @newrev) - end - # Refresh merge request diff if we push to source or target branch of merge request # Note: we should update merge requests from forks too # rubocop: disable CodeReuse/ActiveRecord def reload_merge_requests merge_requests = @project.merge_requests.opened - .by_source_or_target_branch(@branch_name).to_a + .by_source_or_target_branch(@push.branch_name).to_a # Fork merge requests merge_requests += MergeRequest.opened - .where(source_branch: @branch_name, source_project: @project) + .where(source_branch: @push.branch_name, source_project: @project) .where.not(target_project: @project).to_a filter_merge_requests(merge_requests).each do |merge_request| - if merge_request.source_branch == @branch_name || force_push? + if merge_request.source_branch == @push.branch_name || @push.force_push? merge_request.reload_diff(current_user) else mr_commit_ids = merge_request.commit_shas @@ -117,7 +114,7 @@ module MergeRequests end def find_new_commits - if branch_added? + if @push.branch_added? @commits = [] merge_request = merge_requests_for_source_branch.first @@ -126,28 +123,28 @@ module MergeRequests begin # Since any number of commits could have been made to the restored branch, # find the common root to see what has been added. - common_ref = @project.repository.merge_base(merge_request.diff_head_sha, @newrev) + common_ref = @project.repository.merge_base(merge_request.diff_head_sha, @push.newrev) # If the a commit no longer exists in this repo, gitlab_git throws # a Rugged::OdbError. This is fixed in https://gitlab.com/gitlab-org/gitlab_git/merge_requests/52 - @commits = @project.repository.commits_between(common_ref, @newrev) if common_ref + @commits = @project.repository.commits_between(common_ref, @push.newrev) if common_ref rescue end - elsif branch_removed? + elsif @push.branch_removed? # No commits for a deleted branch. @commits = [] else - @commits = @project.repository.commits_between(@oldrev, @newrev) + @commits = @project.repository.commits_between(@push.oldrev, @push.newrev) end end # Add comment about branches being deleted or added to merge requests def comment_mr_branch_presence_changed - presence = branch_added? ? :add : :delete + presence = @push.branch_added? ? :add : :delete merge_requests_for_source_branch.each do |merge_request| SystemNoteService.change_branch_presence( merge_request, merge_request.project, @current_user, - :source, @branch_name, presence) + :source, @push.branch_name, presence) end end @@ -164,7 +161,7 @@ module MergeRequests SystemNoteService.add_commits(merge_request, merge_request.project, @current_user, new_commits, - existing_commits, @oldrev) + existing_commits, @push.oldrev) notification_service.push_to_merge_request(merge_request, @current_user, new_commits: new_commits, existing_commits: existing_commits) end @@ -195,7 +192,7 @@ module MergeRequests # Call merge request webhook with update branches def execute_mr_web_hooks merge_requests_for_source_branch.each do |merge_request| - execute_hooks(merge_request, 'update', old_rev: @oldrev) + execute_hooks(merge_request, 'update', old_rev: @push.oldrev) end end @@ -203,7 +200,7 @@ module MergeRequests # `MergeRequestsClosingIssues` model (as a performance optimization). # rubocop: disable CodeReuse/ActiveRecord def cache_merge_requests_closing_issues - @project.merge_requests.where(source_branch: @branch_name).each do |merge_request| + @project.merge_requests.where(source_branch: @push.branch_name).each do |merge_request| merge_request.cache_merge_request_closes_issues!(@current_user) end end @@ -215,15 +212,7 @@ module MergeRequests def merge_requests_for_source_branch(reload: false) @source_merge_requests = nil if reload - @source_merge_requests ||= merge_requests_for(@branch_name) - end - - def branch_added? - Gitlab::Git.blank_ref?(@oldrev) - end - - def branch_removed? - Gitlab::Git.blank_ref?(@newrev) + @source_merge_requests ||= merge_requests_for(@push.branch_name) end end end diff --git a/lib/gitlab/git/push.rb b/lib/gitlab/git/push.rb index d4fe3c623c1..8173af31a0d 100644 --- a/lib/gitlab/git/push.rb +++ b/lib/gitlab/git/push.rb @@ -3,10 +3,17 @@ module Gitlab module Git class Push + attr_reader :oldrev, :newrev + def initialize(project, oldrev, newrev, ref) - @project, @oldrev, @newrev = project, oldrev, newrev - @repository = project.repository - @branch_name = Gitlab::Git.ref_name(ref) + @project = project + @oldrev = oldrev + @newrev = newrev + @ref = ref + end + + def branch_name + @branch_name ||= Gitlab::Git.ref_name(@ref) end def branch_added? @@ -20,6 +27,10 @@ module Gitlab def force_push? Gitlab::Checks::ForcePush.force_push?(@project, @oldrev, @newrev) end + + def branch_push? + Gitlab::Git.branch_ref?(@ref) + end end end end From 4abba28944803d9a818925e62211a0f65458e011 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 28 Sep 2018 14:46:22 +0200 Subject: [PATCH 03/40] Add specs for extracted git push class --- lib/gitlab/git/push.rb | 10 +++- spec/lib/gitlab/git/push_spec.rb | 83 ++++++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+), 2 deletions(-) create mode 100644 spec/lib/gitlab/git/push_spec.rb diff --git a/lib/gitlab/git/push.rb b/lib/gitlab/git/push.rb index 8173af31a0d..b6e414927ea 100644 --- a/lib/gitlab/git/push.rb +++ b/lib/gitlab/git/push.rb @@ -3,6 +3,8 @@ module Gitlab module Git class Push + include Gitlab::Utils::StrongMemoize + attr_reader :oldrev, :newrev def initialize(project, oldrev, newrev, ref) @@ -13,7 +15,9 @@ module Gitlab end def branch_name - @branch_name ||= Gitlab::Git.ref_name(@ref) + strong_memoize(:branch_name) do + Gitlab::Git.branch_name(@ref) + end end def branch_added? @@ -29,7 +33,9 @@ module Gitlab end def branch_push? - Gitlab::Git.branch_ref?(@ref) + strong_memoize(:branch_push) do + Gitlab::Git.branch_ref?(@ref) + end end end end diff --git a/spec/lib/gitlab/git/push_spec.rb b/spec/lib/gitlab/git/push_spec.rb new file mode 100644 index 00000000000..c87e972f31a --- /dev/null +++ b/spec/lib/gitlab/git/push_spec.rb @@ -0,0 +1,83 @@ +require 'spec_helper' + +describe Gitlab::Git::Push do + set(:project) { create(:project, :repository) } + + let(:oldrev) { project.commit('HEAD~10').id } + let(:newrev) { project.commit.id } + let(:ref) { 'refs/heads/some-branch' } + + subject { described_class.new(project, oldrev, newrev, ref) } + + describe '#branch_name' do + context 'when it is a branch push' do + let(:ref) { 'refs/heads/my-branch' } + + it 'returns branch name' do + expect(subject.branch_name).to eq 'my-branch' + end + end + + context 'when it is a tag push' do + let(:ref) { 'refs/tags/my-branch' } + + it 'returns nil' do + expect(subject.branch_name).to be_nil + end + end + end + + describe '#branch_push?' do + context 'when pushing a branch ref' do + let(:ref) { 'refs/heads/my-branch' } + + it { is_expected.to be_branch_push } + end + + context 'when it is a tag push' do + let(:ref) { 'refs/tags/my-branch' } + + it { is_expected.not_to be_branch_push } + end + end + + describe '#force_push?' do + context 'when old revision is an ancestor of the new revision' do + let(:oldrev) { 'HEAD~3' } + let(:newrev) { 'HEAD~1' } + + it { is_expected.not_to be_force_push } + end + + context 'when old revision is not an ancestor of the new revision' do + let(:oldrev) { 'HEAD~3' } + let(:newrev) { '123456' } + + it { is_expected.to be_force_push } + end + end + + describe '#branch_added?' do + context 'when old revision is defined' do + it { is_expected.not_to be_branch_added } + end + + context 'when old revision is not defined' do + let(:oldrev) { Gitlab::Git::BLANK_SHA } + + it { is_expected.to be_branch_added } + end + end + + describe '#branch_removed?' do + context 'when new revision is defined' do + it { is_expected.not_to be_branch_removed } + end + + context 'when new revision is not defined' do + let(:newrev) { Gitlab::Git::BLANK_SHA } + + it { is_expected.to be_branch_removed } + end + end +end From 419d8cc7a2d0e5ffd2f3f8894a739827d0c2c549 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 28 Sep 2018 15:13:04 +0200 Subject: [PATCH 04/40] Calculate modified paths of a git push operation --- lib/gitlab/git/diff_stats_collection.rb | 4 ++ lib/gitlab/git/push.rb | 14 ++++++ .../gitlab/git/diff_stats_collection_spec.rb | 8 ++- spec/lib/gitlab/git/push_spec.rb | 49 ++++++++++++++++++- 4 files changed, 72 insertions(+), 3 deletions(-) diff --git a/lib/gitlab/git/diff_stats_collection.rb b/lib/gitlab/git/diff_stats_collection.rb index d4033f56387..998c41497a2 100644 --- a/lib/gitlab/git/diff_stats_collection.rb +++ b/lib/gitlab/git/diff_stats_collection.rb @@ -18,6 +18,10 @@ module Gitlab indexed_by_path[path] end + def paths + @collection.map(&:path) + end + private def indexed_by_path diff --git a/lib/gitlab/git/push.rb b/lib/gitlab/git/push.rb index b6e414927ea..603f860e4f8 100644 --- a/lib/gitlab/git/push.rb +++ b/lib/gitlab/git/push.rb @@ -28,6 +28,10 @@ module Gitlab Gitlab::Git.blank_ref?(@newrev) end + def branch_updated? + branch_push? && !branch_added? && !branch_removed? + end + def force_push? Gitlab::Checks::ForcePush.force_push?(@project, @oldrev, @newrev) end @@ -37,6 +41,16 @@ module Gitlab Gitlab::Git.branch_ref?(@ref) end end + + def modified_paths + unless branch_updated? + raise ArgumentError, 'Unable to calculate modified paths!' + end + + strong_memoize(:modified_paths) do + @project.repository.diff_stats(@oldrev, @newrev).paths + end + end end end end diff --git a/spec/lib/gitlab/git/diff_stats_collection_spec.rb b/spec/lib/gitlab/git/diff_stats_collection_spec.rb index 89927cbb3a6..b07690ef39c 100644 --- a/spec/lib/gitlab/git/diff_stats_collection_spec.rb +++ b/spec/lib/gitlab/git/diff_stats_collection_spec.rb @@ -14,7 +14,7 @@ describe Gitlab::Git::DiffStatsCollection do let(:diff_stats) { [stats_a, stats_b] } let(:collection) { described_class.new(diff_stats) } - describe '.find_by_path' do + describe '#find_by_path' do it 'returns stats by path when found' do expect(collection.find_by_path('foo')).to eq(stats_a) end @@ -23,4 +23,10 @@ describe Gitlab::Git::DiffStatsCollection do expect(collection.find_by_path('no-file')).to be_nil end end + + describe '#paths' do + it 'returns only modified paths' do + expect(collection.paths).to eq %w[foo bar] + end + end end diff --git a/spec/lib/gitlab/git/push_spec.rb b/spec/lib/gitlab/git/push_spec.rb index c87e972f31a..f19e05c4451 100644 --- a/spec/lib/gitlab/git/push_spec.rb +++ b/spec/lib/gitlab/git/push_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe Gitlab::Git::Push do set(:project) { create(:project, :repository) } - let(:oldrev) { project.commit('HEAD~10').id } + let(:oldrev) { project.commit('HEAD~2').id } let(:newrev) { project.commit.id } let(:ref) { 'refs/heads/some-branch' } @@ -35,12 +35,36 @@ describe Gitlab::Git::Push do end context 'when it is a tag push' do - let(:ref) { 'refs/tags/my-branch' } + let(:ref) { 'refs/tags/my-tag' } it { is_expected.not_to be_branch_push } end end + describe '#branch_updated?' do + context 'when it is a branch push with correct old and new revisions' do + it { is_expected.to be_branch_updated } + end + + context 'when it is not a branch push' do + let(:ref) { 'refs/tags/my-tag' } + + it { is_expected.not_to be_branch_updated } + end + + context 'when old revision is blank' do + let(:oldrev) { Gitlab::Git::BLANK_SHA } + + it { is_expected.not_to be_branch_updated } + end + + context 'when it is not a branch push' do + let(:newrev) { Gitlab::Git::BLANK_SHA } + + it { is_expected.not_to be_branch_updated } + end + end + describe '#force_push?' do context 'when old revision is an ancestor of the new revision' do let(:oldrev) { 'HEAD~3' } @@ -80,4 +104,25 @@ describe Gitlab::Git::Push do it { is_expected.to be_branch_removed } end end + + describe '#modified_paths' do + context 'when a push is a branch update' do + let(:oldrev) { '281d3a76f31c812dbf48abce82ccf6860adedd81' } + let(:new_rev) { '1b12f15a11fc6e62177bef08f47bc7b5ce50b141' } + + it 'returns modified paths' do + expect(subject.modified_paths).to eq ['bar/branch-test.txt', + 'files/js/commit.coffee', + 'with space/README.md'] + end + end + + context 'when a push is not a branch update' do + let(:oldrev) { Gitlab::Git::BLANK_SHA } + + it 'raises an error' do + expect { subject.modified_paths }.to raise_error(ArgumentError) + end + end + end end From 9bad9b0a68074d4b0fe5de7f3c5cefd4a808c726 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 28 Sep 2018 15:38:36 +0200 Subject: [PATCH 05/40] Improve specs for git push class This helps to prevent problems with off-by-one commmit problem in `Gitlab::Git::Push#modified_paths`. --- spec/lib/gitlab/git/push_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/lib/gitlab/git/push_spec.rb b/spec/lib/gitlab/git/push_spec.rb index f19e05c4451..2a7a357f384 100644 --- a/spec/lib/gitlab/git/push_spec.rb +++ b/spec/lib/gitlab/git/push_spec.rb @@ -107,8 +107,8 @@ describe Gitlab::Git::Push do describe '#modified_paths' do context 'when a push is a branch update' do - let(:oldrev) { '281d3a76f31c812dbf48abce82ccf6860adedd81' } - let(:new_rev) { '1b12f15a11fc6e62177bef08f47bc7b5ce50b141' } + let(:newrev) { '498214d' } + let(:oldrev) { '281d3a7' } it 'returns modified paths' do expect(subject.modified_paths).to eq ['bar/branch-test.txt', From 1c4187e38e09c8dfc4d0cdbd4c702aff5d695acb Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 2 Oct 2018 13:01:55 +0200 Subject: [PATCH 06/40] Treat nil git push revisons as a blank Git SHA value --- lib/gitlab/git/push.rb | 4 ++-- spec/lib/gitlab/git/push_spec.rb | 38 ++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/git/push.rb b/lib/gitlab/git/push.rb index 603f860e4f8..7c1309721fd 100644 --- a/lib/gitlab/git/push.rb +++ b/lib/gitlab/git/push.rb @@ -9,8 +9,8 @@ module Gitlab def initialize(project, oldrev, newrev, ref) @project = project - @oldrev = oldrev - @newrev = newrev + @oldrev = oldrev.presence || Gitlab::Git::BLANK_SHA + @newrev = newrev.presence || Gitlab::Git::BLANK_SHA @ref = ref end diff --git a/spec/lib/gitlab/git/push_spec.rb b/spec/lib/gitlab/git/push_spec.rb index 2a7a357f384..566c8209504 100644 --- a/spec/lib/gitlab/git/push_spec.rb +++ b/spec/lib/gitlab/git/push_spec.rb @@ -63,6 +63,12 @@ describe Gitlab::Git::Push do it { is_expected.not_to be_branch_updated } end + + context 'when oldrev is nil' do + let(:oldrev) { nil } + + it { is_expected.not_to be_branch_updated } + end end describe '#force_push?' do @@ -125,4 +131,36 @@ describe Gitlab::Git::Push do end end end + + describe '#oldrev' do + context 'when a valid oldrev is provided' do + it 'returns oldrev' do + expect(subject.oldrev).to eq oldrev + end + end + + context 'when a nil valud is provided' do + let(:oldrev) { nil } + + it 'returns blank SHA' do + expect(subject.oldrev).to eq Gitlab::Git::BLANK_SHA + end + end + end + + describe '#newrev' do + context 'when valid newrev is provided' do + it 'returns newrev' do + expect(subject.newrev).to eq newrev + end + end + + context 'when a nil valud is provided' do + let(:newrev) { nil } + + it 'returns blank SHA' do + expect(subject.newrev).to eq Gitlab::Git::BLANK_SHA + end + end + end end From 1490933090166d71ce009708a70a16b156415052 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 2 Oct 2018 13:10:56 +0200 Subject: [PATCH 07/40] Expose paths modified by a push from a pipeline class --- app/models/ci/pipeline.rb | 28 ++++++++++++++++++ spec/models/ci/pipeline_spec.rb | 51 +++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+) diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 6dac577c514..c0fb79a37e2 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -627,6 +627,18 @@ module Ci end end + def branch_updated? + strong_memoize(:branch_updated) do + push_details.branch_updated? + end + end + + def modified_paths + strong_memoize(:changes) do + push_details.modified_paths + end + end + private def ci_yaml_from_repo @@ -650,6 +662,22 @@ module Ci Gitlab::DataBuilder::Pipeline.build(self) end + def push_details + strong_memoize(:push_details) do + Gitlab::Git::Push.new(project, before_sha, sha, push_ref) + end + end + + def push_ref + if branch? + Gitlab::Git::BRANCH_REF_PREFIX + ref.to_s + elsif tag? + Gitlab::Git::TAG_REF_PREFIX + ref.to_s + else + raise ArgumentError, 'Invalid pipeline type!' + end + end + def latest_builds_status return 'failed' unless yaml_errors.blank? diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 4755702c0e9..1060417b03b 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -825,6 +825,57 @@ describe Ci::Pipeline, :mailer do end end + describe '#branch_updated?' do + context 'when pipeline has before SHA' do + before do + pipeline.update_column(:before_sha, 'a1b2c3d4') + end + + it 'runs on a branch update push' do + expect(pipeline.before_sha).not_to be Gitlab::Git::BLANK_SHA + expect(pipeline.branch_updated?).to be true + end + end + + context 'when pipeline does not have before SHA' do + before do + pipeline.update_column(:before_sha, Gitlab::Git::BLANK_SHA) + end + + it 'does not run on a branch updating push' do + expect(pipeline.branch_updated?).to be false + end + end + end + + describe '#modified_paths' do + context 'when old and new revisions are set' do + let(:project) { create(:project, :repository) } + + before do + pipeline.update(before_sha: '1234abcd', sha: '2345bcde') + end + + it 'fetches stats for changes between commits' do + expect(project.repository) + .to receive(:diff_stats).with('1234abcd', '2345bcde') + .and_call_original + + pipeline.modified_paths + end + end + + context 'when either old or new revision is missing' do + before do + pipeline.update_column(:before_sha, Gitlab::Git::BLANK_SHA) + end + + it 'raises an error' do + expect { pipeline.modified_paths }.to raise_error(ArgumentError) + end + end + end + describe '#has_kubernetes_active?' do context 'when kubernetes is active' do shared_examples 'same behavior between KubernetesService and Platform::Kubernetes' do From 740ee583b3b0950c9b8ab5346e749b52355ec416 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 2 Oct 2018 14:03:20 +0200 Subject: [PATCH 08/40] Make it possible to specifiy only: changes keywords --- lib/gitlab/ci/config/entry/policy.rb | 6 ++++-- .../lib/gitlab/ci/config/entry/policy_spec.rb | 20 ++++++++++++++++++- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/lib/gitlab/ci/config/entry/policy.rb b/lib/gitlab/ci/config/entry/policy.rb index 09e8e52b60f..6ba2d9bbf50 100644 --- a/lib/gitlab/ci/config/entry/policy.rb +++ b/lib/gitlab/ci/config/entry/policy.rb @@ -25,17 +25,19 @@ module Gitlab include Entry::Validatable include Entry::Attributable - attributes :refs, :kubernetes, :variables + ALLOWED_KEYS = %i[refs kubernetes variables changes] + attributes :refs, :kubernetes, :variables, :changes validations do validates :config, presence: true - validates :config, allowed_keys: %i[refs kubernetes variables] + validates :config, allowed_keys: ALLOWED_KEYS validate :variables_expressions_syntax with_options allow_nil: true do validates :refs, array_of_strings_or_regexps: true validates :kubernetes, allowed_values: %w[active] validates :variables, array_of_strings: true + validates :changes, array_of_strings: true end def variables_expressions_syntax diff --git a/spec/lib/gitlab/ci/config/entry/policy_spec.rb b/spec/lib/gitlab/ci/config/entry/policy_spec.rb index 83d39b82068..bef93fe7af7 100644 --- a/spec/lib/gitlab/ci/config/entry/policy_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/policy_spec.rb @@ -1,4 +1,5 @@ -require 'spec_helper' +require 'fast_spec_helper' +require_dependency 'active_model' describe Gitlab::Ci::Config::Entry::Policy do let(:entry) { described_class.new(config) } @@ -124,6 +125,23 @@ describe Gitlab::Ci::Config::Entry::Policy do end end + context 'when specifying a valid changes policy' do + let(:config) { { changes: %w[some/* paths/**/*.rb] } } + + it 'is a correct configuraton' do + expect(entry).to be_valid + expect(entry.value).to eq(config) + end + end + + context 'when changes policy is invalid' do + let(:config) { { changes: [1, 2] } } + + it 'returns errors' do + expect(entry.errors).to include /changes should be an array of strings/ + end + end + context 'when specifying unknown policy' do let(:config) { { refs: ['master'], invalid: :something } } From 0f78ceca1bb5b9db9760a52a20b96e84892bd9ad Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 2 Oct 2018 14:48:18 +0200 Subject: [PATCH 09/40] Add only/except pipeline build policy for `changes` --- lib/gitlab/ci/build/policy/changes.rb | 21 ++++++ .../gitlab/ci/build/policy/changes_spec.rb | 69 +++++++++++++++++++ 2 files changed, 90 insertions(+) create mode 100644 lib/gitlab/ci/build/policy/changes.rb create mode 100644 spec/lib/gitlab/ci/build/policy/changes_spec.rb diff --git a/lib/gitlab/ci/build/policy/changes.rb b/lib/gitlab/ci/build/policy/changes.rb new file mode 100644 index 00000000000..4bcb27ad73b --- /dev/null +++ b/lib/gitlab/ci/build/policy/changes.rb @@ -0,0 +1,21 @@ +module Gitlab + module Ci + module Build + module Policy + class Changes < Policy::Specification + def initialize(globs) + @globs = Array(globs) + end + + def satisfied_by?(pipeline, seed) + return true unless pipeline.branch_updated? + + pipeline.modified_paths.any? do |path| + @globs.any? { |glob| File.fnmatch?(glob, path, File::FNM_PATHNAME) } + end + end + end + end + end + end +end diff --git a/spec/lib/gitlab/ci/build/policy/changes_spec.rb b/spec/lib/gitlab/ci/build/policy/changes_spec.rb new file mode 100644 index 00000000000..1eb1fbb67e7 --- /dev/null +++ b/spec/lib/gitlab/ci/build/policy/changes_spec.rb @@ -0,0 +1,69 @@ +require 'spec_helper' + +describe Gitlab::Ci::Build::Policy::Changes do + set(:project) { create(:project) } + + let(:pipeline) do + build(:ci_empty_pipeline, project: project, + ref: 'master', + source: :push, + sha: '1234abcd', + before_sha: '0123aabb') + end + + let(:ci_build) do + build(:ci_build, pipeline: pipeline, project: project, ref: 'master') + end + + let(:seed) { double('build seed', to_resource: ci_build) } + + before do + allow(pipeline).to receive(:modified_paths) do + %w[some/modified/ruby/file.rb some/other_file.txt] + end + end + + describe '#satisfied_by?' do + it 'is satisfied by matching literal path' do + policy = described_class.new(%w[some/other_file.txt]) + + expect(policy).to be_satisfied_by(pipeline, seed) + end + + it 'is satisfied by matching simple pattern' do + policy = described_class.new(%w[some/*.txt]) + + expect(policy).to be_satisfied_by(pipeline, seed) + end + + it 'is satisfied by matching recusive pattern' do + policy = described_class.new(%w[some/**/*.rb]) + + expect(policy).to be_satisfied_by(pipeline, seed) + end + + it 'is not satisfied when pattern does not match path' do + policy = described_class.new(%w[some/*.rb]) + + expect(policy).not_to be_satisfied_by(pipeline, seed) + end + + it 'is not satisfied when pattern does not match' do + policy = described_class.new(%w[invalid/*.md]) + + expect(policy).not_to be_satisfied_by(pipeline, seed) + end + + context 'when pipelines does not run for a branch update' do + before do + pipeline.before_sha = Gitlab::Git::BLANK_SHA + end + + it 'is always satisfied' do + policy = described_class.new(%w[invalid/*]) + + expect(policy).to be_satisfied_by(pipeline, seed) + end + end + end +end From b772e7f4c63ffa12cba5df200352b509a26f3261 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 2 Oct 2018 15:04:32 +0200 Subject: [PATCH 10/40] Match a dot in paths configured for only: changes --- lib/gitlab/ci/build/policy/changes.rb | 4 +++- spec/lib/gitlab/ci/build/policy/changes_spec.rb | 8 +++++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/ci/build/policy/changes.rb b/lib/gitlab/ci/build/policy/changes.rb index 4bcb27ad73b..dc4f07bf05b 100644 --- a/lib/gitlab/ci/build/policy/changes.rb +++ b/lib/gitlab/ci/build/policy/changes.rb @@ -11,7 +11,9 @@ module Gitlab return true unless pipeline.branch_updated? pipeline.modified_paths.any? do |path| - @globs.any? { |glob| File.fnmatch?(glob, path, File::FNM_PATHNAME) } + @globs.any? do |glob| + File.fnmatch?(glob, path, File::FNM_PATHNAME | File::FNM_DOTMATCH) + end end end end diff --git a/spec/lib/gitlab/ci/build/policy/changes_spec.rb b/spec/lib/gitlab/ci/build/policy/changes_spec.rb index 1eb1fbb67e7..0ac611904cb 100644 --- a/spec/lib/gitlab/ci/build/policy/changes_spec.rb +++ b/spec/lib/gitlab/ci/build/policy/changes_spec.rb @@ -19,7 +19,7 @@ describe Gitlab::Ci::Build::Policy::Changes do before do allow(pipeline).to receive(:modified_paths) do - %w[some/modified/ruby/file.rb some/other_file.txt] + %w[some/modified/ruby/file.rb some/other_file.txt some/.dir/file] end end @@ -42,6 +42,12 @@ describe Gitlab::Ci::Build::Policy::Changes do expect(policy).to be_satisfied_by(pipeline, seed) end + it 'is satisfied by matching a pattern with a dot' do + policy = described_class.new(%w[some/*/file]) + + expect(policy).to be_satisfied_by(pipeline, seed) + end + it 'is not satisfied when pattern does not match path' do policy = described_class.new(%w[some/*.rb]) From c945112093d28437d3de6a75a3e4983be7198afe Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 2 Oct 2018 15:56:17 +0200 Subject: [PATCH 11/40] Add Gitaly integration tests for only: changes feature --- .../gitlab/ci/build/policy/changes_spec.rb | 154 +++++++++++------- spec/lib/gitlab/ci/yaml_processor_spec.rb | 10 +- 2 files changed, 102 insertions(+), 62 deletions(-) diff --git a/spec/lib/gitlab/ci/build/policy/changes_spec.rb b/spec/lib/gitlab/ci/build/policy/changes_spec.rb index 0ac611904cb..ab401108c84 100644 --- a/spec/lib/gitlab/ci/build/policy/changes_spec.rb +++ b/spec/lib/gitlab/ci/build/policy/changes_spec.rb @@ -3,73 +3,105 @@ require 'spec_helper' describe Gitlab::Ci::Build::Policy::Changes do set(:project) { create(:project) } - let(:pipeline) do - build(:ci_empty_pipeline, project: project, - ref: 'master', - source: :push, - sha: '1234abcd', - before_sha: '0123aabb') - end - - let(:ci_build) do - build(:ci_build, pipeline: pipeline, project: project, ref: 'master') - end - - let(:seed) { double('build seed', to_resource: ci_build) } - - before do - allow(pipeline).to receive(:modified_paths) do - %w[some/modified/ruby/file.rb some/other_file.txt some/.dir/file] - end - end - describe '#satisfied_by?' do - it 'is satisfied by matching literal path' do - policy = described_class.new(%w[some/other_file.txt]) - - expect(policy).to be_satisfied_by(pipeline, seed) - end - - it 'is satisfied by matching simple pattern' do - policy = described_class.new(%w[some/*.txt]) - - expect(policy).to be_satisfied_by(pipeline, seed) - end - - it 'is satisfied by matching recusive pattern' do - policy = described_class.new(%w[some/**/*.rb]) - - expect(policy).to be_satisfied_by(pipeline, seed) - end - - it 'is satisfied by matching a pattern with a dot' do - policy = described_class.new(%w[some/*/file]) - - expect(policy).to be_satisfied_by(pipeline, seed) - end - - it 'is not satisfied when pattern does not match path' do - policy = described_class.new(%w[some/*.rb]) - - expect(policy).not_to be_satisfied_by(pipeline, seed) - end - - it 'is not satisfied when pattern does not match' do - policy = described_class.new(%w[invalid/*.md]) - - expect(policy).not_to be_satisfied_by(pipeline, seed) - end - - context 'when pipelines does not run for a branch update' do - before do - pipeline.before_sha = Gitlab::Git::BLANK_SHA + describe 'paths matching matching' do + let(:pipeline) do + build(:ci_empty_pipeline, project: project, + ref: 'master', + source: :push, + sha: '1234abcd', + before_sha: '0123aabb') end - it 'is always satisfied' do - policy = described_class.new(%w[invalid/*]) + let(:ci_build) do + build(:ci_build, pipeline: pipeline, project: project, ref: 'master') + end + + let(:seed) { double('build seed', to_resource: ci_build) } + + before do + allow(pipeline).to receive(:modified_paths) do + %w[some/modified/ruby/file.rb some/other_file.txt some/.dir/file] + end + end + + it 'is satisfied by matching literal path' do + policy = described_class.new(%w[some/other_file.txt]) expect(policy).to be_satisfied_by(pipeline, seed) end + + it 'is satisfied by matching simple pattern' do + policy = described_class.new(%w[some/*.txt]) + + expect(policy).to be_satisfied_by(pipeline, seed) + end + + it 'is satisfied by matching recusive pattern' do + policy = described_class.new(%w[some/**/*.rb]) + + expect(policy).to be_satisfied_by(pipeline, seed) + end + + it 'is satisfied by matching a pattern with a dot' do + policy = described_class.new(%w[some/*/file]) + + expect(policy).to be_satisfied_by(pipeline, seed) + end + + it 'is not satisfied when pattern does not match path' do + policy = described_class.new(%w[some/*.rb]) + + expect(policy).not_to be_satisfied_by(pipeline, seed) + end + + it 'is not satisfied when pattern does not match' do + policy = described_class.new(%w[invalid/*.md]) + + expect(policy).not_to be_satisfied_by(pipeline, seed) + end + + context 'when pipelines does not run for a branch update' do + before do + pipeline.before_sha = Gitlab::Git::BLANK_SHA + end + + it 'is always satisfied' do + policy = described_class.new(%w[invalid/*]) + + expect(policy).to be_satisfied_by(pipeline, seed) + end + end + end + + describe 'gitaly integration' do + set(:project) { create(:project, :repository) } + + let(:pipeline) do + create(:ci_empty_pipeline, project: project, + ref: 'master', + source: :push, + sha: '498214d', + before_sha: '281d3a7') + end + + let(:build) do + create(:ci_build, pipeline: pipeline, project: project, ref: 'master') + end + + let(:seed) { double('build seed', to_resource: build) } + + it 'is satisfied by changes introduced by a push' do + policy = described_class.new(['with space/*.md']) + + expect(policy).to be_satisfied_by(pipeline, seed) + end + + it 'is not satisfied by changes that are not in the push' do + policy = described_class.new(%w[files/js/commit.js]) + + expect(policy).not_to be_satisfied_by(pipeline, seed) + end end end end diff --git a/spec/lib/gitlab/ci/yaml_processor_spec.rb b/spec/lib/gitlab/ci/yaml_processor_spec.rb index a2d429fa859..564635cec2b 100644 --- a/spec/lib/gitlab/ci/yaml_processor_spec.rb +++ b/spec/lib/gitlab/ci/yaml_processor_spec.rb @@ -1354,7 +1354,7 @@ module Gitlab end.to raise_error(Gitlab::Ci::YamlProcessor::ValidationError, "jobs:rspec dependencies should be an array of strings") end - it 'returns errors if pipeline variables expression is invalid' do + it 'returns errors if pipeline variables expression policy is invalid' do config = YAML.dump({ rspec: { script: 'test', only: { variables: ['== null'] } } }) expect { Gitlab::Ci::YamlProcessor.new(config) } @@ -1362,6 +1362,14 @@ module Gitlab 'jobs:rspec:only variables invalid expression syntax') end + it 'returns errors if pipeline changes policy is invalid' do + config = YAML.dump({ rspec: { script: 'test', only: { changes: [1] } } }) + + expect { Gitlab::Ci::YamlProcessor.new(config) } + .to raise_error(Gitlab::Ci::YamlProcessor::ValidationError, + 'jobs:rspec:only changes should be an array of strings') + end + it 'returns errors if extended hash configuration is invalid' do config = YAML.dump({ rspec: { extends: 'something', script: 'test' } }) From 23512484ef08557b39ef82d0b767b76a33111308 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 2 Oct 2018 15:56:56 +0200 Subject: [PATCH 12/40] Freeze mutable constant in CI entry policy class --- lib/gitlab/ci/config/entry/policy.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gitlab/ci/config/entry/policy.rb b/lib/gitlab/ci/config/entry/policy.rb index 6ba2d9bbf50..c92562f8c85 100644 --- a/lib/gitlab/ci/config/entry/policy.rb +++ b/lib/gitlab/ci/config/entry/policy.rb @@ -25,7 +25,7 @@ module Gitlab include Entry::Validatable include Entry::Attributable - ALLOWED_KEYS = %i[refs kubernetes variables changes] + ALLOWED_KEYS = %i[refs kubernetes variables changes].freeze attributes :refs, :kubernetes, :variables, :changes validations do From 0972dbc799673654fed6f6507818bbf8bafe33a8 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 2 Oct 2018 15:59:08 +0200 Subject: [PATCH 13/40] Add frozen strong literal directive to policy changes class --- lib/gitlab/ci/build/policy/changes.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/gitlab/ci/build/policy/changes.rb b/lib/gitlab/ci/build/policy/changes.rb index dc4f07bf05b..7bf51519752 100644 --- a/lib/gitlab/ci/build/policy/changes.rb +++ b/lib/gitlab/ci/build/policy/changes.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module Gitlab module Ci module Build From 0dd892aaa51e5e82c908dcbec5aef0082a9dd2c5 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 2 Oct 2018 16:00:16 +0200 Subject: [PATCH 14/40] Add changelog entry for only: changes feature --- .../feature-gb-pipeline-only-except-with-modified-paths.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/feature-gb-pipeline-only-except-with-modified-paths.yml diff --git a/changelogs/unreleased/feature-gb-pipeline-only-except-with-modified-paths.yml b/changelogs/unreleased/feature-gb-pipeline-only-except-with-modified-paths.yml new file mode 100644 index 00000000000..62676cdad62 --- /dev/null +++ b/changelogs/unreleased/feature-gb-pipeline-only-except-with-modified-paths.yml @@ -0,0 +1,5 @@ +--- +title: Add support for pipeline only/except policy for modified paths +merge_request: 21981 +author: +type: added From a52960f5a1aa039a57c17a577aeb9fb82e238f4a Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 2 Oct 2018 16:13:49 +0200 Subject: [PATCH 15/40] Add basic documentation for only: changes feature --- doc/ci/yaml/README.md | 37 ++++++++++++++++++++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/doc/ci/yaml/README.md b/doc/ci/yaml/README.md index e38628b288b..d1ae7326852 100644 --- a/doc/ci/yaml/README.md +++ b/doc/ci/yaml/README.md @@ -387,6 +387,8 @@ except master. > `refs` and `kubernetes` policies introduced in GitLab 10.0 > > `variables` policy introduced in 10.7 +> +> `changes` policy introduced in 11.4 CAUTION: **Warning:** This an _alpha_ feature, and it it subject to change at any time without @@ -398,10 +400,15 @@ policy configuration. GitLab now supports both, simple and complex strategies, so it is possible to use an array and a hash configuration scheme. -Three keys are now available: `refs`, `kubernetes` and `variables`. +Four keys are now available: `refs`, `kubernetes` and `variables` and `changes`. + +### `refs` and `kubernetes` + Refs strategy equals to simplified only/except configuration, whereas kubernetes strategy accepts only `active` keyword. +### `variables` + `variables` keyword is used to define variables expressions. In other words you can use predefined variables / project / group or environment-scoped variables to define an expression GitLab is going to @@ -445,6 +452,34 @@ end-to-end: Learn more about variables expressions on [a separate page][variables-expressions]. +### `changes` + +Using `changes` keyword with `only` or `except` makes it possible to define if +a job should be created, based on files modified by a git push event. + +```yaml +docker build: + script: docker build -t my-image:$CI_COMMIT_REF_SLUG . + only: + changes: + - Dockerfile + - docker/scripts/* +``` + +In the scenario above, if you are pushing multiple commits to GitLab, to an +exising branch, GitLab is going to create and trigger `docker build` if one of +the commits contains changes to the `Dockerfile` file or any of the files +inside `docker/scripts/` directory. + +CAUTION: **Warning:** +If you are pushing a **new** branch or a tag to GitLab, only/changes is going +to always evaluate to truth and GitLab will create a job. This feature is not +connected with merge requests yet, GitLab is creating pipelines before an user +creates a merge requests and specifies a target branch. Without a target branch +it is not possible to know what the common ancestor is in case of pushing a new +branch, thus we always create a job in that case. This feature works best for +stable branches like `master`. + ## `tags` `tags` is used to select specific Runners from the list of all Runners that are From 09075759a428220ddfb5dacf6a6974c11956e391 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 2 Oct 2018 16:21:53 +0200 Subject: [PATCH 16/40] Copy-edit docs for only: changes feature --- doc/ci/yaml/README.md | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/doc/ci/yaml/README.md b/doc/ci/yaml/README.md index d1ae7326852..ef9a00266e1 100644 --- a/doc/ci/yaml/README.md +++ b/doc/ci/yaml/README.md @@ -467,18 +467,19 @@ docker build: ``` In the scenario above, if you are pushing multiple commits to GitLab, to an -exising branch, GitLab is going to create and trigger `docker build` if one of -the commits contains changes to the `Dockerfile` file or any of the files -inside `docker/scripts/` directory. +exising branch, GitLab is going to create and trigger `docker build` job, +provided that one of the commits contains changes to the `Dockerfile` file or +changes to any of the files inside `docker/scripts/` directory. CAUTION: **Warning:** -If you are pushing a **new** branch or a tag to GitLab, only/changes is going -to always evaluate to truth and GitLab will create a job. This feature is not -connected with merge requests yet, GitLab is creating pipelines before an user -creates a merge requests and specifies a target branch. Without a target branch -it is not possible to know what the common ancestor is in case of pushing a new -branch, thus we always create a job in that case. This feature works best for -stable branches like `master`. +If you are pushing a **new** branch or a new tag to GitLab, only/changes is +going to always evaluate to truth and GitLab will create a job. This feature is +not combined with merge requests yet, and because GitLab is creating pipelines +before an user can create a merge request we don't know a target branch at +this point. Without a target branchit is not possible to know what the common +ancestor is, thus we always create a job in that case. This feature works best for +stable branches like `master` because in that case GitLab uses previous commit, +that is present in a branch, to compare against a newly pushed latest SHA. ## `tags` From 4000358531668e07021160e436f3cdc1b873e9d7 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 3 Oct 2018 12:21:41 +0200 Subject: [PATCH 17/40] Few minor fixed in code and docs for only: changes --- app/models/ci/pipeline.rb | 2 +- doc/ci/yaml/README.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index c0fb79a37e2..a51745d41bb 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -634,7 +634,7 @@ module Ci end def modified_paths - strong_memoize(:changes) do + strong_memoize(:modified_paths) do push_details.modified_paths end end diff --git a/doc/ci/yaml/README.md b/doc/ci/yaml/README.md index ef9a00266e1..0d07b63a2dc 100644 --- a/doc/ci/yaml/README.md +++ b/doc/ci/yaml/README.md @@ -476,7 +476,7 @@ If you are pushing a **new** branch or a new tag to GitLab, only/changes is going to always evaluate to truth and GitLab will create a job. This feature is not combined with merge requests yet, and because GitLab is creating pipelines before an user can create a merge request we don't know a target branch at -this point. Without a target branchit is not possible to know what the common +this point. Without a target branch it is not possible to know what the common ancestor is, thus we always create a job in that case. This feature works best for stable branches like `master` because in that case GitLab uses previous commit, that is present in a branch, to compare against a newly pushed latest SHA. From a1c3d40739ed133e1ca1cd9191628acf938809cf Mon Sep 17 00:00:00 2001 From: Jacopo Date: Wed, 3 Oct 2018 13:00:03 +0200 Subject: [PATCH 18/40] Allows to filter issues by `Any milestone` in the API In GET `api/v4/projects/:id/issues` the user can filter issues that have an assigned milestone through the parameter `milestone=Any+Milestone`. --- app/finders/issuable_finder.rb | 6 ++++++ app/models/concerns/issuable.rb | 1 + .../unreleased/51748-filter-any-milestone-via-api.yml | 5 +++++ doc/api/issues.md | 2 +- spec/finders/issues_finder_spec.rb | 8 ++++++++ spec/requests/api/issues_spec.rb | 10 ++++++++++ 6 files changed, 31 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/51748-filter-any-milestone-via-api.yml diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb index 0209a1397b9..794c2f293c4 100644 --- a/app/finders/issuable_finder.rb +++ b/app/finders/issuable_finder.rb @@ -424,6 +424,10 @@ class IssuableFinder params[:milestone_title] == Milestone::Upcoming.name end + def filter_by_any_milestone? + params[:milestone_title] == Milestone::Any.title + end + def filter_by_started_milestone? params[:milestone_title] == Milestone::Started.name end @@ -433,6 +437,8 @@ class IssuableFinder if milestones? if filter_by_no_milestone? items = items.left_joins_milestones.where(milestone_id: [-1, nil]) + elsif filter_by_any_milestone? + items = items.any_milestone elsif filter_by_upcoming_milestone? upcoming_ids = Milestone.upcoming_ids_by_projects(projects(items)) items = items.left_joins_milestones.where(milestone_id: upcoming_ids) diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 5f65fceb7af..2aa52bbaeea 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -76,6 +76,7 @@ module Issuable scope :recent, -> { reorder(id: :desc) } scope :of_projects, ->(ids) { where(project_id: ids) } scope :of_milestones, ->(ids) { where(milestone_id: ids) } + scope :any_milestone, -> { where('milestone_id IS NOT NULL') } scope :with_milestone, ->(title) { left_joins_milestones.where(milestones: { title: title }) } scope :opened, -> { with_state(:opened) } scope :only_opened, -> { with_state(:opened) } diff --git a/changelogs/unreleased/51748-filter-any-milestone-via-api.yml b/changelogs/unreleased/51748-filter-any-milestone-via-api.yml new file mode 100644 index 00000000000..30304e5a4ac --- /dev/null +++ b/changelogs/unreleased/51748-filter-any-milestone-via-api.yml @@ -0,0 +1,5 @@ +--- +title: Allows to filter issues by Any milestone in the API +merge_request: 22080 +author: Jacopo Beschi @jacopo-beschi +type: added diff --git a/doc/api/issues.md b/doc/api/issues.md index f4c0f4ea65b..cc1d6834a20 100644 --- a/doc/api/issues.md +++ b/doc/api/issues.md @@ -37,7 +37,7 @@ GET /issues?my_reaction_emoji=star | ------------------- | ---------------- | ---------- | --------------------------------------------------------------------------------------------------------------------------------------------------- | | `state` | string | no | Return all issues or just those that are `opened` or `closed` | | `labels` | string | no | Comma-separated list of label names, issues must have all labels to be returned. `No+Label` lists all issues with no labels | -| `milestone` | string | no | The milestone title. `No+Milestone` lists all issues with no milestone | +| `milestone` | string | no | The milestone title. `No+Milestone` lists all issues with no milestone. `Any+Milestone` lists all issues that have an assigned milestone | | `scope` | string | no | Return issues for the given scope: `created_by_me`, `assigned_to_me` or `all`. Defaults to `created_by_me`
For versions before 11.0, use the now deprecated `created-by-me` or `assigned-to-me` scopes instead.
_([Introduced][ce-13004] in GitLab 9.5. [Changed to snake_case][ce-18935] in GitLab 11.0)_ | | `author_id` | integer | no | Return issues created by the given user `id`. Combine with `scope=all` or `scope=assigned_to_me`. _([Introduced][ce-13004] in GitLab 9.5)_ | | `assignee_id` | integer | no | Return issues assigned to the given user `id` _([Introduced][ce-13004] in GitLab 9.5)_ | diff --git a/spec/finders/issues_finder_spec.rb b/spec/finders/issues_finder_spec.rb index d78451112ec..0689c843104 100644 --- a/spec/finders/issues_finder_spec.rb +++ b/spec/finders/issues_finder_spec.rb @@ -125,6 +125,14 @@ describe IssuesFinder do end end + context 'filtering by any milestone' do + let(:params) { { milestone_title: Milestone::Any.title } } + + it 'returns issues with any assigned milestone' do + expect(issues).to contain_exactly(issue1) + end + end + context 'filtering by upcoming milestone' do let(:params) { { milestone_title: Milestone::Upcoming.name } } diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb index 1e2e13a723c..9f6cf12f9a7 100644 --- a/spec/requests/api/issues_spec.rb +++ b/spec/requests/api/issues_spec.rb @@ -56,6 +56,7 @@ describe API::Issues do let!(:note) { create(:note_on_issue, author: user, project: project, noteable: issue) } let(:no_milestone_title) { URI.escape(Milestone::None.title) } + let(:any_milestone_title) { URI.escape(Milestone::Any.title) } before(:all) do project.add_reporter(user) @@ -811,6 +812,15 @@ describe API::Issues do expect(json_response.first['id']).to eq(confidential_issue.id) end + it 'returns an array of issues with any milestone' do + get api("#{base_url}/issues?milestone=#{any_milestone_title}", user) + + response_ids = json_response.map { |issue| issue['id'] } + + expect_paginated_array_response(size: 2) + expect(response_ids).to contain_exactly(closed_issue.id, issue.id) + end + it 'sorts by created_at descending by default' do get api("#{base_url}/issues", user) From 7d55c1353d6402f33a9fef734148fb776da076d3 Mon Sep 17 00:00:00 2001 From: Ronald Claveau Date: Thu, 28 Jun 2018 08:13:21 +0200 Subject: [PATCH 19/40] List public ssh keys by id or username without authentication --- .../features-unauth-access-ssh-keys.yml | 5 +++ doc/api/users.md | 2 +- lib/api/users.rb | 6 +-- spec/requests/api/users_spec.rb | 40 +++++++------------ 4 files changed, 23 insertions(+), 30 deletions(-) create mode 100644 changelogs/unreleased/features-unauth-access-ssh-keys.yml diff --git a/changelogs/unreleased/features-unauth-access-ssh-keys.yml b/changelogs/unreleased/features-unauth-access-ssh-keys.yml new file mode 100644 index 00000000000..bae2bcfaabd --- /dev/null +++ b/changelogs/unreleased/features-unauth-access-ssh-keys.yml @@ -0,0 +1,5 @@ +--- +title: Enable unauthenticated access to public SSH keys via the API +merge_request: 20118 +author: Ronald Claveau +type: changed diff --git a/doc/api/users.md b/doc/api/users.md index 762ea53edee..433f5d30449 100644 --- a/doc/api/users.md +++ b/doc/api/users.md @@ -556,7 +556,7 @@ Parameters: ## List SSH keys for user -Get a list of a specified user's SSH keys. Available only for admin +Get a list of a specified user's SSH keys. ``` GET /users/:id/keys diff --git a/lib/api/users.rb b/lib/api/users.rb index ac09ca7f7b7..e96887948b1 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -254,7 +254,7 @@ module API end # rubocop: enable CodeReuse/ActiveRecord - desc 'Get the SSH keys of a specified user. Available only for admins.' do + desc 'Get the SSH keys of a specified user.' do success Entities::SSHKey end params do @@ -263,10 +263,8 @@ module API end # rubocop: disable CodeReuse/ActiveRecord get ':id/keys' do - authenticated_as_admin! - user = User.find_by(id: params[:id]) - not_found!('User') unless user + not_found!('User') unless user && can?(current_user, :read_user, user) present paginate(user.keys), with: Entities::SSHKey end diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index b7d62df0663..09c1d016081 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -785,35 +785,25 @@ describe API::Users do end describe 'GET /user/:id/keys' do - before do - admin + it 'returns 404 for non-existing user' do + user_id = not_existing_user_id + + get api("/users/#{user_id}/keys") + + expect(response).to have_gitlab_http_status(404) + expect(json_response['message']).to eq('404 User Not Found') end - context 'when unauthenticated' do - it 'returns authentication error' do - get api("/users/#{user.id}/keys") - expect(response).to have_gitlab_http_status(401) - end - end + it 'returns array of ssh keys' do + user.keys << key + user.save - context 'when authenticated' do - it 'returns 404 for non-existing user' do - get api('/users/999999/keys', admin) - expect(response).to have_gitlab_http_status(404) - expect(json_response['message']).to eq('404 User Not Found') - end + get api("/users/#{user.id}/keys") - it 'returns array of ssh keys' do - user.keys << key - user.save - - get api("/users/#{user.id}/keys", admin) - - expect(response).to have_gitlab_http_status(200) - expect(response).to include_pagination_headers - expect(json_response).to be_an Array - expect(json_response.first['title']).to eq(key.title) - end + expect(response).to have_gitlab_http_status(200) + expect(response).to include_pagination_headers + expect(json_response).to be_an Array + expect(json_response.first['title']).to eq(key.title) end end From 295b0466b38f261ad10be72e18c0915f4f646379 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Wed, 3 Oct 2018 17:00:09 +0100 Subject: [PATCH 20/40] Removes icon key from job endpoint This was added because we thought we'd need to render the environment status but we already have this information in the build status --- app/serializers/build_details_entity.rb | 8 -------- changelogs/unreleased/50904-remove-icon-env.yml | 5 +++++ spec/controllers/projects/jobs_controller_spec.rb | 1 - spec/fixtures/api/schemas/job/deployment_status.json | 2 -- 4 files changed, 5 insertions(+), 11 deletions(-) create mode 100644 changelogs/unreleased/50904-remove-icon-env.yml diff --git a/app/serializers/build_details_entity.rb b/app/serializers/build_details_entity.rb index c85b1790e73..3d508a9a407 100644 --- a/app/serializers/build_details_entity.rb +++ b/app/serializers/build_details_entity.rb @@ -1,10 +1,6 @@ # frozen_string_literal: true class BuildDetailsEntity < JobEntity - include EnvironmentHelper - include RequestAwareEntity - include CiStatusHelper - expose :coverage, :erased_at, :duration expose :tag_list, as: :tags expose :has_trace?, as: :has_trace @@ -15,10 +11,6 @@ class BuildDetailsEntity < JobEntity expose :deployment_status, if: -> (*) { build.has_environment? } do expose :deployment_status, as: :status - expose :icon do |build| - ci_label_for_status(build.status) - end - expose :persisted_environment, as: :environment, with: EnvironmentEntity end diff --git a/changelogs/unreleased/50904-remove-icon-env.yml b/changelogs/unreleased/50904-remove-icon-env.yml new file mode 100644 index 00000000000..7915d45d6bd --- /dev/null +++ b/changelogs/unreleased/50904-remove-icon-env.yml @@ -0,0 +1,5 @@ +--- +title: Remove icon key from job endopoint +merge_request: +author: +type: other diff --git a/spec/controllers/projects/jobs_controller_spec.rb b/spec/controllers/projects/jobs_controller_spec.rb index fd11cb31a2a..30a418c0e88 100644 --- a/spec/controllers/projects/jobs_controller_spec.rb +++ b/spec/controllers/projects/jobs_controller_spec.rb @@ -225,7 +225,6 @@ describe Projects::JobsController, :clean_gitlab_redis_shared_state do expect(response).to have_gitlab_http_status(:ok) expect(json_response).to match_schema('job/job_details') expect(json_response['deployment_status']["status"]).to eq 'creating' - expect(json_response['deployment_status']["icon"]).to eq 'passed' expect(json_response['deployment_status']["environment"]).not_to be_nil end end diff --git a/spec/fixtures/api/schemas/job/deployment_status.json b/spec/fixtures/api/schemas/job/deployment_status.json index a90b8b35654..83b1899fdf3 100644 --- a/spec/fixtures/api/schemas/job/deployment_status.json +++ b/spec/fixtures/api/schemas/job/deployment_status.json @@ -2,7 +2,6 @@ "type": "object", "required": [ "status", - "icon", "environment" ], "properties": { @@ -20,7 +19,6 @@ { "type": "null" } ] }, - "icon": { "type": "string" }, "environment": { "$ref": "../environment.json" } }, "additionalProperties": false From bfdac6324c717d014b7271a83c2bf883814f93d7 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 4 Oct 2018 11:50:06 +0200 Subject: [PATCH 21/40] Copy-edit documentation for only/except changes feature --- doc/ci/yaml/README.md | 38 +++++++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 13 deletions(-) diff --git a/doc/ci/yaml/README.md b/doc/ci/yaml/README.md index 0d07b63a2dc..442a8ba643b 100644 --- a/doc/ci/yaml/README.md +++ b/doc/ci/yaml/README.md @@ -388,7 +388,7 @@ except master. > > `variables` policy introduced in 10.7 > -> `changes` policy introduced in 11.4 +> `changes` policy [introduced in 11.4][changes-policy-issue] CAUTION: **Warning:** This an _alpha_ feature, and it it subject to change at any time without @@ -455,7 +455,9 @@ Learn more about variables expressions on [a separate page][variables-expression ### `changes` Using `changes` keyword with `only` or `except` makes it possible to define if -a job should be created, based on files modified by a git push event. +a job should be created based on files modified by a git push event. + +For example: ```yaml docker build: @@ -466,20 +468,29 @@ docker build: - docker/scripts/* ``` -In the scenario above, if you are pushing multiple commits to GitLab, to an -exising branch, GitLab is going to create and trigger `docker build` job, -provided that one of the commits contains changes to the `Dockerfile` file or -changes to any of the files inside `docker/scripts/` directory. +In the scenario above, if you are pushing multiple commits to GitLab to an +existing branch, GitLab creates and triggers `docker build` job, provided that +one of the commits contains changes to either: + +- The `Dockerfile` file. +- Any of the files inside `docker/scripts/` directory. CAUTION: **Warning:** -If you are pushing a **new** branch or a new tag to GitLab, only/changes is -going to always evaluate to truth and GitLab will create a job. This feature is -not combined with merge requests yet, and because GitLab is creating pipelines +There are some caveats when using this feature with new branches and tags. See +the section below. + +#### Using `changes` with new branches and tags + +If you are pushing a **new** branch or a **new** tag to GitLab, the policy +always evaluates to truth and GitLab will create a job. This feature is not +connected with merge requests yet, and because GitLab is creating pipelines before an user can create a merge request we don't know a target branch at -this point. Without a target branch it is not possible to know what the common -ancestor is, thus we always create a job in that case. This feature works best for -stable branches like `master` because in that case GitLab uses previous commit, -that is present in a branch, to compare against a newly pushed latest SHA. +this point. + +Without a target branch, it is not possible to know what the common ancestor is, +thus we always create a job in that case. This feature works best for stable +branches like `master` because in that case GitLab uses the previous commit +that is present in a branch to compare against the latest SHA that was pushed. ## `tags` @@ -1976,3 +1987,4 @@ CI with various languages. [ce-12909]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/12909 [schedules]: ../../user/project/pipelines/schedules.md [variables-expressions]: ../variables/README.md#variables-expressions +[changes-policy-issue]: https://gitlab.com/gitlab-org/gitlab-ce/issues/19232 From eb6e0f4b6f881e2bd7cd90822b4b7ff0952a56b0 Mon Sep 17 00:00:00 2001 From: Zsolt Kovari Date: Thu, 4 Oct 2018 13:55:57 +0000 Subject: [PATCH 22/40] Use proper Configure label instead of Configuration in issue_workflow.md --- doc/development/contributing/issue_workflow.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/development/contributing/issue_workflow.md b/doc/development/contributing/issue_workflow.md index fed29d37b26..2f06677bfec 100644 --- a/doc/development/contributing/issue_workflow.md +++ b/doc/development/contributing/issue_workflow.md @@ -60,7 +60,7 @@ people. The current team labels are: -- ~Configuration +- ~Configure - ~"CI/CD" - ~Create - ~Distribution From 7cb2755617a2f29498f4523b8915bc77abcae4df Mon Sep 17 00:00:00 2001 From: Mario de la Ossa Date: Tue, 2 Oct 2018 20:56:51 -0600 Subject: [PATCH 23/40] Banzai project ref- share context more aggresively Changes `Banzai::CrossProjectReference#parent_from_ref` to return the project in the context if the project's `full_path` matches the ref we're looking for, as it makes no sense to go to the database to find a Project we already have loaded. --- app/finders/labels_finder.rb | 4 ++-- .../mao-48221-issues_show_sql_count.yml | 5 +++++ lib/banzai/cross_project_reference.rb | 1 + lib/banzai/filter/label_reference_filter.rb | 2 +- .../projects/issues_controller_spec.rb | 12 ++++++++++++ .../banzai/cross_project_reference_spec.rb | 19 ++++++++++++------- .../commit_range_reference_filter_spec.rb | 1 + 7 files changed, 34 insertions(+), 10 deletions(-) create mode 100644 changelogs/unreleased/mao-48221-issues_show_sql_count.yml diff --git a/app/finders/labels_finder.rb b/app/finders/labels_finder.rb index 08fc2968e77..0293101f3c1 100644 --- a/app/finders/labels_finder.rb +++ b/app/finders/labels_finder.rb @@ -116,7 +116,7 @@ class LabelsFinder < UnionFinder end def project? - params[:project_id].present? + params[:project].present? || params[:project_id].present? end def projects? @@ -139,7 +139,7 @@ class LabelsFinder < UnionFinder return @project if defined?(@project) if project? - @project = Project.find(params[:project_id]) + @project = params[:project] || Project.find(params[:project_id]) @project = nil unless authorized_to_read_labels?(@project) else @project = nil diff --git a/changelogs/unreleased/mao-48221-issues_show_sql_count.yml b/changelogs/unreleased/mao-48221-issues_show_sql_count.yml new file mode 100644 index 00000000000..d634d15946e --- /dev/null +++ b/changelogs/unreleased/mao-48221-issues_show_sql_count.yml @@ -0,0 +1,5 @@ +--- +title: Banzai label ref finder - minimize SQL calls by sharing context more aggresively +merge_request: 22070 +author: +type: performance diff --git a/lib/banzai/cross_project_reference.rb b/lib/banzai/cross_project_reference.rb index 3f1e95d4cc0..43f913a8859 100644 --- a/lib/banzai/cross_project_reference.rb +++ b/lib/banzai/cross_project_reference.rb @@ -13,6 +13,7 @@ module Banzai # Returns a Project, or nil if the reference can't be found def parent_from_ref(ref) return context[:project] || context[:group] unless ref + return context[:project] if context[:project]&.full_path == ref Project.find_by_full_path(ref) end diff --git a/lib/banzai/filter/label_reference_filter.rb b/lib/banzai/filter/label_reference_filter.rb index b92e9e55bb9..04ec38209c7 100644 --- a/lib/banzai/filter/label_reference_filter.rb +++ b/lib/banzai/filter/label_reference_filter.rb @@ -48,7 +48,7 @@ module Banzai include_ancestor_groups: true, only_group_labels: true } else - { project_id: parent.id, + { project: parent, include_ancestor_groups: true } end diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index 5b347b1109a..44419ed6a2a 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -637,6 +637,18 @@ describe Projects::IssuesController do project_id: project, id: id end + + it 'avoids (most) N+1s loading labels' do + label = create(:label, project: project).to_reference + labels = create_list(:label, 10, project: project).map(&:to_reference) + issue = create(:issue, project: project, description: 'Test issue') + + control_count = ActiveRecord::QueryRecorder.new { issue.update(description: [issue.description, label].join(' ')) }.count + + # Follow-up to get rid of this `2 * label.count` requirement: https://gitlab.com/gitlab-org/gitlab-ce/issues/52230 + expect { issue.update(description: [issue.description, labels].join(' ')) } + .not_to exceed_query_limit(control_count + 2 * labels.count) + end end describe 'GET #realtime_changes' do diff --git a/spec/lib/banzai/cross_project_reference_spec.rb b/spec/lib/banzai/cross_project_reference_spec.rb index aadfe7637dd..ba995e16be7 100644 --- a/spec/lib/banzai/cross_project_reference_spec.rb +++ b/spec/lib/banzai/cross_project_reference_spec.rb @@ -1,16 +1,21 @@ require 'spec_helper' describe Banzai::CrossProjectReference do - include described_class + let(:including_class) { Class.new.include(described_class).new } + + before do + allow(including_class).to receive(:context).and_return({}) + allow(including_class).to receive(:parent_from_ref).and_call_original + end describe '#parent_from_ref' do context 'when no project was referenced' do it 'returns the project from context' do project = double - allow(self).to receive(:context).and_return({ project: project }) + allow(including_class).to receive(:context).and_return({ project: project }) - expect(parent_from_ref(nil)).to eq project + expect(including_class.parent_from_ref(nil)).to eq project end end @@ -18,15 +23,15 @@ describe Banzai::CrossProjectReference do it 'returns the group from context' do group = double - allow(self).to receive(:context).and_return({ group: group }) + allow(including_class).to receive(:context).and_return({ group: group }) - expect(parent_from_ref(nil)).to eq group + expect(including_class.parent_from_ref(nil)).to eq group end end context 'when referenced project does not exist' do it 'returns nil' do - expect(parent_from_ref('invalid/reference')).to be_nil + expect(including_class.parent_from_ref('invalid/reference')).to be_nil end end @@ -37,7 +42,7 @@ describe Banzai::CrossProjectReference do expect(Project).to receive(:find_by_full_path) .with('cross/reference').and_return(project2) - expect(parent_from_ref('cross/reference')).to eq project2 + expect(including_class.parent_from_ref('cross/reference')).to eq project2 end end end diff --git a/spec/lib/banzai/filter/commit_range_reference_filter_spec.rb b/spec/lib/banzai/filter/commit_range_reference_filter_spec.rb index e1af5a15371..cbff2fdab14 100644 --- a/spec/lib/banzai/filter/commit_range_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/commit_range_reference_filter_spec.rb @@ -60,6 +60,7 @@ describe Banzai::Filter::CommitRangeReferenceFilter do exp = act = "See #{commit1.id.reverse}...#{commit2.id}" allow(project.repository).to receive(:commit).with(commit1.id.reverse) + allow(project.repository).to receive(:commit).with(commit2.id) expect(reference_filter(act).to_html).to eq exp end From 939467885851eb0f3fc5bcbaab6cb585bb85710d Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Fri, 21 Sep 2018 15:43:24 -0500 Subject: [PATCH 24/40] Add ProjectFeature check for feature flag This will allow an explicitly-disabled feature flag to override a feature being available for a project. As an extreme example, we could quickly disable issues across all projects at runtime by running `Feature.disable(:issues)`. --- app/models/project_feature.rb | 3 +++ spec/models/project_feature_spec.rb | 16 ++++++++++++++++ 2 files changed, 19 insertions(+) diff --git a/app/models/project_feature.rb b/app/models/project_feature.rb index 4a0324e8b5c..754c2461d23 100644 --- a/app/models/project_feature.rb +++ b/app/models/project_feature.rb @@ -55,6 +55,9 @@ class ProjectFeature < ActiveRecord::Base default_value_for :repository_access_level, value: ENABLED, allows_nil: false def feature_available?(feature, user) + # This feature might not be behind a feature flag at all, so default to true + return false unless ::Feature.enabled?(feature, user, default_enabled: true) + get_permission(user, access_level(feature)) end diff --git a/spec/models/project_feature_spec.rb b/spec/models/project_feature_spec.rb index cd7f77024da..2a193864e46 100644 --- a/spec/models/project_feature_spec.rb +++ b/spec/models/project_feature_spec.rb @@ -73,6 +73,22 @@ describe ProjectFeature do end end end + + context 'when feature is disabled by a feature flag' do + it 'returns false' do + stub_feature_flags(issues: false) + + expect(project.feature_available?(:issues, user)).to eq(false) + end + end + + context 'when feature is enabled by a feature flag' do + it 'returns true' do + stub_feature_flags(issues: true) + + expect(project.feature_available?(:issues, user)).to eq(true) + end + end end context 'repository related features' do From d7fa6679cbf01b934c7080b515579c233e5ddad7 Mon Sep 17 00:00:00 2001 From: Mario de la Ossa Date: Thu, 4 Oct 2018 15:45:00 -0600 Subject: [PATCH 25/40] Backport CE changes from draft_notes addition in EE --- app/models/concerns/diff_positionable_note.rb | 81 +++++++++++++++++++ app/models/diff_note.rb | 68 +--------------- 2 files changed, 82 insertions(+), 67 deletions(-) create mode 100644 app/models/concerns/diff_positionable_note.rb diff --git a/app/models/concerns/diff_positionable_note.rb b/app/models/concerns/diff_positionable_note.rb new file mode 100644 index 00000000000..b61bf29e6ad --- /dev/null +++ b/app/models/concerns/diff_positionable_note.rb @@ -0,0 +1,81 @@ +# frozen_string_literal: true +module DiffPositionableNote + extend ActiveSupport::Concern + + included do + before_validation :set_original_position, on: :create + before_validation :update_position, on: :create, if: :on_text? + + serialize :original_position, Gitlab::Diff::Position # rubocop:disable Cop/ActiveRecordSerialize + serialize :position, Gitlab::Diff::Position # rubocop:disable Cop/ActiveRecordSerialize + serialize :change_position, Gitlab::Diff::Position # rubocop:disable Cop/ActiveRecordSerialize + end + + %i(original_position position change_position).each do |meth| + define_method "#{meth}=" do |new_position| + if new_position.is_a?(String) + new_position = JSON.parse(new_position) rescue nil + end + + if new_position.is_a?(Hash) + new_position = new_position.with_indifferent_access + new_position = Gitlab::Diff::Position.new(new_position) + end + + return if new_position == read_attribute(meth) + + super(new_position) + end + end + + def on_text? + position&.position_type == "text" + end + + def on_image? + position&.position_type == "image" + end + + def supported? + for_commit? || self.noteable.has_complete_diff_refs? + end + + def active?(diff_refs = nil) + return false unless supported? + return true if for_commit? + + diff_refs ||= noteable.diff_refs + + self.position.diff_refs == diff_refs + end + + def set_original_position + return unless position + + self.original_position = self.position.dup unless self.original_position&.complete? + end + + def update_position + return unless supported? + return if for_commit? + + return if active? + return unless position + + tracer = Gitlab::Diff::PositionTracer.new( + project: self.project, + old_diff_refs: self.position.diff_refs, + new_diff_refs: self.noteable.diff_refs, + paths: self.position.paths + ) + + result = tracer.trace(self.position) + return unless result + + if result[:outdated] + self.change_position = result[:position] + else + self.position = result[:position] + end + end +end diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb index 047d353b4b5..95694377fe3 100644 --- a/app/models/diff_note.rb +++ b/app/models/diff_note.rb @@ -5,14 +5,11 @@ # A note of this type can be resolvable. class DiffNote < Note include NoteOnDiff + include DiffPositionableNote include Gitlab::Utils::StrongMemoize NOTEABLE_TYPES = %w(MergeRequest Commit).freeze - serialize :original_position, Gitlab::Diff::Position # rubocop:disable Cop/ActiveRecordSerialize - serialize :position, Gitlab::Diff::Position # rubocop:disable Cop/ActiveRecordSerialize - serialize :change_position, Gitlab::Diff::Position # rubocop:disable Cop/ActiveRecordSerialize - validates :original_position, presence: true validates :position, presence: true validates :line_code, presence: true, line_code: true, if: :on_text? @@ -21,8 +18,6 @@ class DiffNote < Note validate :verify_supported validate :diff_refs_match_commit, if: :for_commit? - before_validation :set_original_position, on: :create - before_validation :update_position, on: :create, if: :on_text? before_validation :set_line_code, if: :on_text? after_save :keep_around_commits after_commit :create_diff_file, on: :create @@ -31,31 +26,6 @@ class DiffNote < Note DiffDiscussion end - %i(original_position position change_position).each do |meth| - define_method "#{meth}=" do |new_position| - if new_position.is_a?(String) - new_position = JSON.parse(new_position) rescue nil - end - - if new_position.is_a?(Hash) - new_position = new_position.with_indifferent_access - new_position = Gitlab::Diff::Position.new(new_position) - end - - return if new_position == read_attribute(meth) - - super(new_position) - end - end - - def on_text? - position.position_type == "text" - end - - def on_image? - position.position_type == "image" - end - def create_diff_file return unless should_create_diff_file? @@ -87,15 +57,6 @@ class DiffNote < Note self.diff_file.line_code(self.diff_line) end - def active?(diff_refs = nil) - return false unless supported? - return true if for_commit? - - diff_refs ||= noteable.diff_refs - - self.position.diff_refs == diff_refs - end - def created_at_diff?(diff_refs) return false unless supported? return true if for_commit? @@ -141,37 +102,10 @@ class DiffNote < Note for_commit? || self.noteable.has_complete_diff_refs? end - def set_original_position - self.original_position = self.position.dup unless self.original_position&.complete? - end - def set_line_code self.line_code = self.position.line_code(self.project.repository) end - def update_position - return unless supported? - return if for_commit? - - return if active? - - tracer = Gitlab::Diff::PositionTracer.new( - project: self.project, - old_diff_refs: self.position.diff_refs, - new_diff_refs: self.noteable.diff_refs, - paths: self.position.paths - ) - - result = tracer.trace(self.position) - return unless result - - if result[:outdated] - self.change_position = result[:position] - else - self.position = result[:position] - end - end - def verify_supported return if supported? From 2062560ef9480d6fa07a95d9ef1a7f6fab5317f5 Mon Sep 17 00:00:00 2001 From: Thong Kuah Date: Wed, 3 Oct 2018 23:04:18 +1300 Subject: [PATCH 26/40] Use tiller directly for Auto DevOps This saves a external network call to fetch a helm plugin. The cost is a few lines of shell script --- lib/gitlab/ci/templates/Auto-DevOps.gitlab-ci.yml | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/lib/gitlab/ci/templates/Auto-DevOps.gitlab-ci.yml b/lib/gitlab/ci/templates/Auto-DevOps.gitlab-ci.yml index a96595b33a5..7ab814a0ed8 100644 --- a/lib/gitlab/ci/templates/Auto-DevOps.gitlab-ci.yml +++ b/lib/gitlab/ci/templates/Auto-DevOps.gitlab-ci.yml @@ -689,9 +689,6 @@ rollout 100%: helm version --client tiller -version - helm init --client-only - helm plugin install https://github.com/adamreese/helm-local - curl -L -o /usr/bin/kubectl "https://storage.googleapis.com/kubernetes-release/release/v${KUBERNETES_VERSION}/bin/linux/amd64/kubectl" chmod +x /usr/bin/kubectl kubectl version --client @@ -800,9 +797,9 @@ rollout 100%: function initialize_tiller() { echo "Checking Tiller..." - helm local start - helm local status export HELM_HOST=":44134" + tiller -listen ${HELM_HOST} -alsologtostderr & + echo "Tiller is listening on ${HELM_HOST}" if ! helm version --debug; then echo "Failed to init Tiller." From cab875eeb7ec6babbab8e5e36c0a0ff4db77f73c Mon Sep 17 00:00:00 2001 From: Thong Kuah Date: Fri, 5 Oct 2018 10:24:33 +1300 Subject: [PATCH 27/40] Redirect IO streams to prevent hanging https://en.wikipedia.org/wiki/Nohup#Overcoming_hanging https://gitlab.com/gitlab-org/gitlab-runner/issues/2880 --- lib/gitlab/ci/templates/Auto-DevOps.gitlab-ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gitlab/ci/templates/Auto-DevOps.gitlab-ci.yml b/lib/gitlab/ci/templates/Auto-DevOps.gitlab-ci.yml index 7ab814a0ed8..ed4ec7e6385 100644 --- a/lib/gitlab/ci/templates/Auto-DevOps.gitlab-ci.yml +++ b/lib/gitlab/ci/templates/Auto-DevOps.gitlab-ci.yml @@ -798,7 +798,7 @@ rollout 100%: echo "Checking Tiller..." export HELM_HOST=":44134" - tiller -listen ${HELM_HOST} -alsologtostderr & + tiller -listen ${HELM_HOST} -alsologtostderr > /dev/null 2>&1 & echo "Tiller is listening on ${HELM_HOST}" if ! helm version --debug; then From 7501639049a4486da60e0a00869f9f026ba83660 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= Date: Thu, 4 Oct 2018 21:40:07 -0300 Subject: [PATCH 28/40] Require spec helpers loaded by other spec helpers first --- spec/spec_helper.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index d1337325973..cd69160be10 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -34,6 +34,11 @@ Rainbow.enabled = false # Requires supporting ruby files with custom matchers and macros, etc, # in spec/support/ and its subdirectories. # Requires helpers, and shared contexts/examples first since they're used in other support files + +# Load these first since they may be required by other helpers +require Rails.root.join("spec/support/helpers/git_helpers.rb") + +# Then the rest Dir[Rails.root.join("spec/support/helpers/*.rb")].each { |f| require f } Dir[Rails.root.join("spec/support/shared_contexts/*.rb")].each { |f| require f } Dir[Rails.root.join("spec/support/shared_examples/*.rb")].each { |f| require f } From 66809b207472b9aa37e452c79c98f4d47844fb6a Mon Sep 17 00:00:00 2001 From: Evan Read Date: Fri, 5 Oct 2018 10:52:25 +1000 Subject: [PATCH 29/40] Fix links and Markdown. --- doc/administration/pages/index.md | 75 +++++++++++++++---------------- 1 file changed, 37 insertions(+), 38 deletions(-) diff --git a/doc/administration/pages/index.md b/doc/administration/pages/index.md index 3af0a5759a7..2952a98626a 100644 --- a/doc/administration/pages/index.md +++ b/doc/administration/pages/index.md @@ -92,9 +92,8 @@ where `example.io` is the domain under which GitLab Pages will be served and `192.0.2.1` is the IPv4 address of your GitLab instance and `2001::1` is the IPv6 address. If you don't have IPv6, you can omit the AAAA record. -> **Note:** -You should not use the GitLab domain to serve user pages. For more information -see the [security section](#security). +NOTE: **Note:** +You should not use the GitLab domain to serve user pages. For more information see the [security section](#security). [wiki-wildcard-dns]: https://en.wikipedia.org/wiki/Wildcard_DNS_record @@ -107,12 +106,13 @@ since that is needed in all configurations. ### Wildcard domains -> **Requirements:** -> - [Wildcard DNS setup](#dns-configuration) -> -> --- -> -> URL scheme: `http://page.example.io` +**Requirements:** + +- [Wildcard DNS setup](#dns-configuration) + +--- + +URL scheme: `http://page.example.io` This is the minimum setup that you can use Pages with. It is the base for all other setups as described below. Nginx will proxy all requests to the daemon. @@ -126,18 +126,18 @@ The Pages daemon doesn't listen to the outside world. 1. [Reconfigure GitLab][reconfigure] - Watch the [video tutorial][video-admin] for this configuration. ### Wildcard domains with TLS support -> **Requirements:** -> - [Wildcard DNS setup](#dns-configuration) -> - Wildcard TLS certificate -> -> --- -> -> URL scheme: `https://page.example.io` +**Requirements:** + +- [Wildcard DNS setup](#dns-configuration) +- Wildcard TLS certificate + +--- + +URL scheme: `https://page.example.io` Nginx will proxy all requests to the daemon. Pages daemon doesn't listen to the outside world. @@ -168,13 +168,14 @@ you have IPv6 as well as IPv4 addresses, you can use them both. ### Custom domains -> **Requirements:** -> - [Wildcard DNS setup](#dns-configuration) -> - Secondary IP -> -> --- -> -> URL scheme: `http://page.example.io` and `http://domain.com` +**Requirements:** + +- [Wildcard DNS setup](#dns-configuration) +- Secondary IP + +--- + +URL scheme: `http://page.example.io` and `http://domain.com` In that case, the Pages daemon is running, Nginx still proxies requests to the daemon but the daemon is also able to receive requests from the outside @@ -197,14 +198,15 @@ world. Custom domains are supported, but no TLS. ### Custom domains with TLS support -> **Requirements:** -> - [Wildcard DNS setup](#dns-configuration) -> - Wildcard TLS certificate -> - Secondary IP -> -> --- -> -> URL scheme: `https://page.example.io` and `https://domain.com` +**Requirements:** + +- [Wildcard DNS setup](#dns-configuration) +- Wildcard TLS certificate +- Secondary IP + +--- + +URL scheme: `https://page.example.io` and `https://domain.com` In that case, the Pages daemon is running, Nginx still proxies requests to the daemon but the daemon is also able to receive requests from the outside @@ -320,12 +322,12 @@ latest previous version. --- -**GitLab 8.17 ([documentation][8-17-docs])** +**GitLab 8.17 ([documentation](https://gitlab.com/gitlab-org/gitlab-ce/blob/8-17-stable/doc/administration/pages/index.md))** - GitLab Pages were ported to Community Edition in GitLab 8.17. - Documentation was refactored to be more modular and easy to follow. -**GitLab 8.5 ([documentation][8-5-docs])** +**GitLab 8.5 ([documentation](https://gitlab.com/gitlab-org/gitlab-ee/blob/8-5-stable-ee/doc/pages/administration.md))** - In GitLab 8.5 we introduced the [gitlab-pages][] daemon which is now the recommended way to set up GitLab Pages. @@ -334,13 +336,10 @@ latest previous version. - Custom CNAME and TLS certificates support. - Documentation was moved to one place. -**GitLab 8.3 ([documentation][8-3-docs])** +**GitLab 8.3 ([documentation](https://gitlab.com/gitlab-org/gitlab-ee/blob/8-3-stable-ee/doc/pages/administration.md))** - GitLab Pages feature was introduced. -[8-3-docs]: https://gitlab.com/gitlab-org/gitlab-ee/blob/8-3-stable-ee/doc/pages/administration.md -[8-5-docs]: https://gitlab.com/gitlab-org/gitlab-ee/blob/8-5-stable-ee/doc/pages/administration.md -[8-17-docs]: https://gitlab.com/gitlab-org/gitlab-ce/blob/8-17-stable-ce/doc/administration/pages/index.md [backup]: ../../raketasks/backup_restore.md [ce-14605]: https://gitlab.com/gitlab-org/gitlab-ce/issues/14605 [ee-80]: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/80 From 278c9e9e533bb01d2509331a4c6f384177f6d995 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Fri, 5 Oct 2018 06:44:09 +0000 Subject: [PATCH 30/40] Revert "Merge branch 'feature/git-v2-flag' into 'master'" This reverts merge request !21520 --- lib/gitlab/gitaly_client.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gitlab/gitaly_client.rb b/lib/gitlab/gitaly_client.rb index 500aabcbbb8..4ec87f6a3e7 100644 --- a/lib/gitlab/gitaly_client.rb +++ b/lib/gitlab/gitaly_client.rb @@ -220,7 +220,7 @@ module Gitlab result end - SERVER_FEATURE_FLAGS = %w[gogit_findcommit git_v2].freeze + SERVER_FEATURE_FLAGS = %w[gogit_findcommit].freeze def self.server_feature_flags SERVER_FEATURE_FLAGS.map do |f| From a545703e739fea0b7d79cc8f0d59e0a64c0eb15b Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 5 Oct 2018 10:28:42 +0200 Subject: [PATCH 31/40] Improve only: changes feature documentation Align with current technical writing style-guides. --- doc/ci/yaml/README.md | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/doc/ci/yaml/README.md b/doc/ci/yaml/README.md index 442a8ba643b..15dde36cca8 100644 --- a/doc/ci/yaml/README.md +++ b/doc/ci/yaml/README.md @@ -388,7 +388,7 @@ except master. > > `variables` policy introduced in 10.7 > -> `changes` policy [introduced in 11.4][changes-policy-issue] +> `changes` policy [introduced](https://gitlab.com/gitlab-org/gitlab-ce/issues/19232) in 11.4 CAUTION: **Warning:** This an _alpha_ feature, and it it subject to change at any time without @@ -482,7 +482,7 @@ the section below. #### Using `changes` with new branches and tags If you are pushing a **new** branch or a **new** tag to GitLab, the policy -always evaluates to truth and GitLab will create a job. This feature is not +always evaluates to true and GitLab will create a job. This feature is not connected with merge requests yet, and because GitLab is creating pipelines before an user can create a merge request we don't know a target branch at this point. @@ -1987,4 +1987,3 @@ CI with various languages. [ce-12909]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/12909 [schedules]: ../../user/project/pipelines/schedules.md [variables-expressions]: ../variables/README.md#variables-expressions -[changes-policy-issue]: https://gitlab.com/gitlab-org/gitlab-ce/issues/19232 From 637df35f0823a529d985821ab232cf3d90b76a0c Mon Sep 17 00:00:00 2001 From: Ramya Authappan Date: Fri, 5 Oct 2018 09:00:55 +0000 Subject: [PATCH 32/40] Adding qa-selectors for e2e tests --- .../issue_show/components/edit_actions.vue | 4 +-- .../components/fields/description.vue | 3 +- .../issue_show/components/fields/title.vue | 2 +- .../issue_show/components/title.vue | 3 +- .../notes/components/comment_form.vue | 15 +++++--- qa/qa.rb | 3 ++ qa/qa/page/component/issuable/common.rb | 35 +++++++++++++++++++ qa/qa/page/project/issue/show.rb | 2 ++ 8 files changed, 58 insertions(+), 9 deletions(-) create mode 100644 qa/qa/page/component/issuable/common.rb diff --git a/app/assets/javascripts/issue_show/components/edit_actions.vue b/app/assets/javascripts/issue_show/components/edit_actions.vue index bcf8686afcc..c3d39082714 100644 --- a/app/assets/javascripts/issue_show/components/edit_actions.vue +++ b/app/assets/javascripts/issue_show/components/edit_actions.vue @@ -66,7 +66,7 @@