huserben / TfsExtensions

Extensions for TFS 2015+ such as custom Widgets (require TFS 2017) and Build Tasks
MIT License
45 stars 22 forks source link

Trigger Build failing for path-like branches #54

Closed cdacamar closed 6 years ago

cdacamar commented 6 years ago

We have git branches like prod/fe. When trigger build is invoked it produces the following error:

2018-02-26T01:52:29.7812471Z ==============================================================================
2018-02-26T01:52:29.7812635Z Task         : Trigger Build
2018-02-26T01:52:29.7812800Z Description  : This tasks allows to trigger a new Build (add it to the queue) as part of a Build Definition. It contains as well some conditions that can be applied, for example if the last build of certain definition was successful or not.
2018-02-26T01:52:29.7813009Z Version      : 2.6.0
2018-02-26T01:52:29.7813125Z Author       : Benjamin Huser
2018-02-26T01:52:29.7813237Z Help         : 
2018-02-26T01:52:29.7813352Z ==============================================================================
2018-02-26T01:52:30.2768158Z Using current Team Project Url
2018-02-26T01:52:30.2768710Z Path to Server: https://devdiv.visualstudio.com/DevDiv
2018-02-26T01:52:30.2772512Z Using Personal Access Token
2018-02-26T01:52:30.2773145Z Build shall be triggered for same user that triggered current build: Cameron DaCamara
2018-02-26T01:52:30.2773634Z Source Version: c444f5a6d0db89579f4071765a5b2d80b88c6a53
2018-02-26T01:52:30.2773830Z Triggered Build will use the same source version: c444f5a6d0db89579f4071765a5b2d80b88c6a53
2018-02-26T01:52:30.2774008Z Using same branch as source version: refs/heads/dev/feature/modules_two_phase
2018-02-26T01:52:30.2774405Z Will trigger build with following parameters: \"BuildConfiguration\": \"chk,ret\",\"BuildPlatform\": \"x86,amd64\",\"FileSpecification\": \"CompilerAndLibraries\",\"DoPerfTests\": \"true\",\"DoSelfbuild\": \"true\",\"DoPublishToVCFS\": \"true\",\"DoBuildNoPogo\": \"true\"
2018-02-26T01:52:30.6732284Z Found parameter BuildConfiguration with value: \"chk
2018-02-26T01:52:30.6792237Z ##[error]Cannot read property 'trim' of undefined
2018-02-26T01:52:30.6851686Z ##[section]Finishing: Launch MSVC-CI
huserben commented 6 years ago

Hi

I think the problem is the parameter value "chk, ret". In the newest version the approach of how they are handled has changed. The "," causes issues. I'll have a look later today how to fix it. As a workaround, is it possible to use something else then a comma?

cdacamar commented 6 years ago

The only issue with that workaround is that those parameters are processed as "Multipliers" in the VSTS build configuration. As far as I can tell VSTS always wants comma separated lists to properly give us parallelism. We could inject a translation phase but this option is not preferable if the bug can be easily fixed.

huserben commented 6 years ago

Yes I see. I will look into it as soon as possible. However that will take roundabout one day.

huserben commented 6 years ago

So since the new version from yesterday the parameters are supplied are treated a bit differently than before. So far the string supplied was just sent "as is" to the server. This was in so far a problem as you had to escape the string yourself so that it produces correct JSON format (thus as well the wrapping of the items in \"). In other cases, it was not possible to use the parameters at all because if you specified a Variable then you had no choice and could not escape the item yourself.

Therefore now the string supplied to the task is split up first by comma so we get a list of all key-value pairs and then by colon to really get the individual keys and values and then we can apply the proper stringification (as the parameter values even need double-stringification for some reason this was the best way of doing it). Sidenote: Now you don't need to put the \" around your items anymore.

Of course this now brings problems for when we use the comma as part of the value of a variable.

My proposal would be that it is solved as follows. Given the following string entered on the Task configuration: BuildConfiguration: chk,ret,BuildPlatform: x86,amd64,FileSpecification: CompilerAndLibraries

We will still split first by the comma, which will result in following items:

When looping through all these items it is splitted by the first occurence of the colon to identifiy each parameter key and value. As the second value will have no occurence of a colon it will fail at the moment. My proposal would be that i'll check the next values if they contain any colon, if they don't I will treat them as value of the previous item.
This means we would get:

I think this would cover 99% of the use cases. However you could still have a failing task if your value that is separated by a comma contains a colon as part of the value itself. For example if you would have the following:
FileLocations: C:\Test\MyDirectory, D:\Temp\HereIHaveAnotherDirectory

However I'm not sure if this is really needed at the moment. Maybe you can tell me if you have such a use case. In order to properly implement this I would need to change the separator characters from the Task, which would mean I would need to bump the version to 3.* as the API changes and previous configurations will not work as before.

What do you think, is the above mentioned solution acceptable or do you need anyway the one that works with having multiple paths as value of one parameter?

cdacamar commented 6 years ago

Looking over our build definitions I don't believe the split path problem will be an issue. We really only have commas in places that split up non-path configuration values. This should suit our needs :+1:

huserben commented 6 years ago

Allright, thanks for the quick feedback.

Then I will implement this solution and update the documentation accordingly.

I'll update you once the new version is uploaded.

huserben commented 6 years ago

@cdacamar

Version 2.6.2 is now available and includes a fix for your problem.

Please try it out and let me know if it worked or not :-)

cdacamar commented 6 years ago

Everything appears to be running smoothly. Thank you!