Skip to content
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

Investigate support in 1.2.0 for TeamCity 9.1 #156

Closed
netwolfuk opened this issue Apr 28, 2020 · 33 comments
Closed

Investigate support in 1.2.0 for TeamCity 9.1 #156

netwolfuk opened this issue Apr 28, 2020 · 33 comments

Comments

@netwolfuk
Copy link
Member

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 netwolfuk added the bug label Apr 28, 2020
@netwolfuk netwolfuk added this to the v1.2 milestone Apr 28, 2020
@netwolfuk
Copy link
Member Author

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

@netwolfuk
Copy link
Member Author

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

@Nachiket26
Copy link

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
Copy link
Member Author

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

@netwolfuk
Copy link
Member Author

netwolfuk commented May 1, 2020

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 d89344c
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
Copy link

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

@netwolfuk
Copy link
Member Author

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
Copy link
Member Author

netwolfuk commented May 3, 2020

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
Copy link
Member Author

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
Copy link

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
Copy link
Member Author

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
Copy link
Member Author

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

@netwolfuk
Copy link
Member Author

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
Copy link

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

@netwolfuk
Copy link
Member Author

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
Copy link

368 looks perfect. Thank you very much @netwolfuk

@Nachiket26
Copy link

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
Copy link
Member Author

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
Copy link

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

@Nachiket26
Copy link

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
Copy link
Member Author

Is that all one line in the template?

@Nachiket26
Copy link

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
Copy link
Member Author

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
Copy link
Member Author

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

@Nachiket26
Copy link

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
Copy link

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
Copy link
Member Author

netwolfuk commented May 7, 2020

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
Copy link

Yes, all issues are resolved. 👍

@netwolfuk
Copy link
Member Author

netwolfuk commented May 7, 2020

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
Copy link
Member Author

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
Copy link
Member Author

Yes, all issues are resolved. 👍

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

@netwolfuk
Copy link
Member Author

Alpha.6 Released

@netwolfuk
Copy link
Member Author

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.

netwolfuk added a commit that referenced this issue Apr 10, 2024
TeamCity 9.x using Jersey 1.16, and asm 3.1

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. Fixes #156
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants