Pull-Request triggers not working as expected

mfriedrich74 commented 10 years ago

Hi,

I have one project in Jenkins which should build Pull-Requests only: RefSpec: +refs/pull-requests//from:refs/remotes/origin/pr/ Branch Specifier: pr/*

The newest plugin adds the branch name and commit. I guess it is not building my project because it sends the source branch of the pull-request.

What do i need to do to grigger builds only for the one Pull-Request project? Remove Refspec and Branch from Jenkins, but how do i make it not triggering other projects with no branch specifier?

I want Pull-request builds only; or prefere / reserve hardware for pull-request builds Any ideas?

Mike

mfriedrich74 commented 10 years ago

Can the Branch options be used? Like: "Build from" = refs/pull-requests/*/from

WhiteHatTux commented 10 years ago

I have the same problem. Setting the branch option to refs/pull-requests/*/from on the configuration for the whole project would disable the other jobs for the release+develop+feature-branches.

ashnazg commented 10 years ago

If I'm reading these correctly, you're saying that a build trigger notification from your PR is also building other Jenkins jobs that point to the same repository... ?

In my setup, I generally have three jobs per repo:

the first PR build, to build against the PR's branch itself ("from")
the second PR build, to build against what would be the result of the PR merging to develop ("merge")
the general build of the 'develop' branch

When a build trigger from any branch is sent from this repo, it looks to me (according to the Jenkins log) that all three of these build jobs will be triggered to poll, but the only jobs that begin building will be ones where a change was seen on the job's "Branch Specifier" value. I have no issue with a change to a PR causing a build to 'develop', or vice versa.

Jenkins Job Config:

PR FROM
- Refspec +refs/pull-requests/:refs/remotes/origin/pull-requests/
- Branch Specifier origin/pull-requests/*/from
PR MERGE
- Refspec +refs/pull-requests/:refs/remotes/origin/pull-requests/
- Branch Specifier origin/pull-requests/*/merge
DEVELOP
- Refspec +refs/heads/develop:refs/remotes/origin/develop
- Branch Specifier origin/develop

I also have "Poll SCM" checked on all three, but with no schedule populated.

In the Stash Webhook config, the default of "Build All" is set. Thus, it is the per-job config that is controlling which builds actually run when a build trigger is received.

mfriedrich74 commented 10 years ago

I disabled all projects in Jenkins, except the one for building the pull-requests which is similar to your "PR FROM" config. So, i cant tell if it would trigger other projects too.

The plugin version 2.4.2 does not send the branch and commit on Pull-Request create/update. It triggers a build on my (pull-request) project. If it triggers too often I don't know.

The plugin version 2.5 adds the branch and commit. It does not trigger. I was assuming it is because it sends the source branch wich does not macth my specified refspec/branchspec in the pull-request project.

Manually starting the pull-request project in Jenkins works.

pull-request project:

RefSpec: "+refs/pull-requests/*/from:refs/remotes/origin/pr/*"
Branch Specifier: pr/*

ashnazg commented 10 years ago

Your version of Stash is putting its pull requests in origin/pr/* rather than in origin/pull-requests/*?

mfriedrich74 commented 10 years ago

Thats not how refspecs work.

The left side of the colon is whats fetched from the server. The right side is my definition - what I choose where to put it locally. I just chose a shorter branch name for displaying purposes in Jenkins.

ashnazg commented 10 years ago

Ah, right... sometimes one needs coffee, other times one needs people with a clue :-D

ohamada commented 10 years ago

We are also experiencing problems with pull request triggers. Our configuration is similar to what ashnazg posted above - problem is that the trigger from stash contains correct repostiory address, correct sha1 of the commit in pull request, but the branch is always master. This is wrong behavior in my opinion - the branch should point directly to the pull request and not to the target nor originating branch.

WhiteHatTux commented 10 years ago

I absolutely agree with @ohamada

mikesir87 commented 10 years ago

Yes. It's a known bug right now. Currently, during a notification that is generated by a pull request change, the branch information does not contain the branch details for the pull request. I'm working on a fix, but it's probably not going to be until next week before a release is made.

WhiteHatTux commented 10 years ago

Great to know, that you are working on a fix. Thank you. Great plugin, we use it very extensively

christiangalsterer commented 10 years ago

It would be really appreciated if the PR MERGE use case described by @ashnazg would be supported. We also use the plugin very extensively but the most missed feature is that you cannot trigger auto-merge commits done by Stash through this. The problem I see is, that if even if you include the target branch somehow in the notification you may still not trigger the correct build for the following reasons.

Different branches, i.e. releases may require different Jenkins builds due to different configurations, e.g. Maven version. So one need to somehow dispatch the notification to the correct build. If one would include the target branch to the notification and assuming Jenkins GIT plugin handles this situation correctly, that the notifications contains the PR merge branch and the target branch along with the SHA-1 of the auto merge commit you would then trigger then two build in Jenkins, one for auto-merge/target branch and one for the PR.

What do you think?

mikesir87 commented 10 years ago

What I'm thinking is that when a commit/push/event of some sort has occurred on a branch that is involved with a pull request, multiple branches are included in the notification to Jenkins. The URL the plugin uses allows for multiple branches to be indicated. So, one for the actual branch, as well as each pull request (if it can be determined anyways).

stagrlee commented 10 years ago

I'd be interested in not just branch pull requests, but pull requests from Forks.

mikesir87 commented 10 years ago

Good point @stagrlee. I'll see what I can figure out there.

WhiteHatTux commented 10 years ago

The option with multiple branches in the URL seems perfect for me, so Jenkins will do the filtering on what it actually wants to build.

christiangalsterer commented 10 years ago

I did a little bit of research in the Jenkins Git plugin and it looks like that when multiple branches are specified the PR MERGE workflow should work, i.e. when the target branch is included.

What is important is the order in which the branches are added to the repository. As the Jenkins Git plugin first iterates over the branches of the git remote locations, the Stash plugin should include the target branch of the auto merge of a pull request as the first item to the branches list and the SHA-1 of the auto merge for the pull request Actually the target branch is enough and the pr*/merge is not needed to make this working, but if if both branches are added the the target branch must come first. This enables to have different Jenkins jobs for each "target" branch/version.

See also code snippet (hudson.plugins.git.GitStatus.JenkinsAbstractProjectListener) from Jenkins Git plugin.

 else {
                                OUT: for (BranchSpec branchSpec : git.getBranches()) {
                                    for (String branch : branches) {
                                        if (branchSpec.matches(repository.getName() + "/" + branch)) {
                                            branchFound = true;
                                            break OUT;
                                        }
                                    }
                                }
                            }

mikesir87 commented 10 years ago

