netwolfuk commented 4 years ago

Installing the tcWebHooks REST API in version 1.2.0 alpha5 causes the TeamCity REST API to break with the following errors:

Packages registered by rest extensions: [jetbrains.buildServer.server.restcontrib, webhook.teamcity.server.rest] 
er.rest.APIController/rest-api - Error initializing REST API:  
java.lang.ArrayIndexOutOfBoundsException

Doesn't appear to happen in version 1.1.x.x.

See conversation at the end of #103

netwolfuk commented 4 years ago

@Nachiket26, lets deal with 9.1.x related issues on this ticket.

netwolfuk commented 4 years ago

Hi @Nachiket26 Can you please confirm which version of Java you are using with TC9?

Nachiket26 commented 4 years ago

Yes, sure @netwolfuk I am using java version "1.8.0_102" on my actual server and "1.8.0_251" to test locally.

netwolfuk commented 4 years ago

ok, great thanks. I didn't want to have to backport to Java 7 :-)

netwolfuk commented 4 years ago

I've checked out and built and deployed every commit between 1.1.x.x and v1.20.alpha3.

The problem was introduced in commit d89344cc1076dd8bb8befd351a42ace99f1678df From then, the tcWebHooks REST API fails in TeamCity 9.1.7

There is a lot to work through in that commit. The obvious one being the TeamCity version, changed from 9.1 to 10.0 But I rolled that back to 9.1 and it didn't resolve it. My current hunch is that it's related to the ExceptionMapperUtil and the TemplateInUseExceptionMapper.

Hopefully I'll get some more time over the weekend to go over the changes in that commit and find the issue.

Nachiket26 commented 4 years ago

Thank you very much @netwolfuk for the debugging. Please let me know if I can be of any help.

netwolfuk commented 4 years ago

Note to self:

    public Collection<String> getBuildTypeExternalIds(Collection<String> internalIds) {
        List<String> externalExternalIds = new ArrayList<String>();
        for (String internalId : internalIds) {
            if (this.projectManager.findBuildTypeById(internalId) != null) {
                externalExternalIds.add(this.projectManager.findBuildTypeById(internalId).getExternalId());
            }
        }
//      return internalIds.stream()
//              .filter(id ->
//                      Objects.nonNull(this.projectManager.findBuildTypeById(id))
//                   && Objects.nonNull(this.projectManager.findBuildTypeById(id).getExternalId()))
//              .map(id -> this.projectManager.findBuildTypeById(id).getExternalId())
//              .collect(Collectors.toList());
        return externalExternalIds;
    }

netwolfuk commented 4 years ago

I have found the bug. I found I still had the old branch on my home computer, which has been squashed into commit d89344c. It consisted of 27 separate commits. I went though each one, checked out the point in the git history, ran the build, deployed the plugin and restarted TC9. Fortunately TC9 restarts really fast, since it's still on the old Tomcat version. I have literally restarted TeamCity 27 times this weekend! LOL. TeamCity 9 restarts faster than the build takes to run.

I had to convert a Java8 stream to a Java 7 For loop, since it's not compatible with TC9.

This issue is that for TC9, the TeamCity REST API uses Jersey 1.16, and asm 3.1. My REST API, extends from the TC REST API, so I am constrained by what is shipped with TeamCity.

ASM 3.1, only supports Java7, so we can't use Java8 streams and such in the REST API if we want to support TeamCity 9.

It appears that since TC10, Jetbrains have moved to using Jersey 1.19, and a newer version of ASM which supports Java 8. This is why this was never seen on TC10 and above.

I eventually found this question on StackOverFlow, which helped me track down which file in the problematic commit was causing the issue.

I'll link a build for you to try ASAP.

netwolfuk commented 4 years ago

Hi @Nachiket26 Can you please test a build from here (just login as a guest).

The builds from today fix three issues: #156, #157 and also the bug we talked about with the webhooks not displaying in the Project Config page.

Nachiket26 commented 4 years ago

Hi @netwolfuk , I can confirm that with build #367 I could configure the webhook plugin for TC 9.1, change the template to Velocity template, verify that the pages open correctly and define and use macros. Thanks a lot for all your efforts. Please let me know if I can pick up this build to configure on my prod TC or you will be publishing officially on this repo.

There is a minor thing though. For the earlier builds, I used to get the value of unresolved json paths as "UNRESOLVED". e.g. when build.cvs.number was not resolved in case of multiple repositories I was getting the value of changes as "UNRESOLVED". With this build #367.

I get a very specific error : 999 :: Unexpected exception. Please log a bug on GitHub tcplugins/tcWebHooks. Exception was: Encountered "(build.vcs.number,0,7,32)}>\", \"short\": true },\n { \"title\" : \"Triggered By\", \"value\" : \"" at WebHookVelocityVariableMessageBuilder[line 13, column 97] Was expecting one of: "[" ... "}" ..

Is that an intentional change introduced? It is useful but then it becomes mandatory to keep an eye on TC events / logs for webhook events.

Thanks again.

netwolfuk commented 4 years ago

Hi @Nachiket26 thanks for taking the time to test it out for me.

With regards to the 'UNRESOLVED', it's a bit different because in Velocity it's useful to have a null returned to the template engine to make a conditional decision. However I must admit this is poorly communicated. I need to update the wiki page about velocity templates. Feel free to add it if you get a chance.

It would be easy to write a macro to null check the string and return 'UNRESOLVED'.

However it's something useful, so I'll write it into the engine and try to get you another build to try by tomorrow.

netwolfuk commented 4 years ago

It'll be something like NullUtils.toUnResolved(build.vcs.value)

netwolfuk commented 4 years ago

Hi @Nachiket26 I have added a Velocity utility called $nullUtils with the following methods:

$nullUtils.toUnResolved(Object value)
$nullUtils.toUnResolved(Object value, boolean wrapWithQuotes)

The wrapWithQuotes adds a " to the beginning and end. This is useful if you are expecting it to be an object, but if it returns a string, it will need to be wrapped.

Example usage:

{ "title" : "unresolved", "value" : "$nullUtils.toUnResolved($blah)"},
{ "title" : "unresolved-not-quoted-by-default", "value" : $nullUtils.toUnResolved($blah)},
{ "title" : "unresolved-quoted", "value" : $nullUtils.toUnResolved($blah,true) },
{ "title" : "unresolved-not-quoted", "value" : $nullUtils.toUnResolved($blah,false) },
{ "title" : "unresolved-extra-quoted", "value" : "$nullUtils.toUnResolved($blah,true)" },
{ "title" : "resolved", "value" : "$nullUtils.toUnResolved($buildNumber)"}

Example output:

{ "title" : "unresolved", "value" : "UNRESOLVED"},
{ "title" : "unresolved-not-quoted-by-default", "value" : UNRESOLVED},
{ "title" : "unresolved-quoted", "value" : "UNRESOLVED" },
{ "title" : "unresolved-not-quoted", "value" : UNRESOLVED },
{ "title" : "unresolved-extra-quoted", "value" : ""UNRESOLVED"" },
{ "title" : "resolved", "value" : "tc-main-77"}

Nachiket26 commented 4 years ago

@netwolfuk , could this be tried on build #368 or should I wait for tcWebHooksPlugin 1.2.0-alpha.6?

netwolfuk commented 4 years ago

Yes, please try it on 368. If your don't find any other showstoppers, I'll probably release that exact build as alpha 6.

Nachiket26 commented 4 years ago

368 looks perfect. Thank you very much @netwolfuk

Nachiket26 commented 4 years ago

Sorry a minor query, within the macro - I can define the whether to use a newline, comma or something as a separator, right?

#foreach( $change in $mychanges ) $change.change.username

seems to be taking /n (new-line) as a separator, so asking to confirm.

Thanks.

netwolfuk commented 4 years ago

oh, I'm not sure. Do you have a reference to the docs where it says that?

This is what I am building for slack with changes....

## Define macro called "resolveSlackUid"
## If username not found in parameter slackMapping, then just return the username from the commit
#macro( resolveSlackUid $myUserName)
#set ($uidMap = $jsonTool.jsonToMap($slackMapping))
#if( $uidMap.get($myUserName) )<@$uidMap.get($myUserName)>#else<@$myUserName>#end
#end
## Define macro called "showchanges"
#macro( showchanges $mychanges )
#if ( $mychanges.size() > 0 ) 
#foreach( $change in $mychanges )
• #substr($change.version,0,7,32) :: #resolveSlackUid($change.change.username) :: #escapejson($change.change.comment.split("\n").get(0))\n
#end
#else No Changes found #end
#end
## Define macro to resolve status
#macro( resolveStatus)
#set( $resultsMap = { "BUILD_SUCCESSFUL" : "Success", "BUILD_FAILED" : "Failed", "BUILD_FIXED" : "Success (fixed)", "BUILD_BROKEN" : "Failed (broken)" } )
#if($resultsMap.containsKey(${derivedBuildEventType}))$resultsMap.get(${derivedBuildEventType})#else#capitalise(${buildStateDescription})#end
#end
#macro( getCommitInfo )
#if(${build_vcs_number})#substr(${build_vcs_number},0,7,32)#else None #end
#end
## Define color map
#set( $colorMap = { "success" : "good", "failure" : "danger" } )
{  
    "username": "TeamCity",
    "icon_url" : "https://raw.githubusercontent.com/tcplugins/tcWebHooks/master/docs/icons/teamcity-logo-48x48.png",
    "attachments": [ 
        { 
            "title": "#resolveStatus() : ${buildName} <${buildStatusUrl}|build #${buildNumber}>", 
            "fallback": "#resolveStatus() : ${buildName} build #${buildNumber}", 
            #if($colorMap.containsKey(${buildResult})) "color": "$colorMap.get(${buildResult})", #end
            "fields" : [
                { "title" : "Status", "value" : "${buildStatus}" },
                { "title" : "Project Name", "value" : "<${rootUrl}/project.html?projectId=${projectExternalId}|${projectName}>", "short": true },
                { "title" : "Build Name", "value" : "<${rootUrl}/viewType.html?buildTypeId=${buildExternalTypeId}|${buildName}>", "short": true },
                #if(${branchDisplayName}){ "title" : "Branch", "value" : "${branchDisplayName}", "short": true },#end
                { "title" : "Commit", "value" : "<${buildStatusUrl}&tab=buildChangesDiv|#getCommitInfo()>", "short": true },
                { "title" : "Triggered By", "value" : "${triggeredBy}", "short" : true },
                { "title" : "Agent", "value" : "${agentName}", "short" : true },
                { "title" : "Changes", "value" : "#showchanges( $changes )"}
            ]
        }
    ]
}

