-
Notifications
You must be signed in to change notification settings - Fork 588
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
Teamcity build parameters #2069
Teamcity build parameters #2069
Conversation
No idea why travis didn't update the GH status but the build is successful |
I like this, I currently pull the branch in a nasty way! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm generally OK, but somehow I'm a bit hesitant about the concrete API (or function names, I'm not sure)
let private system = lazy(get (systemFile)) | ||
|
||
/// Get all system parameters | ||
let getAllSystem () = system.Value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somehow I don't like this API TeamCity.BuildParamters.getAllSystem()
How about making the module internal and adding
TeamCity.getAllSystemParameters()
not sure if that reads better. If you disagree or find a better solution I'm OKish with leaving as is or discussing another alternative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can, I tried grouping them but it's not mandatory, i'll try to see if I can find good names for all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the end the thing that might read the best is to remove the tryXXX
and have the getters as static properties
TeamCity.BuildParameters.System |> Map.tryFind "foo"
Would it be ok ?
@@ -235,6 +373,40 @@ module TeamCity = | |||
/// The Build number assigned to the build by TeamCity using the build number format or None if it's not on TeamCity. | |||
static member BuildNumber = Environment.environVarOrNone "BUILD_NUMBER" | |||
|
|||
/// Get the branch of the main VCS root | |||
static member Branch with get() = BuildParameters.tryGetConfiguration "vcsroot.branch" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we add these to the environment then we might add the other APIs from BuildParameters
as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible but I thing it's nice that this class/module only contains specific parameters, consumers here don't care that it came via the java parameters file or via environment variables as long as it's something team city make available
New version with a smaller API surface. Reading properties syntax become : let fooBar = TeamCity.BuildParameters.System |> Map.tryFind "foo.bar" |
Yes looks good. Thanks! |
Description
TeamCity exposes a lot of build parameters that FAKE 4 made accessible but the class and some associated methods were made internal (and unused) in FAKE 5
This PR add them back with a slighty modified API.
The public API surface change is :
It add build parameters for people who need them (We query all VCS properties that way for example) and exposes some common ones in
Environment
.PS: Lot of things can be done via build parameters, they contains for example the PATH of all versions of visual studio installed that are detected by TC