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

Enhanced TeamCity Logging #2111

Merged
merged 12 commits into from Sep 27, 2018
Merged

Conversation

BlythMeister
Copy link
Contributor

Description

Superseeds: #2109
Fixes #2108

Update TeamCity listener to match the documentation for service messages
Add functionality in the targets to report build state through trace

@@ -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" ;)

@BlythMeister
Copy link
Contributor Author

@matthid this all good now?

@@ -39,11 +39,13 @@ module TeamCityImportExtensions =
/// The general documentation on how to use CI server integration can be found [here](/buildserver.html).
/// This module does not provide any special APIs please use FAKE APIs and they should integrate into this CI server.
/// If some integration is not working as expected or you have features you would like to use directly please open an issue.
///
/// For more information on TeamCity interaction from builc scripts [see here](https://confluence.jetbrains.com/display/TCD18/Build+Script+Interaction+with+TeamCity)
Copy link
Member

Choose a reason for hiding this comment

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

I'd assume more important would be how to set the "fail on traceError flag" and how fake in particular interacts with TeamCity. I don't think just the link to teamcity docs helps fake users here.

I'd love to see another PR to improve the docs but I'm not blocking this one

@matthid matthid merged commit e4da873 into fsprojects:release/next Sep 27, 2018
@matthid matthid mentioned this pull request Sep 27, 2018
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 😂

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.

None yet

3 participants