-
Notifications
You must be signed in to change notification settings - Fork 56
Add options to set PR to Approved or Needs Work #55
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
package stashpullrequestbuilder.stashpullrequestbuilder; | ||
|
||
import hudson.model.Result; | ||
import hudson.model.Run; | ||
|
||
public class StashMarkStatus { | ||
|
||
public void handleStatus(Boolean approveOnBuildSuccessful, Boolean needsWorkOnBuildFailure, String pullRequestId, | ||
Result result, StashRepository repository) { | ||
if(approveOnBuildSuccessful && result == Result.SUCCESS) { | ||
repository.markStatus(pullRequestId, "APPROVED"); | ||
} | ||
|
||
if(needsWorkOnBuildFailure && result == Result.FAILURE) { | ||
repository.markStatus(pullRequestId, "NEEDS_WORK"); | ||
} | ||
} | ||
} | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ | |
import org.apache.commons.httpclient.methods.DeleteMethod; | ||
import org.apache.commons.httpclient.methods.GetMethod; | ||
import org.apache.commons.httpclient.methods.PostMethod; | ||
import org.apache.commons.httpclient.methods.PutMethod; | ||
import org.apache.commons.httpclient.methods.StringRequestEntity; | ||
import org.apache.commons.httpclient.params.HttpConnectionParams; | ||
import org.apache.commons.httpclient.params.HttpParams; | ||
|
@@ -64,11 +65,13 @@ public class StashApiClient { | |
private String project; | ||
private String repositoryName; | ||
private Credentials credentials; | ||
private String username; | ||
|
||
|
||
public StashApiClient(String stashHost, String username, String password, String project, String repositoryName, boolean ignoreSsl) { | ||
this.credentials = new UsernamePasswordCredentials(username, password); | ||
this.project = project; | ||
this.username = username; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are going to reformat the whole codebase and enable automatic formatting, which would make formatting issues irrelevant. However, please use reasonable formatting in your submissions to facilitate reviews. |
||
this.repositoryName = repositoryName; | ||
this.apiBaseUrl = stashHost.replaceAll("/$", "") + "/rest/api/1.0/projects/"; | ||
if (ignoreSsl) { | ||
|
@@ -128,6 +131,17 @@ public void deletePullRequestComment(String pullRequestId, String commentId) { | |
deleteRequest(path); | ||
} | ||
|
||
public void markStatus(String pullRequestId, String status) { | ||
String path = pullRequestPath(pullRequestId) + "/participants/" + username; | ||
|
||
try { | ||
putRequest(path, status); | ||
} catch (UnsupportedEncodingException e) { | ||
e.printStackTrace(); | ||
} catch (IOException e) { | ||
logger.log(Level.SEVERE, "Failed to mark Stash PR status " + path + " " + e); | ||
} | ||
} | ||
|
||
public StashPullRequestComment postPullRequestComment(String pullRequestId, String comment) { | ||
String path = pullRequestPath(pullRequestId) + "/comments"; | ||
|
@@ -414,6 +428,94 @@ public Callable<String> init(HttpClient client, PostMethod httppost) { | |
return response; | ||
} | ||
|
||
private String putRequest(String path, String status) throws UnsupportedEncodingException { | ||
logger.log(Level.FINEST, "PR-PUT-REQUEST:" + path + " with: " + status); | ||
HttpClient client = getHttpClient(); | ||
client.getState().setCredentials(AuthScope.ANY, credentials); | ||
|
||
PutMethod httpput = new PutMethod(path); | ||
//http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html; section 14.10. | ||
//tells the server that we want it to close the connection when it has sent the response. | ||
//address large amount of close_wait sockets client and fin sockets server side | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use complete sentences in large comments and add space after There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The same comment is used also in 2 other places. Do you think it should be also fixed there? Btw it just a extract from html rfc about closing connection - IMHO this comment can be removed entirely. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's remove it then. |
||
httpput.setRequestHeader("Connection", "close"); | ||
httpput.setRequestHeader("X-Atlassian-Token", "no-check"); //xsrf | ||
|
||
if (status != null) { | ||
ObjectNode node = mapper.getNodeFactory().objectNode(); | ||
node.put("status", status); | ||
StringRequestEntity requestEntity = null; | ||
try { | ||
requestEntity = new StringRequestEntity( | ||
mapper.writeValueAsString(node), | ||
"application/json", | ||
"UTF-8"); | ||
} catch (IOException e) { | ||
e.printStackTrace(); | ||
} | ||
httpput.setRequestEntity(requestEntity); | ||
} | ||
|
||
String response = ""; | ||
FutureTask<String> httpTask = null; | ||
Thread thread; | ||
|
||
try { | ||
//Run the http request in a future task so we have the opportunity | ||
//to cancel it if it gets hung up; which is possible if stuck at | ||
//socket native layer. see issue JENKINS-30558 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That issue is closed with "cannot reproduce". https://issues.jenkins-ci.org/browse/JENKINS-30558 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This ticket is mentioned in few other places should we also remove it entirely? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At least don't add it blindly. We can cleanup the existing comments in a separate PR. |
||
httpTask = new FutureTask<String>(new Callable<String>() { | ||
|
||
private HttpClient client; | ||
private PutMethod httpput; | ||
|
||
@Override | ||
public String call() throws Exception { | ||
|
||
String response = ""; | ||
int responseCode = HttpStatus.SC_INTERNAL_SERVER_ERROR; | ||
responseCode = client.executeMethod(httpput); | ||
if (!validResponseCode(responseCode)) { | ||
logger.log(Level.SEVERE, "Failing to get response from Stash PR PUT" + httpput.getURI().getPath()); | ||
throw new RuntimeException("Didn't get a 200 response from Stash PR PUT! Response; '" + | ||
responseCode + "' with message; " + response); | ||
} | ||
InputStream responseBodyAsStream = httpput.getResponseBodyAsStream(); | ||
StringWriter stringWriter = new StringWriter(); | ||
IOUtils.copy(responseBodyAsStream, stringWriter, "UTF-8"); | ||
response = stringWriter.toString(); | ||
logger.log(Level.FINEST, "API Request Response: " + response); | ||
|
||
return response; | ||
|
||
} | ||
|
||
public Callable<String> init(HttpClient client, PutMethod httpput) { | ||
this.client = client; | ||
this.httpput = httpput; | ||
return this; | ||
} | ||
|
||
}.init(client, httpput)); | ||
thread = new Thread(httpTask); | ||
thread.start(); | ||
response = httpTask.get((long) StashApiClient.HTTP_REQUEST_TIMEOUT_SECONDS, TimeUnit.SECONDS); | ||
|
||
} catch (TimeoutException e) { | ||
e.printStackTrace(); | ||
httpput.abort(); | ||
throw new RuntimeException(e); | ||
} catch (Exception e) { | ||
e.printStackTrace(); | ||
throw new RuntimeException(e); | ||
} finally { | ||
httpput.releaseConnection(); | ||
} | ||
|
||
logger.log(Level.FINEST, "PR-PUT-RESPONSE:" + response); | ||
|
||
return response; | ||
} | ||
|
||
private boolean validResponseCode(int responseCode) { | ||
return responseCode == HttpStatus.SC_OK || | ||
responseCode == HttpStatus.SC_ACCEPTED || | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -52,5 +52,11 @@ | |
<f:entry title="Phrase to request a (re-)build" field="ciBuildPhrases"> | ||
<f:textbox default="test this please"/> | ||
</f:entry> | ||
<f:entry title="Approve PR on build success" field="approveOnBuildSuccessful"> | ||
<f:checkbox default="false"/> | ||
</f:entry> | ||
<f:entry title="Mark PR with Needs Work on build failure" field="needsWorkOnBuildFailure"> | ||
<f:checkbox default="false"/> | ||
</f:entry> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add html help files for those fields. |
||
</f:advanced> | ||
</j:jelly> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
package stashpullrequestbuilder.stashpullrequestbuilder; | ||
|
||
import hudson.model.Result; | ||
|
||
import org.junit.Before; | ||
import org.junit.Test; | ||
|
||
import static org.mockito.Mockito.*; | ||
|
||
public class StashMarkStatusTest { | ||
|
||
private StashRepository repository; | ||
|
||
@Before | ||
public void setUp() throws Exception { | ||
repository = mock(StashRepository.class); | ||
} | ||
|
||
@Test | ||
public void handleStatus_shouldMarkStatusApprovedOnSuccessfulBuild() throws Exception { | ||
StashMarkStatus status = new StashMarkStatus(); | ||
|
||
status.handleStatus(true, false, "", Result.SUCCESS, repository); | ||
|
||
verify(repository).markStatus("", "APPROVED"); | ||
} | ||
|
||
@Test | ||
public void handleStatus_shouldMarkStatusNeedsWorkOnFailedBuild() throws Exception { | ||
StashMarkStatus status = new StashMarkStatus(); | ||
|
||
status.handleStatus(false, true, "", Result.FAILURE, repository); | ||
|
||
verify(repository).markStatus("", "NEEDS_WORK"); | ||
} | ||
|
||
@Test | ||
public void handleStatus_shouldNotMarkStatusApprovedWhenDisabled() throws Exception { | ||
StashMarkStatus status = new StashMarkStatus(); | ||
|
||
status.handleStatus(false, false, "", Result.SUCCESS, repository); | ||
|
||
verify(repository, never()).markStatus("", "APPROVED"); | ||
} | ||
|
||
@Test | ||
public void handleStatus_shouldNotMarkStatusNeedsWorkWhenDisabled() throws Exception { | ||
StashMarkStatus status = new StashMarkStatus(); | ||
|
||
status.handleStatus(false, false, "", Result.FAILURE, repository); | ||
|
||
verify(repository, never()).markStatus("", "NEEDS_WORK"); | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think
StashMarkStatus
needs to be a separate class. I'd rather stop proliferation of classes that have no state or map 1-to-1 to the existing classes.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is the best place for this handler?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just put that code where it's being called. If the code gets too long, we care refactor it across more natural lines.