Enhancement/restapi update #391
Conversation
…api and fix missing "parent" property in JSON for required builds
There was a problem hiding this comment.
Thank you for the effort in testing and designing something that works on multiple versions of Bitbucket! I added a few comments around retaining API compatibility so the latest update won't break compilation for folks, but other than that I think the direction is really solid.
| 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); |
There was a problem hiding this comment.
Why drop logging the exception here?
| @@ -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"); | |||
There was a problem hiding this comment.
Nice, appreciate this update 👍
| @@ -1,4 +1,4 @@ | |||
| package org.jenkinsci.plugins.stashNotifier; | |||
| package org.jenkinsci.plugins.stashNotifier.NotifierSelectors; | |||
There was a problem hiding this comment.
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?
| @@ -8,9 +8,16 @@ public class BuildStatusUriFactory { | |||
| private BuildStatusUriFactory() { | |||
| } | |||
|
|
|||
| public static URI create(String baseUri, String commit) { | |||
There was a problem hiding this comment.
Can we leave this method around and create a new overload for the extra parameters?
|
|
||
| public NotificationContext(PrintStream logger, String runId) { |
There was a problem hiding this comment.
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 class DefaultHttpNotifierSelector implements HttpNotifierSelector { | ||
| private final List<HttpNotifier> httpNotifiers; | ||
|
|
||
| public DefaultHttpNotifierSelector(List<HttpNotifier> notifiers) { |
There was a problem hiding this comment.
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.
| */ | ||
| @NonNull HttpNotifier select(@NonNull SelectionContext context); | ||
| HttpNotifier select(@NonNull SelectionContext context); |
There was a problem hiding this comment.
Can we bring back the NonNull declaration?
| class DefaultApacheHttpNotifier implements HttpNotifier { | ||
| public class DefaultApacheHttpNotifier implements HttpNotifier { | ||
| protected static final int MAX_FIELD_LENGTH = 255; | ||
| protected static final int MAX_URL_FIELD_LENGTH = 450; |
There was a problem hiding this comment.
Nice to see these additional validations 👍
| @@ -51,32 +52,34 @@ | |||
| import java.security.NoSuchAlgorithmException; | |||
| import java.security.UnrecoverableKeyException; | |||
|
|
|||
| class DefaultApacheHttpNotifier implements HttpNotifier { | |||
| public class DefaultApacheHttpNotifier implements HttpNotifier { | |||
There was a problem hiding this comment.
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(...)
}
}
Updated Pullrequest to also include tests for requests with bitbucket projectkey and slug.
Also put creation of payload in HttpNotifier classes to make it possible to create different payloads for different versions of stash/bitbucket.
Introduces ExtendedApacheHttpNotifier for extended payload and new selector that creates the ExtendedApacheHttpNotifier only if a BitBucket projectkey and slug is provided.
Else the behaviour is the same as before.
Testing done
Submitter checklist