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.

Sign up for GitHub

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

Jump to bottom

Enhanced TeamCity Logging #2111

Merged
merged 12 commits into from
Sep 27, 2018
Merged

Enhanced TeamCity Logging #2111

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
701e46c
Make `traceImportant` produce a warning in TeamCity.
otto-gebb Sep 26, 2018
d703d14
Merge remote-tracking branch 'otto-gebb/feature/fix-teamcity-warning'…
BlythMeister Sep 27, 2018
0ba928c
Error messages are sent to TeamCity with status ERROR
BlythMeister Sep 27, 2018
5a5b11d
When a closing tag is "Failed" report build status failure
BlythMeister Sep 27, 2018
122b966
TeamCity reportBuildStatus correctly
BlythMeister Sep 27, 2018
77df0e0
Targets setBuildState a part of writing report
BlythMeister Sep 27, 2018
de7ffff
Update trace listener docs to show the reasoning behind the tags chosen
BlythMeister Sep 27, 2018
7cf3b14
Correct documentation
BlythMeister Sep 27, 2018
940faff
Correct types
BlythMeister Sep 27, 2018
1e082e3
Compilation fix
BlythMeister Sep 27, 2018
55f78d0
Merge remote-tracking branch 'origin/release/next' into TeamCityLogging
BlythMeister Sep 27, 2018
1f88af9
Update docs for TeamCity
BlythMeister Sep 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 20 additions & 6 deletions src/app/Fake.BuildServer.TeamCity/TeamCity.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,14 @@ module TeamCity =
(TeamCityWriter.encapsulateSpecialChars name) (TeamCityWriter.encapsulateSpecialChars message) (TeamCityWriter.encapsulateSpecialChars details)
(TeamCityWriter.encapsulateSpecialChars expected) (TeamCityWriter.encapsulateSpecialChars actual) |> TeamCityWriter.sendStrToTeamCity

/// Sends a warning message.
let internal warning message =
TeamCityWriter.sendToTeamCity "##teamcity[message text='%s' status='WARNING']" message

/// Sends an error message.
let internal error message =
TeamCityWriter.sendToTeamCity "##teamcity[message text='%s' status='ERROR']" message

/// TeamCity build parameters
///
/// See [Predefined Build Parameters documentation](https://confluence.jetbrains.com/display/TCD18/Predefined+Build+Parameters) for more information
Expand Down Expand Up @@ -361,9 +369,8 @@ module TeamCity =
with get() = RecentlyFailedTests.cache.Value

/// Implements a TraceListener for TeamCity build servers.
/// ## Parameters
/// - `importantMessagesToStdErr` - Defines whether to trace important messages to StdErr.
/// - `colorMap` - A function which maps TracePriorities to ConsoleColors.
///
/// See [the documentation](https://confluence.jetbrains.com/display/TCD18/Build+Script+Interaction+with+TeamCity) for more information
BlythMeister marked this conversation as resolved.
Show resolved Hide resolved
type internal TeamCityTraceListener() =

interface ITraceListener with
Expand All @@ -382,8 +389,10 @@ module TeamCity =
testFailed testName message detail
| TraceData.TestStatus (testName,TestStatus.Failed(message, detail, Some (expected, actual))) ->
comparisonFailure testName message detail expected actual
| TraceData.BuildState state when state = TagStatus.Success ->
reportBuildStatus "SUCCESS" "{build.status.text}"
| TraceData.BuildState state ->
ConsoleWriter.write false color true (sprintf "Changing BuildState to: %A" state)
reportBuildStatus "FAILURE" (sprintf "%o - {build.status.text}" state)
| TraceData.CloseTag (KnownTags.Test name, time, _) ->
finishTestCase name time
| TraceData.OpenTag (KnownTags.TestSuite name, _) ->
Expand All @@ -394,10 +403,15 @@ module TeamCity =
match description with
| Some d -> TeamCityWriter.sendOpenBlock tag.Name (sprintf "%s: %s" tag.Type d)
| _ -> TeamCityWriter.sendOpenBlock tag.Name tag.Type
| TraceData.CloseTag (tag, _, status) when status = TagStatus.Failed ->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

| TraceData.CloseTag (tag, _, TagStatus.Failed)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait...that's a thing...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Called "pattern matching" ;)

TeamCityWriter.sendCloseBlock tag.Name
reportBuildStatus "FAILURE" (sprintf "Failure in %s" tag.Name)
| TraceData.CloseTag (tag, _, _) ->
TeamCityWriter.sendCloseBlock tag.Name
| TraceData.ImportantMessage text | TraceData.ErrorMessage text ->
ConsoleWriter.write false color true text
| TraceData.ImportantMessage text ->
warning text
| TraceData.ErrorMessage text ->
error text
| TraceData.LogMessage(text, newLine) | TraceData.TraceMessage(text, newLine) ->
ConsoleWriter.write false color newLine text
| TraceData.ImportData (ImportData.BuildArtifactWithName _, path)
Expand Down
6 changes: 5 additions & 1 deletion src/app/Fake.Core.Target/Target.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,11 +444,15 @@ module Target =
| Some e -> alignedError name time e.Message)

aligned "Total:" total null
if not context.HasError then aligned "Status:" "Ok" null
if not context.HasError then
aligned "Status:" "Ok" null
Trace.setBuildState TagStatus.Success
else
alignedError "Status:" "Failure" null
Trace.setBuildState TagStatus.Failed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BlythMeister Is there any particular reason why we need that? I'm asking because I just realized (after trying to figure out for hours which commit is to blame) that this makes the VSTS build red. This is because we have some testing going on with negative/failing cases which will call this API and make the build red.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason is so that when running on a CI (like teamcity) when the build fails the state is set to failure.
It's to try and prevent the build failure ending with "Process returned exit code 1"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But shouldn't that Process returned exit code 1 already be solved by using the error apis?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

possibly...happy to revert this bit of the change and try

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does mean though that any listener with "TraceData.BuildState" is a bit pointless as nothing ever traces with that...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about we make it so there is a new run or update existing runs to return the TargetContext from runInternal
That way, as a user you can make the decision to react to the overall run result...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(facepalm)....a bit like runAndGetContext

I'll shut up :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

match context.PreviousTargets.Length, context.HasError with
| 0, _ -> Trace.setBuildState TagStatus.Warning
| _, true -> Trace.setBuildState TagStatus.Failed
| _, _ -> Trace.setBuildState TagStatus.Success

in my script after calling runAndGetContext means it does what i want :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I should have been a bit more conservative with these changes... Found another edge case where the build went red in teamcity (good that we have a teamcity build now) so I need a bit more time to release this properly.

Regarding your suggestion. Yes indeed we could make that an opt-in api:

let context = Target.tryRunOrDefaultAndGetContext "Default"
Target.reportBuildStateFromContext context

(Or something along those lines)

Can you open a PR or an issue for this (so it is not forgotten in this PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take a look next week and ensure that all run methods also have a WithContext
and add a function to report status based on context which you could (and I will) pipe the run into.
Watch this space 😂

else
Trace.traceError "No target was successfully completed"
Trace.setBuildState TagStatus.Warning

Trace.traceLine()

Expand Down