So, looks like there are some complications around the pull request notifications. I had run into this back when I first put in the pull request notification, but had forgotten about them. If you take a look at this link (https://answers.atlassian.com/questions/239988/change-pull-request-refs-after-commit-instead-of-after-approval-or-workaround), it explains that the refspecs for the pull requests are updated synchronously, when a developer actually views the pull request.

So, if the plugin sends a notification to Jenkins, the refspec for the pull request Jenkins uses and is building with is going to be for an older merge commit. I'm not quite 100% sure how I'd get around this to let you all still build on each pull request update. And obviously, building just the from branch isn't sufficient here.

For those of you that have been building on pull requests, what have you seen?

stagrlee commented 10 years ago

I did some playing around on the bitbucket side and what seems to work is using a Jenkins plugin that polls bitbucket instead of bitbucket triggering jenkins. Kind of backwards but after a few hours of experimentation, seems to work nicely.

https://wiki.jenkins-ci.org/display/JENKINS/Bitbucket+pullrequest+builder+plugin

mikesir87 commented 10 years ago

Just released a new update. Please let me know if you're experiencing the issues you were facing.

christiangalsterer commented 10 years ago

If I understood the latest discussions correctly, then the change in 2.6.0 is that it contains a switch to exclude the SHA-1 in the Jenkins notification. Although this solves the problem with "detached branch" I think this will not change the situation with the "PR MERGE" use case. Is this also your view or do I miss something?

qianalan commented 10 years ago

Mike, could you please clarify this in the release notes: "Reverted pull-request notifications back to where it was before"? I am not sure how far back the behavior got reverted back. It would be really helpful if you could provide an updated clarification on how things are supposed to work at this point. Is the plugin sending pull request refs now, or branch/commit? Does Jenkins need to poll still (again), or not anymore? etc.

qianalan commented 10 years ago

I inserted some debugging statements and ran some tests. I don't see how this would work if we want to only build upon pull request changes. The "Build from" option in the plugin's Advanced Configuration compares the setting with the actual branch name pushed, so one would not be able to specify something like 'pull-requests/*' there to restrict the triggering to only when PRs are changed. On Jenkins side, the job only gets triggered if the branch specifier matches the incoming call. So specifying pull requests there also does not work. I'm curious how anyone is able to get pull request jobs working in a non-polling scenario. Let me know if I missed something?

mikesir87 commented 10 years ago

@qianalan - sorry for the slow response. The notification that occurs when a pull request is rescoped/created was accidentally removed with the 2.5.0 release. I've added it back in.

However, I've recently remembered that even though the notification occurs, the pull request branch may or may not actually be available to do a build. According to the link below, it appears that the pull request branch does the merge lazily, when the pull request page is actually viewed. If that's the case, even when a notification is sent, the branch hasn't been updated with the merge, which means building would be impossible. Still not quite sure how I might be able to get around that yet though...

https://answers.atlassian.com/questions/239988/change-pull-request-refs-after-commit-instead-of-after-approval-or-workaround

qianalan commented 10 years ago

Thanks for the reploy Mike. I guess I have not been able to find the commit where you did the revert. Which is it? Does this mean the pull request ref should be part of the url for the notifyCommit call?

I'm just trying to build the 'from' branch of the pull request, which should not be affected by the lazy merge of Stash.

On Fri, Oct 24, 2014 at 6:51 PM, Michael Irwin notifications@github.com wrote:

@qianalan https://github.com/qianalan - sorry for the slow response. The notification that occurs when a pull request is rescoped/created was accidentally removed with the 2.5.0 release. I've added it back in.

However, I've recently remembered that even though the notification occurs, the pull request branch may or may not actually be available to do a build. According to the link below, it appears that the pull request branch does the merge lazily, when the pull request page is actually viewed. If that's the case, even when a notification is sent, the branch hasn't been updated with the merge, which means building would be impossible. Still not quite sure how I might be able to get around that yet though...

— Reply to this email directly or view it on GitHub https://github.com/Nerdwin15/stash-jenkins-postreceive-webhook/issues/83#issuecomment-60468105 .

stormbeta commented 9 years ago

This is causing problems for us too.

It's happening because the plugin now sends the branch parameter to Jenkins, and Jenkins won't trigger polling unless there's a job set with the same string as what's passed in the branch parameter.

This means any branches specified as something other than the plain branch name will be ignored for the purposes of polling, which includes pull request builds.

Refspec: +refs/pull-requests/*/from:refs/remotes/origin/pr/* Branch: origin/pr/**

If I manually curl the Jenkins notify url without the branch parameter, this works as expected. If the Stash plugin is set to notify on all branches, then the branch parameter should be omitted since the Jenkins Git plugin is already smart enough to only trigger relevant jobs.

This problem also surfaces with more explicit branch specs; e.g. the plugin sends branches=master but the Jenkins job is set as refs/heads/master, and won't poll because the strings don't match.

For now, we're working around it by configuring nginx to drop the extra parameters (all our repositories in stash are set to always notify, and Jenkins figures out which things to build).

christiangalsterer commented 9 years ago

@mikesir87: There is a new plugin available https://github.com/palantir/stashbot available which supports to trigger a Jenkins on various cases, including support for topic branches and pull requests. The main problem with that plugin is that it creates new jobs which is hardcoded in the plugin and therefore not very suitable for us. I would rather see proper support for pull request and topic branches in this plugin as I think it it has the cleaner approach to just notify Jenkins via the Git plugin.

Therefore I would like to see support for the following two use cases: Topic Branches (Features, Bugfix) merge with later target branch (master, release)

Should send a notification when a topic branch is created and updated
Notification should include SHA-1 of the topic branch commit and the branch where the topic branch is branched from

Pull request:

Should send a notification when pull request is created or updated
Should send a notification when pull request target branch is updated
Notification should include SHA-1 of the pull request commit and the branch of the pull request target

For both use cases the idea is that the actual selection of the Jenkins job is based on the branch specified in the notification (target for PR, branch head for topic branch use cases) for the following reasons:

Auto merge for pull requests are done asynchronously by Stash.
Different Jenkins jobs are (typically) used with different configurations, e.g. different configuration per release branch (which for a Pull request is the target branch and for a topic branch it is the where was branched from)

Therefore I wanted to check with you what are the future plans to support these use cases also referring back to the use cases mentioned by @ashnazg back in September. Do you think this can be supported in the near future? I would volunteer to implement these features, but it would be good to know in advance your view on this and if you would then accept respective pull request. Thanks for your feedback in advance.

mikesir87 commented 9 years ago

@christiangalsterer I'm all up for new features. I really don't have much of a roadmap in mind, as this plugin was originally started as something simple for myself that I decided to open source and make it available for everyone else. Since then, the additions have been driven by the community, mostly in ideas though.

With that in mind, if you'd like to dive in and contribute some code, I'm all game! I'm pretty booked up with other projects, so am not able to contribute a ton myself to new features. However, with the holidays coming up, I may get a little more time to play around.

christiangalsterer commented 9 years ago

Thanks for your reply. I will look into this during the upcoming holidays and see what is possible.

christiangalsterer commented 9 years ago

I found some time to implement support for notifications for PR create and re-scope events. You can find a first implementation at https://github.com/christiangalsterer/stash-jenkins-postreceive-webhook/tree/pr-support During the implementation I found some problems where it would be great to get some feedback in order to see where the go next. This is also the reason why I didn't create a PR request yet.

Before jumping into the details here a quick overview on the use cases I was looking into so far.

Use Cases	SHA1 in Notification	Branch In Notification	Comments
PR created	SHA1 of the last source branch commit	PR target branch	Implemented and integration testing with life Stash/Jenkins working.
PR reopened	SHA1 of the last source branch commit	PR target branch	Implemented and integration testing with life Stash/Jenkins working.
PR source branch updated	SHA1 of the last source branch commit	PR target branch	Implemented and integration testing with life Stash/Jenkins working.
PR target branch updated	SHA1 of the last target branch commit	PR target branch	Implemented and integration testing with life Stash/Jenkins working.

The motivation around using the PR target branch is to enable different Jenkins jobs for each target branch, where some settings might be different, e.g. Java version, etc. and do to a pre-PR-approve merge build where the PR source is merged into the PR target branch. This is the same idea one finds also in other Jenkins plugins for GitHub and GitLab (more on this a little bit later).

In order to get this working in parallel one need to build the PR use cases and the integration build use cases with the same Jenkins jobs. This is not a limitation of this plugin but of the provided support of the Jenkins Git plugin for build notifications. Without going into implementation details the current design of the Jenkins Git plugin doesn't really support for PR (as you can specify only a single branch but no source and target branch) and therefore one faces some limitations and problems.

All use cases for themselves are working fine. The problems start when you want to combine this with the following use case: Trigger a build for each commit into a topic branch (feature/bugfix) for which later a PR is potentially created and use the build configuration of the "suspected" target branch but do not do a merge yet. Let's call this "Topic Branch Build" for the following discussion. Please also not that this is not implemented yet but seems to be a desired feature based on some earlier comments on this and other issues. In a "Topic Branch Build" the branch name in the notification would be the branch name where the topic branch was branched off, typically master/release so that the Jenkins Git Plugin knows the "target" branch.

Here a quick overview what happens when you try to combine "PR Builds" and "Topic Branch Build" notifications.

If the change is on the target branch of a pull request then two builds are triggered -- a notification for the PR rescope. -- a notification for the target branch
If the change is on the topic branch which becomes later the source branch of a PR. -- a notification for the source branch is send with the source branch as the branch name -- a notification for the source branch is send with the name of the branch was branched off.

Combination 1: Is not a problem and can be handled by Jenkins in a proper way. (Jenkins would still try to merge the branches, but as for the normal notification the source and target branch are the same Jenkins/Git is smart enough to detect this and just skip it).

Combination 2: Here Jenkins cannot differentiate anymore between a change in a PR source branch and a change in topic branch as in noth cases the "target" branch would be send. For the first notification a merge should happen but not for the second notification.

Some ideas how to deal with this: A) Be aware of the limitation and know that "PR use case builds" and "Topic Branch Build" cannot be combined B) Don't use the Jenkins Git Plugin notification support for "PR use case builds" but implement a Jenkins Plugin which implements its own ScmTrigger and use this new trigger in this Stash plugin to notify Jenkins on PR changes. This would be the same approach as in https://wiki.jenkins-ci.org/display/JENKINS/GitHub+pull+request+builder+plugin and https://wiki.jenkins-ci.org/display/JENKINS/Gitlab+Merge+Request+Builder+Plugin B1) Use a propritary protocol B2) Re-use the Stash Rest API payload as the notification payload protocol B3) Wait for Atlassian to define an official protocol for web hooks, similar to what GitHub https://developer.github.com/v3/activity/events/types/#pullrequestevent and GitLab (https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/web_hooks/web_hooks.md) provides C) Don't support PR notifications

Thanks for taking the time to read this long comment. Would be awesome to get your view points and opinions of the implementation in general but especially on the limitations described beforehand in combining several use cases.

If you have questions and want to have more details please let me know.

mikesir87 commented 9 years ago

I haven't gone completely through all the notes yet (there's a lot to ingest), but one thing we can think about is "are there instances in which we would rather use more than a single notification?" There's no rule that states we can only produce one notification. So, if trying to determine when to use source vs target branches, we could always just do both and let Jenkins config determine what builds should be triggered.

More comments to come here soon once I fully read over and peek at your implementation.

christiangalsterer commented 9 years ago

It is indeed a lot to ingest and digest. Maybe two notifications solve some problems. I will try to make some tests on the weekend.

For reference/inspiration/food for thought, Atlassian came out with a first "official" web hook protocol (https://confluence.atlassian.com/display/STASH/POST%20service%20webhook%20for%20Stash). I covers currently only changes but no PR (yet) and has some limitations, e.g. it doesn't contain enough repo informations for multi Stash instance environments.It is also not fully official yet as it is a Atlassian Lab release with no support.

hgomez commented 9 years ago

Hi all

I track this one very closely since I'm battling with Jenkins Setup and Stash to have PR builts, using Jenkins LTS 1.580.2 and Stash 3.5.1.

I'm a bit confused by RefSpec and Branch for example

Refspec: +refs/pull-requests/*/from:refs/remotes/origin/pr/*
Branch: origin/pr/**

How did you get them "visible" from git client ? I didn't see such refs or branches from git (ssh) neither in local Stash repositories.

Should git use http instead ?

Advices / samples more than welcomed

hgomez commented 9 years ago

I finally find my mistake, I should use /merge to local branch (Advanced): master.

I've 2 jobs

one attached to repo itself

Name : origin
RefSpec : +refs/heads/master:refs/remotes/origin/master
Branch : master

a second one to PR's on repo

Name : origin
RefSpec : +refs/pull-requests/*:refs/remotes/origin/pull-requests/*
Branch : origin/pull-requests/*/merge

When I create PR, Trigger is not sent automatically. If I click on PR tab Trigger Build, Jenkins notify url ends with branches=master On hook, if I click to Test Trigger, Jenkins notify url don't have branches=master and build is triggered.

Did I miss something ?

Thanks all

stormbeta commented 9 years ago

@hgomez the notify url ending with branches=master is indeed the problem.

What's happening is that the Git plugin in Jenkins does a literal match on the branch name if the branches parameter is present in the notify url. This breaks things like pull requests builds, because the branch doesn't literally match.

Clicking the Test Trigger button doesn't send the branch name parameter, so it works, but the actual triggers do send it. Unfortunately, there's no way right now to disable this in the plugin, so your options are to either find a different trigger plugin for stash, downgrade the stash plugin to a version that doesn't do this, or find a way to drop the branch parameter before it reaches jenkins (e.g. nginx/apache if you're using them in front of Jenkins).

