QualiSystems / FluentTc

:ocean: :two_men_holding_hands: :office: Integrate with TeamCity fluently
https://www.nuget.org/packages/fluenttc
Apache License 2.0
44 stars 36 forks source link

Feature/build configuration param rawtype support #48

Closed sokolovsv90 closed 8 years ago

sokolovsv90 commented 8 years ago

TeamCity 9.x allows for parameter typing, which is done by providing a special value to a special object (called Type) inside of the parameter object. Theese changes allow to support that and they worked for my case (needed select list

borismod commented 8 years ago

@sokolovsv90, do all the unit tests pass? Are there new tests to cover the new functionality?

sokolovsv90 commented 8 years ago

@borismod, there was two acceptance tests that didn't passed because they were expecting old behavior. I fixed that and added a tests to cover additional functionality

difference between new and old behavior is that old BuildConfigurationRetriever.SetParameters/ProjectPropertySetter.SetProjectParameters behavior sent PUT request with plain text value of parameter, and new behavior sends PUT request with json body that contains serialized parameter object (since I've found no way to set parameter type with plain text body request)

sokolovsv90 commented 8 years ago

and I ran these on my own TC instance - they do work

borismod commented 8 years ago

@sokolovsv90 thanks for contributing to FluentTc

borismod commented 8 years ago

@sokolovsv90 Why do we need PropertyType class, instead of adding RawValue property directly to Property class? IBuildParameterValueBuilder is also used in RunBuildConfiguration. Does "Triggering Build" support RawValue?

sokolovsv90 commented 8 years ago

@borismod Well, there was 2 reasons for adding PropertyType. Firstly, since in terms of TC's rest api type is a distinct object incapsulated into property object (rather than just another field, although having an incapsulated object with a single field is a little bit weird), I assumed it'd be wrong to reflect that object as field in project's domain. And secondly, auto-serialization provides {"name":"somename","value":"somevalue","type":"sometype"} when given a property object with extra field instead of required {"name":"somename","value":"somevalue","type":{"rawValue":"sometype"}}

As for triggering build - I honestly haven't looked into that. Simply because property typing is intended as guarding against invalid property values when manualy launching builds from web interface. Build triggering still accepts parameter's value, which can be anything and is not connected to parameter's type restrictions.

As long as RunBuildConfiguration doesn't explicitly check for every parameter's field being non-null, it should work as before

borismod commented 8 years ago

I am sorry for being so picky, but it's important to me to make sure we're on the same page on this matter.

  1. The Property class is used in two different purposes: it's populated when BuildConfiguration is retrieved using GetBuildConfiguration method and used as interim class for serialization purposes in SetBuildConfigurationParameters. Prior to adding the Type property the structure of the Property class fit both usages. After adding the Type property, in GetBuildConfiguration Type is always null. My suggestion is to create two separate classes for the two purposes: one will have Name, Value properties and will be used for GetBuildConfiguration and the second one will have the same structure as Property class has now (even with readonly properties, it can even be internal) and will be used in SetBuildConfigurationParameters
  2. My second concern refers to the readability of the FluentTc API and the easiness of exploring and discovering of methods usage. It was one of the main reasons I started a new project and not contributed to TeamCitySharp. I like to see clear and easy to understand syntax.
connectedTc.SetBuildConfigurationParameters(_ => 
    _.Name("FluentTc"), 
    p => p.Parameter("name", "newVal", "select data_1='lol' display='normal'"));

The above code has two disadvantages:

I suggest making this more clear by creating a builder that would be easy to read and write and would generate this rawValue string.

sokolovsv90 commented 8 years ago

Firstly, you're not picky at all, I understand your concerns.

  1. I just checked - GetBuildConfiguration returns perfectly valid BuildConfiguration object. Those parameters that had Type - still have it, those that haven't - have null there. Parameter is perfectly valid without Type defined (because, once again, this Type is used only to guard parameter value against invalid input when doing manual build launch from the TC's web interface. More on that is under the https://confluence.jetbrains.com/display/TCD9/Typed+Parameters link) and doing anything other than running build configuration from the web interface shouldn't be affected by it.
  2. That magic string is totally my fault, there should be something like IBuildParameterTypeBuilder for each possible type to match user-friendliness. I'm on it. As for multiple arguments of the same type - IBuildParameterValueBuilder Parameter(string name, string value) have already existed before and I haven't found it's usage confusing. Adding type as separate builder method (like IBuildParameterValueBuilder WithType(IBuildParameterTypeBuilder rawValue)) wouldn't work since IBuildParameterValueBuilder builds lists of parameters instead of a single parameter.

I tried to avoid changing things around as much as I could for the sake of compatibility.