Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/add support for pr approved #827

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

LyroStedman
Copy link

@LyroStedman LyroStedman commented Mar 13, 2024

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
Copy link
Author

LyroStedman commented Mar 13, 2024

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

@lifeofguenter
Copy link
Contributor

Thanks @LyroStedman - should this not be configurable?

@KalleOlaviNiemitalo
Copy link
Contributor

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?

@LyroStedman
Copy link
Author

@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
Copy link
Contributor

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

@LyroStedman
Copy link
Author

@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
Copy link
Author

LyroStedman commented Mar 18, 2024

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 :
image

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

@KalleOlaviNiemitalo
Copy link
Contributor

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

<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
Copy link
Contributor

@LyroStedman
Copy link
Author

LyroStedman commented Mar 18, 2024

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

<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
Copy link
Contributor

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
Copy link
Author

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

@LyroStedman LyroStedman force-pushed the feature/add-support-for-pr-approved branch from 05a306a to fc5ffed Compare March 20, 2024 15:04
…x and above on manually configurated webhooks
@LyroStedman LyroStedman force-pushed the feature/add-support-for-pr-approved branch 2 times, most recently from 346eac9 to 7259dfd Compare March 20, 2024 15:19
@LyroStedman
Copy link
Author

LyroStedman commented Mar 20, 2024

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
Copy link
Contributor

KalleOlaviNiemitalo commented Mar 20, 2024

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
Copy link
Author

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
Copy link
Author

Little up for this PR.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants