Update to the 5.0 APIs, and now pass in the quarantine environment variables #56
Conversation
|
@itay: Please, make PR to https://github.com/seletskiy/atlassian-external-hooks, I do not have access to current repo anymore. |
There was a problem hiding this comment.
The changes look correct to me, in terms of how the SPI is updated.
Merging this change will absolutely require the minimum supported version for the add-on to be raised to Bitbucket Server 5.x. 4.x versions do not have the PostRepositoryHook types, and this change will fail to start when deployed to them.
One last thing I'll note. In the pom.xml for this add-on, I see it still using AMPS 6.2.9. That version is not actually compatible with Bitbucket Server 5.x. The only reason it's not causing pain is this plugin has no integration tests. Regardless, I would recommend updating AMPS to 6.3.3.
| impl.onReceive(context, refChanges, null); | ||
| } | ||
| this.authCtx, this.permissions, this.repoService, this.properties); | ||
| impl.preUpdateImpl(context, request); |
|
|
||
| @Override | ||
| public RepositoryHookResult preUpdate(PreRepositoryHookContext context, PullRequestMergeHookRequest request) { | ||
| // TODO Auto-generated method stub |
There was a problem hiding this comment.
This, and the blank line after it, should probably be removed?
| @@ -99,6 +93,10 @@ public ProcessBuilder createProcessBuilder( | |||
| log.error("Can't get user email address. getEmailAddress() call returns null"); | |||
| } | |||
| env.put("STASH_REPO_NAME", repo.getName()); | |||
|
|
|||
| if (request.getScmHookDetails().isPresent()) { | |||
There was a problem hiding this comment.
I'd write this as:
request.getScmHookDetails().ifPresent(details -> env.putAll(details.getEnvironment()));There was a problem hiding this comment.
Thanks. Fancy Java is new to me :)
| while ((data = input.read()) >= 0) { | ||
| if (count >= 65000) { | ||
| if (!trimmed) { | ||
| builder. |
There was a problem hiding this comment.
builder.append("\n")
.append("Hook response exceeds 65K length limit.\n")
.append("Further output will be trimmed\n");
trimmed = true;Maybe?
| int result = process.waitFor(); | ||
|
|
||
| if (result == 0) { | ||
| return RepositoryHookResult.accepted(); |
|
|
||
| if (result == 0) { | ||
| return RepositoryHookResult.accepted(); | ||
| } else { |
There was a problem hiding this comment.
This else is unnecessary. The if above it returns, so you could just write:
if (result == 0) {
return RepositoryHookResult.accepted();
}
return RepositoryHookResult.rejected(summaryMessage, builder.toString());| String rawDetailedMsg = errWriter.toString(); | ||
| String prePrefix = "<pre style=\"overflow: auto; white-space: nowrap;\">"; | ||
| String preSuffix = "</pre>"; | ||
| String detailedMsg = prePrefix + rawDetailedMsg.replaceAll("(\r\n|\n)", "<br/>").replaceAll(" ", " ") + preSuffix; |
There was a problem hiding this comment.
Does dropping this detailedMsg formatting affect how this displays in the UI when the check fails? Is that desirable? (Or perhaps the new hook SPI doesn't allow for the formatting to work correctly, anymore? I could believe that.)
There was a problem hiding this comment.
(Nevermind; I see from #53 that dropping this is a good thing to do. It sounds like @seletskiy has already made the matching change to his version of the plugin.)
|
@bturner thanks for the feedback. @seletskiy I will correct these and open a new PR against your repo! |
|
@itay I sounds like your fix could solve our current problems. Have you already a timing for finishing your pr? |
|
@itay: ping. |
|
Hi, |
|
We also are struggling with this and would like to see it merged. Anything I can help with? |
|
@itay @seletskiy would it be in bad form to open this PR on https://github.com/seletskiy/atlassian-external-hooks myself ? |
|
Please go ahead :) No reason to block this on my unavailability! |
|
I added this PR on seletskiy's some days ago. Hope i did it the right way... seletskiy#3 |
|
Yep, everything is fine, I just managed to obtain ownership over repository
back.
I'm going to release new version soon, so stay in touch :)
…On Wed, Jan 31, 2018, 17:28 DrVanScott ***@***.***> wrote:
I added this PR on seletskiy's some days ago. Hope i did it the right
way... seletskiy#3
<seletskiy#3>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#56 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AApL_N2XtnLfquhtV_1zTjalc4-2kfEVks5tQEBWgaJpZM4O73K0>
.
|
|
@seletskiy can you make an estimate when you would update your plugin? |
I've updated the code to properly use the new 5.0 hook APIs, as well as pass in the environment variables. This will make it work with the newer versions of Git.
I'm not sure how to properly update the pom.xml here, so I will leave that to @seletskiy or someone else who can suggest. The plugin may need to be marked as 5.0+ only now as well, though I'm not sure.
@bturner if you're able to take a look and give feedback, that would be very welcome.