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

Fix pull request triggers from forks #172

Merged
merged 2 commits into from
Sep 1, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 28 additions & 9 deletions src/main/java/com/nerdwin15/stash/webhook/Notifier.java
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,11 @@ public class Notifier implements DisposableBean {
*/
public static final String OMIT_BRANCH_NAME = "omitBranchName";

/**
* Field name for the omit target branch property
*/
public static final String OMIT_TARGET_BRANCH = "omitTargetBranch";

/**
* Field name for the ignore committers property
*/
Expand All @@ -94,6 +99,7 @@ public class Notifier implements DisposableBean {
private static final String BASE_URL = "%s/git/notifyCommit?url=%s";
private static final String HASH_URL_PARAMETER = "&sha1=%s";
private static final String BRANCH_URL_PARAMETER = "&branches=%s";
private static final String TARGET_BRANCH_PARAMETER = "&TARGET_BRANCH=%s";

private final HttpClientFactory httpClientFactory;
private final SettingsService settingsService;
Expand Down Expand Up @@ -130,15 +136,16 @@ public Notifier(SettingsService settingsService,
* @param repo The repository to base the notification on.
* @param strRef The branch ref related to the commit
* @param strSha1 The commit's SHA1 hash code.
* @param targetBranch Target branch can be used in merge
* @return A future of the text result from Jenkins
*/
@Nonnull
public Future<NotificationResult> notifyBackground(@Nonnull final Repository repo, //CHECKSTYLE:annot
final String strRef, final String strSha1) {
final String strRef, final String strSha1, final String targetBranch) {
return executorService.submit(new Callable<NotificationResult>() {
@Override
public NotificationResult call() throws Exception {
return Notifier.this.notify(repo, strRef, strSha1);
return Notifier.this.notify(repo, strRef, strSha1, targetBranch);
}
});
}
Expand All @@ -148,10 +155,11 @@ public NotificationResult call() throws Exception {
* @param repo The repository to base the notification on.
* @param strRef The branch ref related to the commit
* @param strSha1 The commit's SHA1 hash code.
* @param targetBranch Target branch can be used in merge
* @return Text result from Jenkins
*/
public @Nullable NotificationResult notify(@Nonnull Repository repo, //CHECKSTYLE:annot
String strRef, String strSha1) {
String strRef, String strSha1, String targetBranch) {
final RepositoryHook hook = settingsService.getRepositoryHook(repo);
final Settings settings = settingsService.getSettings(repo);
if (hook == null || !hook.isEnabled() || settings == null) {
Expand All @@ -163,9 +171,10 @@ public NotificationResult call() throws Exception {
settings.getBoolean(IGNORE_CERTS, false),
settings.getString(CLONE_TYPE),
settings.getString(CLONE_URL),
strRef, strSha1,
strRef, strSha1, targetBranch,
settings.getBoolean(OMIT_HASH_CODE, false),
settings.getBoolean(OMIT_BRANCH_NAME, false));
settings.getBoolean(OMIT_BRANCH_NAME, false),
settings.getBoolean(OMIT_TARGET_BRANCH, false));
}

/**
Expand All @@ -177,21 +186,25 @@ public NotificationResult call() throws Exception {
* @param cloneUrl The repository url
* @param strRef The branch ref related to the commit
* @param strSha1 The commit's SHA1 hash code.
* @param targetBranch Target branch can be used in merge
* @param omitHashCode Defines whether the commit's SHA1 hash code is omitted
* in notification to Jenkins.
* @param omitBranchName Defines whether the commit's branch name is omitted
* @param omitTargetBranch Omit the target branch
* @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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I wrote in that manner to maintain consistency with the others vars.


HttpClient client = null;
String url;

try {
url = getUrl(repo, maybeReplaceSlash(jenkinsBase),
cloneType, cloneUrl, strRef, strSha1, omitHashCode, omitBranchName);
cloneType, cloneUrl, strRef, strSha1, targetBranch, omitHashCode, omitBranchName,
omitTargetBranch);
} catch (Exception e) {
LOGGER.error("Error getting Jenkins URL", e);
return new NotificationResult(false, null, e.getMessage());
Expand Down Expand Up @@ -235,12 +248,16 @@ public void destroy() {
* @param customCloneUrl The url used for cloning the repository
* @param strRef The branch ref related to the commit
* @param strSha1 The commit's SHA1 hash code.
* @param targetBranch Target branch can be used in merge
* @param omitHashCode Defines whether the commit's SHA1 hash code is omitted
* in notification to Jenkins.
* @param omitTargetBranch Omit the target branch
* @return The url to use for notifying Jenkins
*/
protected String getUrl(final Repository repository, final String jenkinsBase,
final String cloneType, final String customCloneUrl, final String strRef, final String strSha1, boolean omitHashCode, boolean omitBranchName) {
final String cloneType, final String customCloneUrl, final String strRef,
final String strSha1, final String targetBranch, boolean omitHashCode,
boolean omitBranchName, boolean omitTargetBranch) {
String cloneUrl = customCloneUrl;
// Older installs won't have a cloneType value - treat as custom
if (cloneType != null && !cloneType.equals("custom")) {
Expand All @@ -263,7 +280,9 @@ protected String getUrl(final Repository repository, final String jenkinsBase,
url.append(String.format(BRANCH_URL_PARAMETER, urlEncode(strRef)));
if(strSha1 != null && !omitHashCode)
url.append(String.format(HASH_URL_PARAMETER, strSha1));

if(targetBranch != null && !omitTargetBranch)
url.append(String.format(TARGET_BRANCH_PARAMETER, targetBranch));

return url.toString();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the implications of this change? It looks we're changing the whole PR scheme here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I remember well, i'ts necessary, because a PR can come from a fork.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I'm going to test this out and look into getting it in.

String strSha1 = event.getPullRequest().getFromRef().getLatestCommit();
String targetBranch = event.getPullRequest().getToRef().getDisplayId();

EventContext context = new EventContext(event,
event.getPullRequest().getToRef().getRepository(),
event.getUser().getName());

if (filterChain.shouldDeliverNotification(context))
notifier.notifyBackground(context.getRepository(), strRef, strSha1);
notifier.notifyBackground(context.getRepository(), strRef, strSha1, targetBranch);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -55,12 +55,13 @@ public void onRefsChangedEvent(RepositoryRefsChangedEvent event) {
// Leaving this as-is in case someone relies on that...
String strRef = refCh.getRef().getId().replaceFirst("refs/heads/", "");
String strSha1 = refCh.getToHash();
String targetBranch = refCh.getRef().getDisplayId();

String user = (event.getUser() != null) ? event.getUser().getName() : null;
EventContext context = new EventContext(event, event.getRepository(), user);

if (filterChain.shouldDeliverNotification(context))
notifier.notifyBackground(context.getRepository(), strRef, strSha1);
notifier.notifyBackground(context.getRepository(), strRef, strSha1, targetBranch);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ public JenkinsResource(Notifier notifier,
* @param ignoreCerts True if all certs should be accepted.
* @param omitHashCode True if SHA1 hash should be omitted.
* @param omitBranchName True if branch name should be omitted.
* @param omitTargetBranch True if the PR destination branch should be omitted
* @return The response to send back to the user.
*/
@POST
Expand All @@ -97,7 +98,8 @@ public Map<String, Object> test(@Context Repository repository,
@FormParam(Notifier.CLONE_URL) String cloneUrl,
@FormParam(Notifier.IGNORE_CERTS) boolean ignoreCerts,
@FormParam(Notifier.OMIT_HASH_CODE) boolean omitHashCode,
@FormParam(Notifier.OMIT_BRANCH_NAME) boolean omitBranchName) {
@FormParam(Notifier.OMIT_BRANCH_NAME) boolean omitBranchName,
@FormParam(Notifier.OMIT_TARGET_BRANCH) boolean omitTargetBranch) {

if (jenkinsBase == null || cloneType == null || (cloneType.equals("custom") && cloneUrl == null)) {
Map<String, Object> map = new HashMap<String, Object>();
Expand All @@ -114,7 +116,8 @@ public Map<String, Object> test(@Context Repository repository,
Branch defaultBranch = refService.getDefaultBranch(repository);
NotificationResult result = notifier.notify(repository, jenkinsBase,
ignoreCerts, cloneType, cloneUrl, defaultBranch.getDisplayId(),
defaultBranch.getLatestCommit(), omitHashCode, omitBranchName);
defaultBranch.getLatestCommit(), defaultBranch.getDisplayId(),
omitHashCode, omitBranchName, omitTargetBranch);
log.debug("Got response from jenkins: {}", result);

// Shouldn't have to do this but the result isn't being marshalled correctly
Expand All @@ -133,10 +136,11 @@ public Map<String, Object> test(@Context Repository repository,
@POST
@Path(value = "triggerJenkins")
public Response trigger(@Context Repository repository,
@QueryParam("branches") String branches, @QueryParam("sha1") String sha1) {
@QueryParam("branches") String branches, @QueryParam("sha1") String sha1,
@QueryParam("targetBranch") String targetBranch) {

try {
NotificationResult result = notifier.notify(repository, branches, sha1);
NotificationResult result = notifier.notify(repository, branches, sha1, targetBranch);
if (result.isSuccessful())
return Response.ok().build();
return Response.noContent().build();
Expand Down
2 changes: 2 additions & 0 deletions src/main/resources/i18n/stash-webhook-jenkins.properties
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ stash.webhook.omitHashCode.label=Omit SHA1 Hash Code
stash.webhook.omitHashCode.description=Do not send the commit''s SHA1 hash code to Jenkins
stash.webhook.omitBranchName.label=Omit Branch Name
stash.webhook.omitBranchName.description=Do not send the commit''s branch name to Jenkins
stash.webhook.omitTargetBranch.label=Omit Target Branch
stash.webhook.omitTargetBranch.description=Do not send the target branch name to Jenkins. Useful in merge.
stash.webhook.omitTriggerBuildButton.label=Omit the Trigger Build Button
stash.webhook.omitTriggerBuildButton.description=Do not display the Trigger Build Button on the pull request overview page
stash.webhook.test=Configuration check
Expand Down
5 changes: 3 additions & 2 deletions src/main/resources/static/jenkins-pr-triggerbutton.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@ define('plugin/jenkins/pr-triggerbutton', [
return AJS.contextPath() + '/rest/jenkins/latest/projects/'
+ pageState.getProject().getKey() + '/repos/'
+ pageState.getRepository().getSlug() + '/triggerJenkins'
+ '?branches=' + pageState.getPullRequest().getFromRef().getDisplayId()
+ '&sha1=' + pageState.getPullRequest().getFromRef().getLatestCommit();
+ '?branches=pr/' + pageState.getPullRequest().getId() + '/from'
+ '&sha1=' + pageState.getPullRequest().getFromRef().getLatestCommit()
+ '&targetBranch=' + pageState.getPullRequest().getToRef().getDisplayId();
};

var waiting = '<span class="aui-icon aui-icon-wait">Wait</span>';
Expand Down
4 changes: 3 additions & 1 deletion src/main/resources/static/jenkins.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ define('plugin/jenkins/test', [
$ignoreCerts = $("#ignoreCerts"),
$omitHashCode = $("#omitHashCode"),
$omitBranchName = $("#omitBranchName"),
$omitTargetBranch = $("#omitTargetBranch"),
$status,
defaultUrls;

Expand Down Expand Up @@ -87,7 +88,8 @@ define('plugin/jenkins/test', [
'gitRepoUrl': [$cloneUrl.val()],
'ignoreCerts': [$ignoreCerts.attr('checked') ? "TRUE" : "FALSE"],
'omitHashCode': [$omitHashCode.attr('checked') ? "TRUE" : "FALSE"],
'omitBranchName': [$omitBranchName.attr('checked') ? "TRUE" : "FALSE"]
'omitBranchName': [$omitBranchName.attr('checked') ? "TRUE" : "FALSE"],
'omitTargetBranch': [$omitTargetBranch.attr('checked') ? "TRUE" : "FALSE"]
}
}).always(function () {
setDeleteButtonEnabled(true)
Expand Down
12 changes: 12 additions & 0 deletions src/main/resources/static/jenkins.soy
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,18 @@
{param description: getText('stash.webhook.omitBranchName.description') /}
{/call}

{call widget.aui.form.checkbox}
{param id: 'omitTargetBranch' /}
{param checked: $config['omitTargetBranch'] /}
{param labelContent}
{getText('stash.webhook.omitTargetBranch.label')}
{/param}
{param labelHtml}
{getText('stash.webhook.omitTargetBranch.label')}
{/param}
{param description: getText('stash.webhook.omitTargetBranch.description') /}
{/call}

{call widget.aui.form.checkbox}
{param id: 'omitTriggerBuildButton' /}
{param checked: $config['omitTriggerBuildButton'] /}
Expand Down
Loading