Customers have reported that creating a git tag via the Web interface does not trigger Drone builds (see https://gitlab.zendesk.com/agent/tickets/39168). This seems to be because GitPushService and GitTagPushService are not called when creating tags or branches via the UI.
Is it intended that creating branches/tags through the interface does not trigger Service hooks? The call stack for creating tags from the UI seems to be TagsController#create -> CreateTagService#execute -> Repository#add_tag. Firing relevant events and hooks is done by the PostReceive worker, which is not involved in this sequence.
If it's not intended, would it make sense to add PostReceive.perform_async(...) in Repository#add_tag and Repository#add_branch? Actually I think the only relevant calls are GitTagPushService/GitPushService, correspondently, so I wonder if creating new workers to run these services async is the way to go.
@stanhu ah, I see it now: GitHooksService calls git's native hooks, which in turn should fire a PostReceive worker. I'll dig further into why that's not happening. Is it fair to label this with customer ?
Precisely tag.target == params[:newrev] part is important. Let's look at different cases:
lightweight tag pushed via command-line. tag.target is set to the commit ID that this tag points to. params[:newrev] is set to the same value
annotated tag pushed via command-line. tag.target is set to the tag ID (because annotated tags have their own IDs). params[:newrev] is set to the same value
tag created via our UI. It's an annotated tag. tag.target is set to the tag ID. params[:newrev] is set to the commit ID that this tag points to. The condition evaluates to false.
@adamniedzielski I still can't quite figure out, why is params[:newrev] wrong when the tag is created through the UI? GitTagPushService is called by the post-receive hook, which is invoked by git, not by the UI
@adamniedzielski ah, I think I have it. I think the problem is born here:
defadd_tag(user,tag_name,target,message=nil)oldrev=Gitlab::Git::BLANK_SHAref=Gitlab::Git::TAG_REF_PREFIX+tag_nametarget=commit(target).try(:id)returnfalseunlesstargetoptions={message: message,tagger: user_to_committer(user)}ifmessage# CHECK THIS OUT: v that target should be the sha of the tag, not of the target idGitHooksService.new.execute(user,path_to_repo,oldrev,target,ref)dorugged.tags.create(tag_name,target,options)endfind_tag(tag_name)end
Edit: foiled by monospace again.
If we fix the params for GitHooksService I think we should fix everything
we first need to create the tag (because it's how we can get the tag ID). This is a chicken and egg problem. We could change the API of GitHooksService and modify its state from the block. But this would mean that we pass different target value to pre-receive and update than to post-receive hook. This feels wrong.
@adamniedzielski maybe our GitHooksService logic is flawed? Right now we do:
%w(pre-receive update).each do |hook_name| status, message = run_hook(hook_name) unless status raise PreReceiveError, message end end yield run_hook('post-receive')
Since the block called on yield is the one that creates the tag, that means pre-receive and update are called before the object is created, and post-receive after the fact. This is not how git works, right? If I'm pushing a tag through git push --tags, the tag already exists and has an id (on my local repository). So I think we should move yield to before running any hook. The problem is still assigning new_rev to something else.
Ok, my bad, the objective of yield being there is so that if pre-receive rejects the update the repository does not get modified. Back to chicken and egg...
So the difference between workflows is:
On git push --tags a fully formed git object in a local repository but the push gets rejected in the remote repository and the tag is not created on the remote.
On Repository#add_tag there is only one repository, so when we do rugged.tags.create we are really creating the tag on the target repository. Which is why we want to know if the push would be rejected before running that, but then we end up with the current problem of pre-receive and post-receive running with different newrevs.
I think that we cannot move yield before running the hooks, because other parts of the code rely on the existing behaviour. In general the way how GitHooksServiceworks makes sense, because we first want to run these two "pre" hooks and only when they succeed perform the actual operation on the repository.
The problem in this specific case is that creating the tag and applying it to the repository happens in a single step. Ideally we would like to first create the tag outside the project repository and then integrate it to the project repository, but I cannot imagine any way to do that.
Looking around our code, it seems our approach when modifying things through the UI is:
Create a commit with the changes. This commit really exists in the target repository and has a sha, it's just not pointed at by any ref.
Run GitHooksService with that commit's sha, and inside the block update the target ref to point to the new commit.
So it would actually be easier to create lightweight tags, because we would have a real commit object with a sha and we would just need to update refs/tags/[tag_name] to point to that commit in a similar fashion than what I described above. The problem is that the UI creates only annotated tags (despite what the hints say, see https://gitlab.com/gitlab-org/gitlab-ce/issues/24554), and there doesn't seem to be a way to create the annotated tag object without writing it to refs/tags.
So the way I can think of doing this is replicating what a usual git client would do, which is to clone the repository, create the new tag in the clone, and push it to the target repository to have all hooks execute naturally. Is that too crazy?
Speaking with @smcgivern we came up with a possible solution other than cloning the repo, which is handling this special case directly in PostReceive. Given that if the tag is created through the UI then newrev is the id of the commit the tag points to we could change if tag && tag.target == params[:newrev] to if tag && [tag.target, tag.dereferenced_target.sha].include?(params[:newrev]). The first element would match regular pushes and the second would match pushes from the UI. This is a much cleaner approach than the one I proposed earlier (cloning the repo).
One very edge case about this is that it might generate conflicts if you try to force push a tag through the UI, if a lightweight tag that points to the same commit already exists. However, that's not something the UI lets you do currently (if you try to create a tag that already exists you get an error before any of the hooks things happen), so we should be fine.
@smcgivern@eReGeBe I'd consider fixing the bug in PostReceive as a last resort, because it's basically sending wrong data and then handling it as a special case.
We first create the tag to have SHA and we invoke pre-hooks after that (pretending that the tag is not part of the repository yet). If hooks fail we clean up after ourselves by removing the tag.
The thing about the PostReceive way is that we are handling wrong data, but it will only ever be our own that we're only handling our own wrong data. But I think this way is probably acceptable, too. And now whichever we pick, we have a fallback option if it doesn't work 😛