Skip to content
Open
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package org.jenkinsci.plugins.stashNotifier;

public class BuildInformation {
private final String runId;
private final long runDuration;
private final StashBuildState buildState;
private final String buildKey;
private final String buildUrl;
private final String buildName;
private final String buildDescription;
public BuildInformation(String runId, long runDuration, StashBuildState buildState, String buildKey, String buildUrl, String buildName, String buildDescription) {
this.runId = runId;
this.runDuration = runDuration;
this.buildState = buildState;
this.buildKey = buildKey;
this.buildUrl = buildUrl;
this.buildName = buildName;
this.buildDescription = buildDescription;
}

public String getRunId() {
return runId;
}
public long getRunDuration() {
return runDuration;
}
public StashBuildState getBuildState() {
return buildState;
}
public String getBuildKey() { return buildKey; }
public String getBuildUrl() { return buildUrl; }
public String getBuildName() { return buildName; }
public String getBuildDescription() { return buildDescription; }
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,16 @@ public class BuildStatusUriFactory {
private BuildStatusUriFactory() {
}

public static URI create(String baseUri, String commit) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we leave this method around and create a new overload for the extra parameters?

public static URI create(String baseUri, String projectKey, String slug, String commit) {
String tidyBase = StringUtils.removeEnd(baseUri, "/");
String uri = String.join("/", tidyBase, "rest/build-status/1.0/commits", commit);

String uri;
if(projectKey == null || slug == null || projectKey.isEmpty() || slug.isEmpty()) {
uri = String.join("/", tidyBase, "rest/build-status/1.0/commits", commit);
}
else {
uri = String.join("/", tidyBase, "rest/api/latest/projects", projectKey, "repos", slug, "commits", commit, "builds");
}
return URI.create(uri);
}
}

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,12 @@
public class NotificationContext {
private final PrintStream logger;
private final String runId;
private final BuildInformation buildInformation;

public NotificationContext(PrintStream logger, String runId) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we leave the old constructor too? Happy to mark it deprecated, just would prefer the next release doesn't cause compilation issues if possible.

public NotificationContext(PrintStream logger, String runId, BuildInformation buildInformation) {
this.logger = logger;
this.runId = runId;
this.buildInformation = buildInformation;
}

/**
Expand All @@ -34,4 +36,6 @@ public PrintStream getLogger() {
public String getRunId() {
return runId;
}

public BuildInformation getBuildInformation() { return buildInformation; }
}
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ public static NotificationResult newFailure(String message) {
* @param initSuccess success flag
* @param initMessage message in case notification was not successful
*/
protected NotificationResult(
NotificationResult(
final boolean initSuccess,
final String initMessage) {

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
package org.jenkinsci.plugins.stashNotifier.NotifierSelectors;

import edu.umd.cs.findbugs.annotations.NonNull;
import org.jenkinsci.plugins.stashNotifier.Notifiers.DefaultApacheHttpNotifier;
import org.jenkinsci.plugins.stashNotifier.Notifiers.ExtendedApacheHttpNotifier;
import org.jenkinsci.plugins.stashNotifier.Notifiers.HttpNotifier;
import org.jenkinsci.plugins.stashNotifier.SelectionContext;

import java.util.*;

public class DefaultHttpNotifierSelector implements HttpNotifierSelector {
private final List<HttpNotifier> httpNotifiers;

public DefaultHttpNotifierSelector(List<HttpNotifier> notifiers) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are there more than two implementations?

I like the idea here a lot. I think it would be quite a bit simpler by having the constructor take the Default and Extended notifiers directly, rather than iterating a list for a known value.

httpNotifiers = notifiers;
}

public DefaultHttpNotifierSelector(HttpNotifier notifier) {
httpNotifiers = new ArrayList<>();
httpNotifiers.add(notifier);
}

@Override
public HttpNotifier select(@NonNull SelectionContext context) {
if(context.getBitBucketProjectKey() == null || context.getBitBucketProjectKey().isEmpty() || context.getBitBucketProjectSlug() == null || context.getBitBucketProjectSlug().isEmpty())
{
Optional<HttpNotifier> defaultNotifier = httpNotifiers.stream().filter(hn -> hn.getClass().getName().equals(DefaultApacheHttpNotifier.class.getName())).findFirst();
if(defaultNotifier.isPresent()) {
return defaultNotifier.get();
}
}

Optional<HttpNotifier> extendedNotifier = httpNotifiers.stream().filter(hn -> hn.getClass().getName().equals(ExtendedApacheHttpNotifier.class.getName())).findFirst();

return extendedNotifier.orElseGet(() -> httpNotifiers.get(0));
}
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package org.jenkinsci.plugins.stashNotifier;
package org.jenkinsci.plugins.stashNotifier.NotifierSelectors;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I appreciate the effort to keep similar implementations together, but unfortunately this is a breaking change.

Can we leave it in the original package to minimize the work consumers will have to do?


import edu.umd.cs.findbugs.annotations.NonNull;
import hudson.FilePath;
Expand All @@ -7,6 +7,9 @@
import hudson.model.BuildListener;
import hudson.model.Run;
import hudson.model.TaskListener;
import org.jenkinsci.plugins.stashNotifier.Notifiers.HttpNotifier;
import org.jenkinsci.plugins.stashNotifier.SelectionContext;
import org.jenkinsci.plugins.stashNotifier.StashNotifier;

/**
* Implement this interface to have more control over which {@link HttpNotifier}
Expand All @@ -21,10 +24,10 @@ public interface HttpNotifierSelector {
* this method useful for performing migrations on a running system without
* restarts.
*
* @see StashNotifier#prebuild(AbstractBuild, BuildListener)
* @see StashNotifier#perform(Run, FilePath, Launcher, TaskListener)
* @param context parameters useful for selecting a notifier
* @return selected notifier
* @see StashNotifier#prebuild(AbstractBuild, BuildListener)
* @see StashNotifier#perform(Run, FilePath, Launcher, TaskListener)
*/
@NonNull HttpNotifier select(@NonNull SelectionContext context);
HttpNotifier select(@NonNull SelectionContext context);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we bring back the NonNull declaration?

}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package org.jenkinsci.plugins.stashNotifier;
package org.jenkinsci.plugins.stashNotifier.Notifiers;

import com.cloudbees.plugins.credentials.Credentials;
import com.cloudbees.plugins.credentials.common.CertificateCredentials;
Expand Down Expand Up @@ -36,6 +36,7 @@
import org.apache.http.ssl.SSLContexts;
import org.apache.http.util.EntityUtils;
import org.jenkinsci.plugins.plaincredentials.StringCredentials;
import org.jenkinsci.plugins.stashNotifier.*;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand All @@ -51,32 +52,34 @@
import java.security.NoSuchAlgorithmException;
import java.security.UnrecoverableKeyException;

class DefaultApacheHttpNotifier implements HttpNotifier {
public class DefaultApacheHttpNotifier implements HttpNotifier {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What do you think of adding a DelegatingHttpNotifier that makes the decision of which notifier to use based on the context? I've found that a simple pattern that makes it easy to retain API compatibility.

class DelegatingHttpNotifier implements HttpNotifier {
    private final HttpNotifier standard;
    private final HttpNotifier extended;

    NotificationResult send (context, ...) {
        return context.supportsExtended() ? extended.send(...) : standard.send(...)
    } 
}

protected static final int MAX_FIELD_LENGTH = 255;
protected static final int MAX_URL_FIELD_LENGTH = 450;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice to see these additional validations 👍


private static final Logger LOGGER = LoggerFactory.getLogger(DefaultApacheHttpNotifier.class);

@Override
public @NonNull NotificationResult send(@NonNull URI uri, @NonNull JSONObject payload, @NonNull NotificationSettings settings, @NonNull NotificationContext context) {
public @NonNull NotificationResult send(@NonNull URI uri, @NonNull NotificationSettings settings, @NonNull NotificationContext context) {
PrintStream logger = context.getLogger();
JSONObject payload = createNotificationPayload(context);
try (CloseableHttpClient client = getHttpClient(logger, uri, settings.isIgnoreUnverifiedSSL())) {
HttpPost req = createRequest(uri, payload, settings.getCredentials(), context);
HttpResponse res = client.execute(req);

if (res.getStatusLine().getStatusCode() != 204) {
return NotificationResult.newFailure(EntityUtils.toString(res.getEntity()));
} else {
return NotificationResult.newSuccess();
}
} catch (Exception e) {
LOGGER.warn("{} failed to send {} to Bitbucket Server at {}", context.getRunId(), payload, uri, e);
LOGGER.warn("{} failed to send {} to Bitbucket Server at {}", context.getRunId(), payload, uri);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why drop logging the exception here?

logger.println("Failed to notify Bitbucket Server");
return NotificationResult.newFailure(e.getMessage());
}
}

HttpPost createRequest(
final URI uri,
final JSONObject payload,
final Credentials credentials,
@NonNull NotificationContext context) throws AuthenticationException {
protected HttpPost createRequest(@NonNull final URI uri, final JSONObject payload, final Credentials credentials,
@NonNull NotificationContext context) throws AuthenticationException {
HttpPost req = new HttpPost(uri.toString());

if (credentials != null) {
Expand All @@ -94,7 +97,7 @@ else if (credentials instanceof StringCredentials) {
req.addHeader(HttpHeaders.AUTHORIZATION, "Bearer " + ((StringCredentials)credentials).getSecret().getPlainText());
}
else {
throw new AuthenticationException("Unsupported credials");
throw new AuthenticationException("Unsupported credentials");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice, appreciate this update 👍

}
}

Expand All @@ -104,7 +107,28 @@ else if (credentials instanceof StringCredentials) {
return req;
}

CloseableHttpClient getHttpClient(PrintStream logger, URI stashServer, boolean ignoreUnverifiedSSL) throws Exception {
/**
* Returns the HTTP POST entity body with the JSON representation of the
* run result to be sent to the Bitbucket build API.
*
* @param context the NotificationContext for the current build
* @return JSON body for POST to Bitbucket build API
*/
public JSONObject createNotificationPayload(NotificationContext context) {

BuildInformation information = context.getBuildInformation();
String buildId = abbreviate(information.getBuildKey(), MAX_FIELD_LENGTH);

JSONObject json = new JSONObject();
json.put("state", information.getBuildState().name());
json.put("key", buildId);
json.put("name", abbreviate(information.getBuildName(), MAX_FIELD_LENGTH));
json.put("description", abbreviate(information.getBuildDescription(), MAX_FIELD_LENGTH));
json.put("url", abbreviate(information.getBuildUrl(), MAX_URL_FIELD_LENGTH));
return json;
}

protected CloseableHttpClient getHttpClient(PrintStream logger, @NonNull URI stashServer, boolean ignoreUnverifiedSSL) throws Exception {
final int timeoutInMilliseconds = 60_000;

RequestConfig.Builder requestBuilder = RequestConfig.custom()
Expand Down Expand Up @@ -153,7 +177,7 @@ CloseableHttpClient getHttpClient(PrintStream logger, URI stashServer, boolean i
return clientBuilder.build();
}

SSLContext buildSslContext(boolean ignoreUnverifiedSSL, Credentials credentials) throws UnrecoverableKeyException, NoSuchAlgorithmException, KeyStoreException, KeyManagementException {
protected SSLContext buildSslContext(boolean ignoreUnverifiedSSL, Credentials credentials) throws UnrecoverableKeyException, NoSuchAlgorithmException, KeyStoreException, KeyManagementException {
SSLContextBuilder contextBuilder = SSLContexts.custom();
contextBuilder.setProtocol("TLS");
if (credentials instanceof CertificateCredentials) {
Expand All @@ -179,13 +203,13 @@ void configureProxy(HttpClientBuilder builder, URL url) {
return;
}

SocketAddress addr = proxy.address();
if (!(addr instanceof InetSocketAddress)) {
SocketAddress address = proxy.address();
if (!(address instanceof InetSocketAddress)) {
return;
}

InetSocketAddress proxyAddr = (InetSocketAddress) addr;
HttpHost proxyHost = new HttpHost(proxyAddr.getAddress().getHostAddress(), proxyAddr.getPort());
InetSocketAddress proxyAddress = (InetSocketAddress) address;
HttpHost proxyHost = new HttpHost(proxyAddress.getAddress().getHostAddress(), proxyAddress.getPort());
builder.setProxy(proxyHost);

String proxyUser = proxyConfig.getUserName();
Expand All @@ -198,4 +222,17 @@ void configureProxy(HttpClientBuilder builder, URL url) {
.setProxyAuthenticationStrategy(new ProxyAuthenticationStrategy());
}
}

protected static String abbreviate(String text, int maxWidth) {
if (text == null) {
return null;
}
if (maxWidth < 4) {
throw new IllegalArgumentException("Minimum abbreviation width is 4");
}
if (text.length() <= maxWidth) {
return text;
}
return text.substring(0, maxWidth - 3) + "...";
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package org.jenkinsci.plugins.stashNotifier.Notifiers;

import net.sf.json.JSONObject;
import org.jenkinsci.plugins.stashNotifier.BuildInformation;
import org.jenkinsci.plugins.stashNotifier.NotificationContext;

public class ExtendedApacheHttpNotifier extends DefaultApacheHttpNotifier {
/**
* Returns the HTTP POST entity body with the JSON representation of the
* run result to be sent to the Bitbucket build API.
*
* @param context the NotificationContext for the current build
* @return JSON body for POST to Bitbucket build API
*/
@Override
public JSONObject createNotificationPayload(NotificationContext context) {

BuildInformation information = context.getBuildInformation();
String buildId = abbreviate(information.getBuildKey(), MAX_FIELD_LENGTH);

JSONObject json = super.createNotificationPayload(context);

json.put("parent", buildId);
json.put("buildNumber", information.getRunId());
json.put("duration", information.getRunDuration());

return json;
}
}
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
package org.jenkinsci.plugins.stashNotifier;
package org.jenkinsci.plugins.stashNotifier.Notifiers;

import edu.umd.cs.findbugs.annotations.NonNull;
import net.sf.json.JSONObject;
import org.jenkinsci.plugins.stashNotifier.NotificationContext;
import org.jenkinsci.plugins.stashNotifier.NotificationResult;
import org.jenkinsci.plugins.stashNotifier.NotificationSettings;

import java.net.URI;

Expand All @@ -13,11 +15,10 @@ public interface HttpNotifier {
* Basic contract for sending Bitbucket build status notifications.
*
* @param uri fully-formed URI (stash-base-uri/rest/build-status/1.0/commits/commit-id)
* @param payload body of status to post
* @param settings user or administrator defined settings for the request
* @param context build info
* @return result of posting status
*/
@NonNull
NotificationResult send(@NonNull URI uri, @NonNull JSONObject payload, @NonNull NotificationSettings settings, @NonNull NotificationContext context);
NotificationResult send(@NonNull URI uri, @NonNull NotificationSettings settings, @NonNull NotificationContext context);
}
Loading