-
Notifications
You must be signed in to change notification settings - Fork 351
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
base: master
Are you sure you want to change the base?
Feature/add support for pr approved #827
Conversation
@wahammed @bitwiseman @kdubrovnyi as original reviewers, I tag you in. |
Thanks @LyroStedman - should this not be configurable? |
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
|
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. |
This plugin has a feature that registers a webhook in Bitbucket. Do you mean that won't register for the PR approval event? |
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 ? |
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. 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) : |
@LyroStedman I mean the "Automatic webhook configuration" feature that is described here: Lines 13 to 14 in cf4056c
|
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 |
Oh... I totally passed by that. I get it now. As we say, I'm quick to understand, but I need long explanations 😝 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 :
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. |
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. |
I can't work on it tomorrow, I've booked some time on wednesday to look at this PR. |
05a306a
to
fc5ffed
Compare
…x and above on manually configurated webhooks
346eac9
to
7259dfd
Compare
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
I'm a bit confused about the two following implementations of HookProcessor as per who they work for :
The former is explicit : it applies to Native Server. So why did I only changed the former ? 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 ? |
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. |
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 ? |
Little up for this PR. Is it OK, or should I modify anything (besides a full refactor to externalize the config 😄) ? |
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