Nachiket26 commented 4 years ago

@netwolfuk , Sorry, I did not get a chance to do it today. I'll get back to you tomorrow. Thanks.

Nachiket26 commented 4 years ago

Hi @netwolfuk ,

Sorry for the delay in response.

When I am using the following in the changes (just to add a comma after the commit :: committer pair)

`#if ( $mychanges.size() > 0 )

set( $first = true )

foreach( $change in $mychanges )

if($first)

set($first = false)

else,

end

• #substr($change.version,0,7,32) :: #resolveSlackUid($change.change.username)#end

else No Changes found #end

end`

I get the following in the preview template for the commit :: committers

    {
      "title": "Commits & Committers",
      "value": "• 911g236 :: <user1>,\n• 3c71010 :: <user2>"
    }

Now, I am not sure what that "\n" is coming from - which I think will add a line in the slack output.

Could you please check on this?

netwolfuk commented 4 years ago

Is that all one line in the template?

Nachiket26 commented 4 years ago

No it is as follows :

Define macro called "showchanges"

macro( showchanges $mychanges )

if ( $mychanges.size() > 0 )

set( $first = true )

foreach( $change in $mychanges )

if($first)

set($first = false)

else,

end

• #substr($change.version,0,7,32) :: #resolveSlackUid($change.change.username)#end

else No Changes found #end

end

netwolfuk commented 4 years ago

It's pretty yucky to read, but you can actually remove the new lines like this...

if($first)#set($first = false)#else,#end

That might fix it

netwolfuk commented 4 years ago

There is also some built in loop variables to know if you're the first iteration.

Nachiket26 commented 4 years ago

ok, Thanks @netwolfuk - It does get resolved :-) Thanks for all your help. Are you going to make release soon with 368 as alpha 6?

Nachiket26 commented 4 years ago

It seems that variables like velocityCount, etc are deprecated in 2.0. I tried an example it caused a error - so I thought I would stick to this logic.

netwolfuk commented 4 years ago

Great. I'm not 100% keen on how whitespace it handled, and will look into it in further alphas.

But yes, if you're happy I'll release this build (368) as alpha 6.

Nachiket26 commented 4 years ago

Yes, all issues are resolved. :+1:

netwolfuk commented 4 years ago

It seems that variables like velocityCount, etc are deprecated in 2.0

Oh. I was not aware of that. I think I'm still on 1.7 as that's the latest stable. I should check that though, as I was going to go to 2.2 recently.

netwolfuk commented 4 years ago

Btw. If you want a project to work on, I'd love editor to be smarter with suggestions for velocity. From memory it's a JavaScript callback to get the suggestions.

netwolfuk commented 4 years ago

Yes, all issues are resolved. 👍

Cool. I'm try to get a release out on the next 6 hours

netwolfuk commented 4 years ago

Alpha.6 Released

netwolfuk commented 2 years ago

FYI @Nachiket26 I found the page of differences from Velocity 1.7 to 2.x
https://velocity.apache.org/engine/2.3/upgrading.html

I'm doing some testing on 2.3, as there are some known issues with velocity prior to that version.

tcplugins / tcWebHooks

Investigate support in 1.2.0 for TeamCity 9.1 #156

Example usage:

Example output:

set( $first = true )

foreach( $change in $mychanges )

if($first)

set($first = false)

else,

end

else No Changes found #end

end`

Define macro called "showchanges"

macro( showchanges $mychanges )

if ( $mychanges.size() > 0 )

set( $first = true )

foreach( $change in $mychanges )

if($first)

set($first = false)

else,

end

else No Changes found #end

end

if($first)#set($first = false)#else,#end