Add options to set PR to Approved or Needs Work #55
Conversation
| repository.markStatus(pullRequestId, "NEEDS_WORK"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
Where is the best place for this handler?
There was a problem hiding this comment.
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.
|
|
||
|
|
||
| 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.
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.
| 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.
Please use complete sentences in large comments and add space after // for readability.
There was a problem hiding this comment.
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.
| 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.
That issue is closed with "cannot reproduce". https://issues.jenkins-ci.org/browse/JENKINS-30558
There was a problem hiding this comment.
This ticket is mentioned in few other places should we also remove it entirely?
There was a problem hiding this comment.
At least don't add it blindly. We can cleanup the existing comments in a separate PR.
| </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.
Please add html help files for those fields.
There was a problem hiding this comment.
I second @proski comments.
Please take care to add good help/documentation. It's great that this works for you, but describing the feature well is key to enabling others to make use of it.
|
With the changes just merged to master local build should format the files automatically |
|
@jakub-bochenski, @proski I think that @mrdima just grabbed original pull request nemccarthy#123 as is without any code modification so he is not directly guilty :) As I'm also interested in this change I can give a hand on this. Just please guide us through all required by you code change which should be applied. |
|
I've created new pull request with provided resolved conflict, formatting and help files in #56 |
|
Let's close this PR in favor of PR #56 to avoid parallel discussions. It's out of date anyway. |
Based on the work of nemccarthy#123
We already use the original PR of tettaji successfully for the last one and a half years. In our workflow we do not automatically merge by only a successful jenkins build, we also require 2 human peer reviews next to the jenkins (build) review. We merge "manually" when we desire to do so after these 3 approvals.