Side note: using merge may not work as expected, as it's created asynchronously:

When you view a pull request in Stash, Stash shows the diff between the 'from' branch of the pull request and the merge of the 'from' branch and the 'to' branch. This merge needs to be recalculated every time someone pushes to either the 'from' or 'to' branch. For performance reasons, these calculations are done asynchronously on background threads, which means that the refs under /refs/pull-requests are updated some time after push has completed. Therefore, when the webhook sends a notification to Jenkins, the refs under /refs/pull-requests will not have been updated yet.

It is safe to use the from spec instead, though you'll have to manually merge it to master if you want to test the merged code.

goodbomb commented 9 years ago

Hi, I'm also having trouble getting PRs to trigger our Jenkins builds. My setup works fine when I run the build manually from Jenkins, but PRs and subsequent commits to those PRs don't trigger the Jenkins build. I've tried multiple approaches with no success. Any insight would be great.

Here are my current approaches:

RefSpec : +refs/pull-requests/*:refs/remotes/origin/pull-requests/*
Branch : **

also tried:

RefSpec : +refs/pull-requests/*:refs/remotes/origin/pull-requests/*
Branch : origin/pull-requests/*/from

Based on the comment by @stormbeta, do we need to write a script that loops through the refs to detect changes in order to trigger the builds?

