From 9e6cdc64741583ed0db74485892c1970ff960eab Mon Sep 17 00:00:00 2001 From: Adam Niedzielski Date: Mon, 28 Nov 2016 13:00:42 +0100 Subject: Revert "Pass correct tag target to post-receive hook when creating tag via UI" This reverts commit ae51774bc45d2e15ccc61b01a30d1b588f179f85. --- app/models/repository.rb | 13 +++---------- spec/models/repository_spec.rb | 22 ---------------------- 2 files changed, 3 insertions(+), 32 deletions(-) diff --git a/app/models/repository.rb b/app/models/repository.rb index bf136ccdb6c..5e831f84879 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -196,18 +196,11 @@ class Repository options = { message: message, tagger: user_to_committer(user) } if message - rugged.tags.create(tag_name, target, options) - tag = find_tag(tag_name) - - GitHooksService.new.execute(user, path_to_repo, oldrev, tag.target, ref) do - # we already created a tag, because we need tag SHA to pass correct - # values to hooks + GitHooksService.new.execute(user, path_to_repo, oldrev, target, ref) do + rugged.tags.create(tag_name, target, options) end - tag - rescue GitHooksService::PreReceiveError - rugged.tags.delete(tag_name) - raise + find_tag(tag_name) end def rm_branch(user, branch_name) diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 04afb8ebc98..214bf478d19 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -1303,28 +1303,6 @@ describe Repository, models: true do repository.add_tag(user, '8.5', 'master', 'foo') end - it 'does not create a tag when a pre-hook fails' do - allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([false, '']) - - expect do - repository.add_tag(user, '8.5', 'master', 'foo') - end.to raise_error(GitHooksService::PreReceiveError) - - repository.expire_tags_cache - expect(repository.find_tag('8.5')).to be_nil - end - - it 'passes tag SHA to hooks' do - spy = GitHooksService.new - allow(GitHooksService).to receive(:new).and_return(spy) - allow(spy).to receive(:execute).and_call_original - - tag = repository.add_tag(user, '8.5', 'master', 'foo') - - expect(spy).to have_received(:execute). - with(anything, anything, anything, tag.target, anything) - end - it 'returns a Gitlab::Git::Tag object' do tag = repository.add_tag(user, '8.5', 'master', 'foo') -- cgit v1.2.1 From cf58271e11f6704523be5211ecfb2d02ae1091fe Mon Sep 17 00:00:00 2001 From: Adam Niedzielski Date: Mon, 28 Nov 2016 15:04:51 +0100 Subject: Pass tag SHA to post-receive hook when tag is created via UI We only know the tag SHA after we create the tag. This means that we pass a different value to the hooks that happen before creating the tag, and a different value to the hooks that happen after creating the tag. This is not an ideal situation, but it is a trade-off we decided to make. For discussion of the alternatives please refer to https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7700#note_18982873 "pre-receive" and "update" hooks always get the SHA of the commit that the tag points to. "post-receive" gets the tag SHA if it is an annotated tag or the commit SHA if it is an lightweight tag. Currently we always create annotated tags if UI is used. --- app/models/repository.rb | 5 +++-- app/services/git_hooks_service.rb | 6 +++-- ...-developer-access-can-no-longer-create-tags.yml | 4 ++++ spec/models/repository_spec.rb | 26 ++++++++++++++++++++++ 4 files changed, 37 insertions(+), 4 deletions(-) create mode 100644 changelogs/unreleased/24813-project-members-with-developer-access-can-no-longer-create-tags.yml diff --git a/app/models/repository.rb b/app/models/repository.rb index 5e831f84879..e2e7d08abac 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -196,8 +196,9 @@ class Repository options = { message: message, tagger: user_to_committer(user) } if message - GitHooksService.new.execute(user, path_to_repo, oldrev, target, ref) do - rugged.tags.create(tag_name, target, options) + GitHooksService.new.execute(user, path_to_repo, oldrev, target, ref) do |service| + raw_tag = rugged.tags.create(tag_name, target, options) + service.newrev = raw_tag.target_id end find_tag(tag_name) diff --git a/app/services/git_hooks_service.rb b/app/services/git_hooks_service.rb index 172bd85dade..6cd3908d43a 100644 --- a/app/services/git_hooks_service.rb +++ b/app/services/git_hooks_service.rb @@ -1,6 +1,8 @@ class GitHooksService PreReceiveError = Class.new(StandardError) + attr_accessor :oldrev, :newrev, :ref + def execute(user, repo_path, oldrev, newrev, ref) @repo_path = repo_path @user = Gitlab::GlId.gl_id(user) @@ -16,7 +18,7 @@ class GitHooksService end end - yield + yield self run_hook('post-receive') end @@ -25,6 +27,6 @@ class GitHooksService def run_hook(name) hook = Gitlab::Git::Hook.new(name, @repo_path) - hook.trigger(@user, @oldrev, @newrev, @ref) + hook.trigger(@user, oldrev, newrev, ref) end end diff --git a/changelogs/unreleased/24813-project-members-with-developer-access-can-no-longer-create-tags.yml b/changelogs/unreleased/24813-project-members-with-developer-access-can-no-longer-create-tags.yml new file mode 100644 index 00000000000..9254db40742 --- /dev/null +++ b/changelogs/unreleased/24813-project-members-with-developer-access-can-no-longer-create-tags.yml @@ -0,0 +1,4 @@ +--- +title: Pass tag SHA to post-receive hook when tag is created via UI +merge_request: 7700 +author: diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 214bf478d19..b797d19161d 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -1308,6 +1308,32 @@ describe Repository, models: true do expect(tag).to be_a(Gitlab::Git::Tag) end + + it 'passes commit SHA to pre-receive and update hooks,\ + and tag SHA to post-receive hook' do + pre_receive_hook = Gitlab::Git::Hook.new('pre-receive', repository.path_to_repo) + update_hook = Gitlab::Git::Hook.new('update', repository.path_to_repo) + post_receive_hook = Gitlab::Git::Hook.new('post-receive', repository.path_to_repo) + + allow(Gitlab::Git::Hook).to receive(:new). + and_return(pre_receive_hook, update_hook, post_receive_hook) + + allow(pre_receive_hook).to receive(:trigger).and_call_original + allow(update_hook).to receive(:trigger).and_call_original + allow(post_receive_hook).to receive(:trigger).and_call_original + + tag = repository.add_tag(user, '8.5', 'master', 'foo') + + commit_sha = repository.commit('master').id + tag_sha = tag.target + + expect(pre_receive_hook).to have_received(:trigger). + with(anything, anything, commit_sha, anything) + expect(update_hook).to have_received(:trigger). + with(anything, anything, commit_sha, anything) + expect(post_receive_hook).to have_received(:trigger). + with(anything, anything, tag_sha, anything) + end end context 'with an invalid target' do -- cgit v1.2.1