stash-jenkins-postreceive-webhook icon indicating copy to clipboard operation
stash-jenkins-postreceive-webhook copied to clipboard

Pull-Request triggers not working as expected

Open mfriedrich74 opened this issue 10 years ago • 41 comments

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 avatar Sep 08 '14 17:09 mfriedrich74

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

mfriedrich74 avatar Sep 08 '14 17:09 mfriedrich74

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.

WhiteHatTux avatar Sep 09 '14 06:09 WhiteHatTux

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.

ashnazg avatar Sep 09 '14 13:09 ashnazg

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/*

mfriedrich74 avatar Sep 09 '14 19:09 mfriedrich74

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

ashnazg avatar Sep 09 '14 19:09 ashnazg

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.

mfriedrich74 avatar Sep 10 '14 11:09 mfriedrich74

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

ashnazg avatar Sep 10 '14 13:09 ashnazg

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.

ohamada avatar Sep 17 '14 14:09 ohamada

I absolutely agree with @ohamada

WhiteHatTux avatar Sep 18 '14 15:09 WhiteHatTux

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.

mikesir87 avatar Sep 18 '14 16:09 mikesir87

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

WhiteHatTux avatar Sep 18 '14 16:09 WhiteHatTux

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?

christiangalsterer avatar Sep 18 '14 16:09 christiangalsterer

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).

mikesir87 avatar Sep 18 '14 18:09 mikesir87

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

stagrlee avatar Sep 18 '14 18:09 stagrlee

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

mikesir87 avatar Sep 18 '14 18:09 mikesir87

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.

WhiteHatTux avatar Sep 18 '14 18:09 WhiteHatTux

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;
                                        }
                                    }
                                }
                            }

christiangalsterer avatar Sep 20 '14 11:09 christiangalsterer

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?

mikesir87 avatar Sep 28 '14 01:09 mikesir87

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

stagrlee avatar Sep 28 '14 18:09 stagrlee

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

mikesir87 avatar Oct 16 '14 19:10 mikesir87

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?

christiangalsterer avatar Oct 19 '14 05:10 christiangalsterer

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 avatar Oct 22 '14 20:10 qianalan

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?

qianalan avatar Oct 24 '14 23:10 qianalan

@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

mikesir87 avatar Oct 25 '14 01:10 mikesir87

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 [email protected] 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 .

qianalan avatar Oct 25 '14 05:10 qianalan

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).

stormbeta avatar Dec 03 '14 17:12 stormbeta

@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.

christiangalsterer avatar Dec 14 '14 17:12 christiangalsterer

@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.

mikesir87 avatar Dec 16 '14 18:12 mikesir87

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

christiangalsterer avatar Dec 19 '14 15:12 christiangalsterer

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.

  1. 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
  2. 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.

christiangalsterer avatar Dec 26 '14 13:12 christiangalsterer