reconquest / atlassian-external-hooks

External Hooks plugin for Atlassian Bitbucket
https://external-hooks.reconquest.io
Other
45 stars 37 forks source link

Rebase via PR menu doesn't trigger hook #104

Open DrVanScott opened 4 years ago

DrVanScott commented 4 years ago

In the "three dot" menu of a PR in Bitbucket, it is possible to start a rebase. Hooks are not executed for the new commits which will be pushed

Bitbucket: 6.5.1 Plugin: 7.2.0-1

seletskiy commented 4 years ago

@DrVanScott

Hello. Thanks for the report. We'll check it in our test setup and get back shortly.

seletskiy commented 4 years ago

According to our research so far it's not yet possible to call any hook scripts on rebase event due internal logic of the Bitbucket Server.

I'm passing this ticket next to Bryan who is working closely on the Bitbucket Server.

@bturner: please correct if I'm wrong on this, but it seems that rebase internally triggers GitPullRequestRebasedEvent which can't be used to register our hook scripts to be triggered by it because it doesn't implement RepositoryHookTrigger interface.

bturner commented 4 years ago

@seletskiy,

Sure, it's possible. I think you're looking for GitRepositoryHookTrigger.REBASE. The link is to the 6.8.1 Javadocs, but the enum and the REBASE constant have existed since Bitbucket Server 5.5.0, so you can likely support hooks on rebases even prior to the HookScript support we added in 6.2.0.

Hope this helps, Bryan Turner Atlassian Bitbucket

kovetskiy commented 4 years ago

@bturner

There is a nuance that this event is triggered every time when a user opens a pull request page because UI triggers following API to get canRebase and canWrite variables from backend: /rest/git/latest/projects/{project_key}/repos/{repo_slug}/pull-requests/{pull_request_id}/rebase

As far as I understand DrVanScott, users want to run HookScripts before the rebase operation and right after it while GitRepositoryHookTrigger.REBASE events are triggered for git-rebase checks.

The GitPullRequestRebasedEvent event (which doesn't work for us because it doesn't implement RepositoryHookTrigger) looks more interesting because it goes right from DefaultGitPullRequestService.RebaseOperation immediately after executing git rebase command. (I am looking at BB 6.8.0 sources /com/atlassian/stash/internal/scm/git/pull/DefaultGitPullRequestService.java)

Also, I have created two HookScripts for both types HookScriptType.PRE and HookScriptType.POST and for some reason only HookScriptType.PRE was invoked (the PRE one didn't exit with non-zero code, I double-checked it).

Thanks for your help. We appreciate it.

bturner commented 4 years ago

@kovetskiy,

Triggering the hook in canRebase is intentional, to allow hook implementors to block rebasing before we try to perform it. When possible, it's much better if hooks can reject the rebase before the system actually runs git rebase, to avoid doing throwaway work. If you don't want to trigger hook scripts from your app in that case, the isDryRun() flag can be used to tell the difference between canRebase and rebase. If you do trigger hook scripts, the scripts themselves can use BB_IS_DRY_RUN to tell the difference (in 6.2+, at least). This same dry-run/non-dry-run approach is taken with StandardRepositoryHookTrigger.PULL_REQUEST_MERGE; hooks are invoked every time canMerge is checked, which is done every time the pull request page is opened. How does your app handle that?

Based on that, @DrVanScott would look for BB_IS_DRY_RUN=false to tell that it's the "real" rebase operation, with hooks being invoked after the rebase is done but before any refs are updated. Since the opening description references "for the new commits", it seems like HookScriptType.PRE handling may be all he really cares about. (He can correct me if I'm wrong, of course.) If his hook script rejects the commits, the rebase results will be discarded without updating any refs--just like any other PRE hook.

The lack of a HookScriptType.POST for rebase is an oversight. There should still be a fallback "generic" event that triggers hooks after the rebase, using the catch-all UNKNOWN trigger. Unfortunately, that UNKNOWN trigger can also be used for other places where the system isn't mapping an event to a specific RepositoryHookTrigger, so pretending like UNKNOWN means REBASE isn't reliable. I'll create a BSERV issue for tracking getting a "real" POST trigger on that.

Best regards, Bryan Turner Atlassian Bitbucket