tuukkamustonen commented 9 years ago

As a workaround, we are using SCM polling in the PR test jobs. Normally, when a PR is created, it takes some time before someone else gets to review it, so the delay is not that bad.

Still, was @christiangalsterer changes abanoned or just pending? Did @mikesir87 have opportunity to review them?

ashnazg commented 9 years ago

Rereading this thread because I'm now having an issue with my PR jobs (which up to now were working fine).

One thing I didn't highlight before is the way I handle my PR MERGE jobs. They are not triggered by notifications or polling... they are started as post-build steps by my PR FROM jobs, only when those jobs succeed. My rationale here was that if the PR FROM build is no good, the PR MERGE build is of no value to me regardless of its own success/failure. It seems very likely that the time delay between the PR creation/change and the eventual launch of my PR MERGE build has meant that any delays in Stash's lazy merging have been rendered moot. I wanted to highlight this technique in case it helps everyone else.

acejam commented 8 years ago

Any updates to this?

Helmsdown commented 8 years ago

+1

kishorebabukavuru commented 5 years ago

Hi I hope I'm on right page, Facing an issue with PR .. On raising PR job got build successful and then again without any intervention job gets triggered with the same PR id. In the job I have identified an issue with push to compare branch.. How to stop this unwanted triggering

mohamicorp / stash-jenkins-postreceive-webhook

Pull-Request triggers not working as expected #83