Feature/add support for pr approved

[LyroStedman]

This PR is a reroll from #429
The original PR seems to be in a rather abandoned state. I'm in a need of this feature, so I chose to start a new PR from the work of @MayaPetter.

The goal of this PR is to introduce the possibility to launch a pipeline from a Bitbucket webhook event triggered by a reviewer approving a PR in Bitbucket.
This user case is described in #428

Important to note : for the cloud version, the event has only been added to the WebHookConfiguration class. It has not been added to the Cloud webhook processor (PullRequestHookProcessor.java) as the default behaviour is to consider any event as an update event.
This begs the question why this isn't the case for the server version?

Your checklist for this pull request

• Make sure you are requesting to pull a topic/feature/bugfix branch (right side) and not your master branch!
• Ensure that the pull request title represents the desired changelog entry
• Added support for the Bitbucket webhook pr:reviewer:approved event for server V 7.X and above, as well as pullrequest:approved event for cloud version.
• Link to relevant issues in GitHub : 428](add-support-to-pr:reviewer:approved-event #428)
• Link to relevant pull requests, original incomplete PR 429
• Did you provide a test-case? @MayaPetter tested the server case in her PR. I'm not currently able to install a server on my company computer. Changes for cloud version have not been tested either. Nonetheless, the changes made in this PR are an updated list from an Enum content and an additional case in a switch from an Enum content.

[LyroStedman]

@wahammed @bitwiseman @kdubrovnyi as original reviewers, I tag you in.
@jkurek1 as you commented on the original PR.

[lifeofguenter]

Thanks @LyroStedman - should this not be configurable?

[KalleOlaviNiemitalo]

I do not want to increase the load on our Jenkins agents by making them rerun the whole pipeline when PR approvals change. Would this PR have that effect? If so, is there something I could then add to when blocks of pipeline stages to skip the rebuild if the pipeline was triggered by an approval change?

• when { triggeredBy … } looks attractive, but it checks the name of the Cause subclass, and IIRC the subclass nowadays is BranchEventCause regardless of what was in the webhook request.
• when { environment … } would work if the webhook type were available as an environment variable, which AFAIK it isn't (Is there a way to retrieve payload user name at pipeline run? #413).
• when { not { changeset pattern: '*' } } would detect runs with no modified files; but if the changeset detection fails (JENKINS-68010, Changeset detection broke between 866.vdea_7dcd3008e and 874.v659a_b_70f5e69 #812), then this condition could incorrectly skip the build. Also, if the changeset feature is configured to compare the new build to something other than the previous build (such as a constant branch in JENKINS-36308, or the PR target branch in JENKINS-66893), then the resulting changeset won't be suitable for this.

[LyroStedman]

@KalleOlaviNiemitalo

I don't think it's an issue : Bitbucket's webhook triggers only on PR approval if you configured it to do so.

In the webhooks parameters, you can choose what types of events will trigger the webhook or not.

By default, I'm pretty sure PR approved trigger is disabled.

So if you don't want to overload you Jenkins instance, you should have nothing to do.

[KalleOlaviNiemitalo]

This plugin has a feature that registers a webhook in Bitbucket. Do you mean that won't register for the PR approval event?

[LyroStedman]

@lifeofguenter

Hi, I think it should indeed be, it would be more convenient.

I don't think the amount of work required is low and with little impact though, nor that it falls into the scope of this PR.

You think about externalizing these constants in a configuration file ?

[LyroStedman]

This plugin has a feature that registers a webhook in Bitbucket. Do you mean that won't register for the PR approval event?

Sorry, I'm not sure I'm understanding what you mean, so I may be answering wrong.

It would register the PR approval event solely if such an event is sent by Bitbucket. So yes, if a Bitbucket admin activates webhooks for PR approval events, Jenkins will start pipelines upon receival.
But if nobody activates these events Bitbucket's side, then there should be no effect on Jenkin's side as Jenkins will never receive PR approval events.

Here is a picture of the extent of Bitbucket webhook events :

Here is a picture of the configuration in Bitbucket for the possibility of events (sorry for the French) :

[KalleOlaviNiemitalo]

@LyroStedman I mean the "Automatic webhook configuration" feature that is described here:

bitbucket-branch-source-plugin/src/main/resources/com/cloudbees/jenkins/plugins/bitbucket/WebhookRegistrationTrait/help.html

Lines 13 to 14 in cf4056c

	<li>Automatic webhook configuration requires that Jenkins has credentials with sufficient permission to
	configure webhooks and also that Jenkins knows the URL that Bitbucket can connect to.

[KalleOlaviNiemitalo]

The events for which it automatically registers the webhook are apparently listed in this file: https://github.com/jenkinsci/bitbucket-branch-source-plugin/blob/880.vcf4056c5a_71f/src/main/java/com/cloudbees/jenkins/plugins/bitbucket/hooks/WebhookConfiguration.java

[LyroStedman]

@LyroStedman I mean the "Automatic webhook configuration" feature that is described here:

bitbucket-branch-source-plugin/src/main/resources/com/cloudbees/jenkins/plugins/bitbucket/WebhookRegistrationTrait/help.html

Lines 13 to 14 in cf4056c

	<li>Automatic webhook configuration requires that Jenkins has credentials with sufficient permission to
	configure webhooks and also that Jenkins knows the URL that Bitbucket can connect to.

Oh... I totally passed by that. I get it now. As we say, I'm quick to understand, but I need long explanations 😝
We use manual configuration, I wasn't aware of this automatic feature.

You're totally right, merging these changes would risk to overload Jenkins instances on a marginal case that is triggering a pipeline for PR approval.

I'll get more insights into that configuration, I overlooked it on relying too much on the original PR. Sorry ☹️

In the meantime, it seems we have two choices :

• giving the ability to trigger a pipeline for PR approval events for everyone, then risking to overload Jenkins instances with unwanted pipelines, forcing devs to explicitly declare the events they want to trigger a pipeline
• limit this feature to manual webhook configuration, preventing devs to declare explicit behaviour just for marginal cases, but with the downside that this feature would be impossible for automatic webhook configuration

I'm leaning toward the second solution as it seems the least intrusive one, considering the marginal use case of PR approval triggering.

This tends to get to @lifeofguenter point : putting it into a config file.

[KalleOlaviNiemitalo]

It'd be nice if a pipeline could declare that it needs webhook calls for PR approval changes; but the webhook settings in Bitbucket are per repository rather than per branch, so this would cause conflicts in a multibranch pipeline job if different branches have Jenkinsfiles that don't request the same events. I guess it would have to be implemented by requesting all events from Bitbucket but then ignoring the unwanted events at the Jenkins side. Anyway, it can be postponed to a different PR.

[LyroStedman]

I can't work on it tomorrow, I've booked some time on wednesday to look at this PR.

[LyroStedman]

Hi @KalleOlaviNiemitalo !

I put some time in today.

As per your review, I check where the class WebhookConfiguration was used, and as you said, it seems to be restricted to automatic webhook configuration.

So if I understand well, removing the events from this class helps to limit the scope of this PR to manual configuration.

I rebuilt the history so the commits won't be a mess.

Now there is one change : adding the event to NativeServerPullRequestHookProcessor class only.

As I understand, it is the processor used for manual configuration as this processor holds the events fired from my Bitbucket server, and this is the (only) place where the error I face is thrown

mars 12, 2024 2:38:46 PM INFOS com.cloudbees.jenkins.plugins.bitbucket.hooks.NativeServerPullRequestHookProcessor process
Unknown hook event SERVER_PULL_REQUEST_APPROVED received from Bitbucket Server

I'm a bit confused about the two following implementations of HookProcessor as per who they work for :

• NativeServerPullRequestHookProcessor
• PullRequestHookProcessor

The former is explicit : it applies to Native Server.
The latter should be applying to Cloud then. But we can see conditions that discriminate Cloud and Server instances. So I'm confused about the cases when a server instance calls this processor.

So why did I only changed the former ?
First, I'm sure it's the class to change as it's the one logged in my logs
Second, wherever PullRequestHookProcessor is called from, the default behaviour is to fire an update event. So it doesn't need to be changed as it already fires an update event on the HookEventType.PULL_REQUEST_APPROVED event.

I'm considering moving the NativeServerPullRequestHookProcessor to this behaviour : do an update by default instead on relying on a preconstructed list of events and throwing an error whenever an event is not on this list.

What do you think about it guys ?

[KalleOlaviNiemitalo]

I'm a bit confused about the two following implementations of HookProcessor as per who they work for :

* NativeServerPullRequestHookProcessor

* PullRequestHookProcessor

The former is explicit : it applies to Native Server. The latter should be applying to Cloud then. But we can see conditions that discriminate Cloud and Server instances. So I'm confused about the cases when a server instance calls this processor.

See #5; the plugin "Post Webhooks for Bitbucket" on Atlassian Marketplace makes Bitbucket Server (or Data Center) send webhook requests that PullRequestHookProcessor handles. AFAIK that plugin used to be open source but was later made proprietary. In contrast, NativeServerPullRequestHookProcessor relies on the native webhook support in Bitbucket Server and does not require any plugins there.

I don't know whether the "Post Webhooks for Bitbucket" plugin supports automatic webhook configuration.

[LyroStedman]

Thanks of lot for the background :)

So NativeServerPullRequestHookProcessor seems to be the right candidate for these changes.

What do you think about changing its behaviour to stop throwing errors when an event is not in the switch cases, and instead give it a default behaviour to fire an update event, as PullRequestHookProcessor already does ?

[LyroStedman]

Little up for this PR.

Is it OK, or should I modify anything (besides a full refactor to externalize the config 😄) ?