Fix pull request triggers from forks #172
Conversation
Signed-off-by: Marcelo Salazar <chelosalazar@gmail.com>
…omes from forks and should be merged in a not default branch.
|
I like your pull requests. Can't you add parameters to that url as it is not supported by the git plugin? |
|
I don't know if you can add additional parameters previous git 2.4 version, this is the reason why I added a omit parameters. |
|
Hi Marcelo, I'm very excited to see this PR. Does this mean you have this working in stash with a single Jenkins job for forked repositories? We typically merge from a fork for each developer and also in a separate feature / bug fix branch back to master or the central release branch. I commented here |
|
This PR is relevant to my interests. Is there any barrier to merging this? |
| * @return The notification result. | ||
| */ | ||
| public @Nullable NotificationResult notify(@Nonnull Repository repo, //CHECKSTYLE:annot | ||
| String jenkinsBase, boolean ignoreCerts, String cloneType, String cloneUrl, | ||
| String strRef, String strSha1, boolean omitHashCode, boolean omitBranchName) { | ||
| String strRef, String strSha1, String targetBranch, boolean omitHashCode, | ||
| boolean omitBranchName, boolean omitTargetBranch) { |
There was a problem hiding this comment.
I feel the omit bools aren't clear enough. I think we should prefer names like shouldOmitTargetBranch. To me, it makes statements like if (!shouldOmitTargetBranch) more clear.
There was a problem hiding this comment.
yes, I wrote in that manner to maintain consistency with the others vars.
| @@ -62,16 +62,16 @@ protected void handleEvent(PullRequestEvent event) { | |||
| return; | |||
| } | |||
|
|
|||
| String strRef = event.getPullRequest().getFromRef().toString() | |||
| .replaceFirst(".*refs/heads/", ""); | |||
| String strRef = "pr/" + Long.toString(event.getPullRequest().getId()) + "/from"; | |||
There was a problem hiding this comment.
What are the implications of this change? It looks we're changing the whole PR scheme here.
There was a problem hiding this comment.
If I remember well, i'ts necessary, because a PR can come from a fork.
There was a problem hiding this comment.
Alright, I'm going to test this out and look into getting it in.
|
Pulled this in, I didn't run into any problems while testing on 4.0.1 branch. |
This PR fix the problem when the PR hook is triggered from forks. #96
Today when the PR Trigger Button is clicked or a hook is dispatched in background (new PR, Reopen, change....), the plugin notifies Jenkins with the from sha1 and the from branch, but the last is useless if you are trying to reintegrate a PR or test your integration, because set a
*,**, wherever.. in Branch Specifier field could be ambiguous and not clarify if it is a PR or not.Additionally I added a new parameter in notification called
TARGET_BRANCH, it is useful in merge when destination branch is not master or a default branch. It needs git plugin 2.4+.This PR notifies always the branch parameter in this format:
pr/<pull request id>/from. I usedfrombecause,mergeref is async #83 (comment) and if you need to merge your code before test, it could be made easy in Jenkins with:Additional Behaviours -> Merge Before Build with:
Name of repository: origin
Branch to merge to: $TARGET_BRANCH
With this PR, your Jenkins Git config should look like:
Name: origin
Refspec: +refs/pull-requests//from:refs/remotes/origin/pr/
Branch Specifier: origin/pr/*/from
sorry about my english
Signed-off-by: Marcelo Salazar chelosalazar@gmail.com