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

Teamcity build parameters #2069

Merged
merged 6 commits into from
Sep 3, 2018

Conversation

vbfox
Copy link
Contributor

@vbfox vbfox commented Aug 22, 2018

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 :

module Fake.BuildServer.TeamCity =
+	module BuildParameters =
+		System: Map<string, string>
+		Configuration: Map<string, string>
+		Runner: Map<string, string>
+		All: Map<string, string>
+	type FileChangeType = // DU
+	type FileChange = // Record
	type Environment =
+		Branch: string option
+		BranchDisplayName: string option
+		IsDefaultBranch: string option
+		CheckoutDirectory: string option
+		ChangedFiles: FileChange  list option
+		RecentlyFailedTests: string list option

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

@vbfox
Copy link
Contributor Author

vbfox commented Aug 23, 2018

No idea why travis didn't update the GH status but the build is successful

@BlythMeister
Copy link
Contributor

I like this, I currently pull the branch in a nasty way!

Copy link
Member

@matthid matthid left a 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
Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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

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?

Copy link
Contributor Author

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

@vbfox
Copy link
Contributor Author

vbfox commented Sep 2, 2018

New version with a smaller API surface.

Reading properties syntax become :

let fooBar = TeamCity.BuildParameters.System |> Map.tryFind "foo.bar"

@matthid
Copy link
Member

matthid commented Sep 3, 2018

Yes looks good. Thanks!
One the one hand I'd like to push people into using "platform independent" APIs (therefore the low API surface of the build server modules). But I see from working with the TeamFoundation module lately that this is not always so easy..

@matthid matthid merged commit 3b8828f into fsprojects:release/next Sep 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants