diff --git a/app/services/git/base_hooks_service.rb b/app/services/git/base_hooks_service.rb index 11beebdabafda4f4e85df2048b9a0ca0965d112c..e40ec781c64d324760e3391bef52bfc9820254bb 100644 --- a/app/services/git/base_hooks_service.rb +++ b/app/services/git/base_hooks_service.rb @@ -10,7 +10,7 @@ class BaseHooksService < ::BaseService def execute create_events - create_pipelines + create_pipeline execute_project_hooks # Not a hook, but it needs access to the list of changed commits @@ -55,9 +55,8 @@ def removing_ref? Gitlab::Git.blank_ref?(newrev) end - def create_pipelines - return unless params.fetch(:create_pipelines, true) - return if removing_ref? + def create_pipeline + return unless create_pipeline? sidekiq_safe_pipeline_params = pipeline_params.merge(push_options: push_options&.deep_stringify_keys) @@ -66,6 +65,10 @@ def create_pipelines .execute_async(:push, pipeline_options) end + def create_pipeline? + params.fetch(:create_pipelines, true) && !removing_ref? + end + def execute_project_hooks return unless params.fetch(:execute_project_hooks, true) diff --git a/app/services/git/tag_hooks_service.rb b/app/services/git/tag_hooks_service.rb index 4614cfbd1800312159263e7d7eb185158e2e1fe8..05d5e9ef4558e8b3268b531102011391c8d6e21a 100644 --- a/app/services/git/tag_hooks_service.rb +++ b/app/services/git/tag_hooks_service.rb @@ -6,6 +6,14 @@ class TagHooksService < ::Git::BaseHooksService alias_method :removing_tag?, :removing_ref? + # If a tag does not have a dereferenced_target this means it's not + # referencing a commit. This can happen when you create a tag for a tree or + # blob object. We cannot run pipelines against trees and blobs so we skip + # the creation. + def create_pipeline? + super && tag_commit.present? + end + def hook_name :tag_push_hooks end @@ -35,7 +43,7 @@ def tag def tag_commit strong_memoize(:tag_commit) do - project.commit(tag.dereferenced_target) if tag + project.commit(tag.dereferenced_target) if tag&.dereferenced_target end end end diff --git a/spec/services/git/process_ref_changes_service_spec.rb b/spec/services/git/process_ref_changes_service_spec.rb index 0a7b1a1a40fef0c87f140dc3fffd20bc42dbfefd..9b9023cae9ac053b3a773c51c630a0766d742fe5 100644 --- a/spec/services/git/process_ref_changes_service_spec.rb +++ b/spec/services/git/process_ref_changes_service_spec.rb @@ -19,14 +19,6 @@ def multiple_changes(change, count) end end - let(:changes) do - [ - { index: 0, oldrev: Gitlab::Git::SHA1_BLANK_SHA, newrev: '789012', ref: "#{ref_prefix}/create" }, - { index: 1, oldrev: '123456', newrev: '789012', ref: "#{ref_prefix}/update" }, - { index: 2, oldrev: '123456', newrev: Gitlab::Git::SHA1_BLANK_SHA, ref: "#{ref_prefix}/delete" } - ] - end - before do allow(git_changes).to receive(changes_method).and_return(changes) end @@ -145,7 +137,10 @@ def multiple_changes(change, count) subject.execute # We don't run a pipeline for a deletion - expect(Ci::Pipeline.pluck(:ref)).to contain_exactly('create', 'update') + refs = changes.filter_map do |change| + Gitlab::Git.ref_name(change[:ref]) unless change[:newrev] == Gitlab::Git::SHA1_BLANK_SHA + end + expect(Ci::Pipeline.pluck(:ref)).to match_array(refs) end it "calls Gitlab::AppJsonLogger when a Git push is made" do @@ -244,10 +239,17 @@ def multiple_changes(change, count) end context 'branch changes' do - let(:changes_method) { :branch_changes } - let(:ref_prefix) { 'refs/heads' } - - it_behaves_like 'service for processing ref changes', Git::BranchPushService + it_behaves_like 'service for processing ref changes', Git::BranchPushService do + let(:ref_prefix) { 'refs/heads' } + let(:changes_method) { :branch_changes } + let(:changes) do + [ + { index: 0, oldrev: Gitlab::Git::SHA1_BLANK_SHA, newrev: '789012', ref: "refs/heads/create" }, + { index: 1, oldrev: '123456', newrev: '789012', ref: "refs/heads/update" }, + { index: 2, oldrev: '123456', newrev: Gitlab::Git::SHA1_BLANK_SHA, ref: "refs/heads/delete" } + ] + end + end context 'when there are merge requests associated with branches' do let(:tag_changes) do @@ -258,13 +260,13 @@ def multiple_changes(change, count) let(:branch_changes) do [ - { index: 0, oldrev: Gitlab::Git::SHA1_BLANK_SHA, newrev: '789012', ref: "#{ref_prefix}/create1" }, - { index: 1, oldrev: Gitlab::Git::SHA1_BLANK_SHA, newrev: '789013', ref: "#{ref_prefix}/create2" }, - { index: 2, oldrev: Gitlab::Git::SHA1_BLANK_SHA, newrev: '789014', ref: "#{ref_prefix}/create3" }, - { index: 3, oldrev: '789015', newrev: '789016', ref: "#{ref_prefix}/changed1" }, - { index: 4, oldrev: '789017', newrev: '789018', ref: "#{ref_prefix}/changed2" }, - { index: 5, oldrev: '789019', newrev: Gitlab::Git::SHA1_BLANK_SHA, ref: "#{ref_prefix}/removed1" }, - { index: 6, oldrev: '789020', newrev: Gitlab::Git::SHA1_BLANK_SHA, ref: "#{ref_prefix}/removed2" } + { index: 0, oldrev: Gitlab::Git::SHA1_BLANK_SHA, newrev: '789012', ref: "refs/heads/create1" }, + { index: 1, oldrev: Gitlab::Git::SHA1_BLANK_SHA, newrev: '789013', ref: "refs/heads/create2" }, + { index: 2, oldrev: Gitlab::Git::SHA1_BLANK_SHA, newrev: '789014', ref: "refs/heads/create3" }, + { index: 3, oldrev: '789015', newrev: '789016', ref: "refs/heads/changed1" }, + { index: 4, oldrev: '789017', newrev: '789018', ref: "refs/heads/changed2" }, + { index: 5, oldrev: '789019', newrev: Gitlab::Git::SHA1_BLANK_SHA, ref: "refs/heads/removed1" }, + { index: 6, oldrev: '789020', newrev: Gitlab::Git::SHA1_BLANK_SHA, ref: "refs/heads/removed2" } ] end @@ -286,7 +288,7 @@ def multiple_changes(change, count) user.id, Gitlab::Git::SHA1_BLANK_SHA, '789012', - "#{ref_prefix}/create1", + "refs/heads/create1", { 'gitaly_context' => nil, 'push_options' => nil }).ordered expect(UpdateMergeRequestsWorker).to receive(:perform_async).with( @@ -294,7 +296,7 @@ def multiple_changes(change, count) user.id, Gitlab::Git::SHA1_BLANK_SHA, '789013', - "#{ref_prefix}/create2", + "refs/heads/create2", { 'gitaly_context' => nil, 'push_options' => nil }).ordered expect(UpdateMergeRequestsWorker).to receive(:perform_async).with( @@ -302,7 +304,7 @@ def multiple_changes(change, count) user.id, '789015', '789016', - "#{ref_prefix}/changed1", + "refs/heads/changed1", { 'gitaly_context' => nil, 'push_options' => nil }).ordered expect(UpdateMergeRequestsWorker).to receive(:perform_async).with( @@ -310,7 +312,7 @@ def multiple_changes(change, count) user.id, '789020', Gitlab::Git::SHA1_BLANK_SHA, - "#{ref_prefix}/removed2", + "refs/heads/removed2", { 'gitaly_context' => nil, 'push_options' => nil }).ordered subject.execute @@ -325,7 +327,7 @@ def multiple_changes(change, count) message: "Some pipelines may not have been created because ref count exceeded limit", ref_limit: Gitlab::CurrentSettings.git_push_pipeline_limit, total_ref_count: branch_changes.count + tag_changes.count, - possible_omitted_refs: ["#{ref_prefix}/changed2", "refs/tags/v10.0.0"], + possible_omitted_refs: ["refs/heads/changed2", "refs/tags/v10.0.0"], possible_omitted_ref_count: 2 ) ) @@ -337,9 +339,19 @@ def multiple_changes(change, count) end context 'tag changes' do - let(:changes_method) { :tag_changes } - let(:ref_prefix) { 'refs/tags' } - - it_behaves_like 'service for processing ref changes', Git::TagPushService + it_behaves_like 'service for processing ref changes', Git::TagPushService do + let(:changes_method) { :tag_changes } + let(:ref_prefix) { 'refs/tags' } + let(:changes) do + tags = project.repository.tags + tag_1 = tags.first + tag_2 = tags.second + [ + { index: 0, oldrev: Gitlab::Git::SHA1_BLANK_SHA, newrev: tag_1.target, ref: "refs/tags/#{tag_1.name}" }, + { index: 1, oldrev: '123456', newrev: tag_2.target, ref: "refs/tags/#{tag_2.name}" }, + { index: 2, oldrev: '123456', newrev: Gitlab::Git::SHA1_BLANK_SHA, ref: "refs/tags/delete" } + ] + end + end end end diff --git a/spec/services/git/tag_hooks_service_spec.rb b/spec/services/git/tag_hooks_service_spec.rb index a826866d9e66454ed5fdde62fa67fcad7b6ebe09..37e8a19fd2f1c4bf694b792eefa183998e725891 100644 --- a/spec/services/git/tag_hooks_service_spec.rb +++ b/spec/services/git/tag_hooks_service_spec.rb @@ -48,10 +48,38 @@ project.add_developer(user) end - it "creates a new pipeline", :sidekiq_inline do - expect { service.execute }.to change { Ci::Pipeline.count }.by(1) + context 'when the tag references a commit' do + it "creates a new pipeline", :sidekiq_inline do + expect { service.execute }.to change { Ci::Pipeline.count }.by(1) - expect(Ci::Pipeline.last).to be_push + expect(Ci::Pipeline.last).to be_push + end + end + + context 'when the tag references a blob' do + let(:tag_name) { 'blob' } + + before do + blob_id = project.repository.readme.id + project.repository.add_tag(user, tag_name, blob_id) + end + + it "does not create a pipeline", :sidekiq_inline do + expect { service.execute }.not_to change { Ci::Pipeline.count } + end + end + + context 'when the tag references a tree' do + let(:tag_name) { 'tree' } + + before do + tree_id = project.repository.tree.trees.first.id + project.repository.add_tag(user, tag_name, tree_id) + end + + it "does not create a pipeline", :sidekiq_inline do + expect { service.execute }.not_to change { Ci::Pipeline.count } + end end end