Accepting a merge request will not trigger EmailsOnPush

• Zendesk ticket: https://gitlab.zendesk.com/agent/tickets/6966

Accepting a merge request will not trigger an EmailsOnPush notification since the satellites were removed. @DouweM double checked for me and confirmed that it is related to the satellite removal.

According to @DouweM we should execute the GitPushService for every change to the commit log, even if it is from within the web UI.

@dzaporozhets what do you think?

/cc @JobV

Looks like executing push services and hooks upon MR merge was broken when satellites were removed. I think we should execute the GitPushService for every change to the commit log, even if it is from within the web UI (merge MR, edit file, create file).

@patricio what GitLab version? It should work in master and in 7.13.5 and 7.14.x. It might be broken in 7.12.x and 7.13.(0..4)

I think we should execute the GitPushService for every change to the commit log, even if it is from within the web UI (merge MR, edit file, create file).

@DouweM thats how it works now in master.

@dzaporozhets thank you for the clarification. I forgot to ask version number and assumed it was pretty recent. I will ask that right now.

@dzaporozhets Are you sure? From looking at the code, I don't see that in MergeService.

@DouweM I think you are right. The customer is using 7.14, so the issue is still present.

/cc @dzaporozhets

@DouweM here satellites do merge - https://gitlab.com/gitlab-org/gitlab-ce/blob/v7.14.1/app/services/merge_requests/auto_merge_service.rb#L11

@patricio ok it should work in 7.14 so it might be a bug

@dzaporozhets Ah, right, this is 7.14 so it should definitely work. With master I don't see it in MergeService though, but I may be looking for the wrong thing.

@DouweM in master we have pre-receive and post-receive hooks executed during commits https://gitlab.com/gitlab-org/gitlab-ce/blob/master/app/models/repository.rb#L552 so it should work too. We basically emulate push with executing this hooks manually

@dzaporozhets Ah, awesome! I had no idea it worked like that, cool :)

@DouweM what should the next step be here? Since master has the correct code, it will revert to working mode once 8.0 is released. Should we look for the bug and fix it as a patch release for 7.14, or just wait for 8.0?

/cc @rspeicher

@patricio Have we had anyone else report this?

@DouweM this is the first report we've had about it.

@rspeicher do you think it is worth it to include this fix in 7.14.2?

@patricio If @DouweM can whip up a quick fix for it, sure. ETA for 7.14.2 is Monday the 7th.

@DouweM do you think is worth it to port the fix back to 7.14, considering that 8.0 is only 11 days away?

@patricio I think at this point we're better off waiting.

Status changed to closed

@DouweM I agree. I reported to the customer that 8.0 will fix his issue. We can close this one.