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

TL: Fix logs appearance from nuget client/credential provider #9407

Conversation

YuliiaKovalova
Copy link
Member

@YuliiaKovalova YuliiaKovalova commented Nov 8, 2023

Fixes #9068

Context

Before this change we were showing specific messages in TL that matched a selected pattern + all the build warnings were displayed at the final build stage.

Changes Made

Introduce immediate print functionality for blocking messages from nuget client/credential provider

Testing

Manual + UTs

Notes

The implementation will be reconsidered after introducing TL API

src/MSBuild/TerminalLogger/TerminalLogger.cs Outdated Show resolved Hide resolved
src/MSBuild/TerminalLogger/TerminalLogger.cs Outdated Show resolved Hide resolved
src/MSBuild/TerminalLogger/TerminalLogger.cs Outdated Show resolved Hide resolved
Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

Looking good, just a couple more tweaks!

src/MSBuild/TerminalLogger/TerminalLogger.cs Show resolved Hide resolved
src/MSBuild/TerminalLogger/TerminalLogger.cs Outdated Show resolved Hide resolved
src/MSBuild/TerminalLogger/TerminalLogger.cs Outdated Show resolved Hide resolved
src/MSBuild.UnitTests/TerminalLogger_Tests.cs Outdated Show resolved Hide resolved
Copy link
Member

@ladipro ladipro left a comment

Choose a reason for hiding this comment

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

Looks great, thank you. From UX perspective I wonder if the output shouldn't contain the name of the project because it looks like we're currently printing just the bare message which is going to land at a random line. It may be mistakenly attributed to the last project that finished building.
cc @baronfel

@YuliiaKovalova
Copy link
Member Author

YuliiaKovalova commented Nov 15, 2023

Looks great, thank you. From UX perspective I wonder if the output shouldn't contain the name of the project because it looks like we're currently printing just the bare message which is going to land at a random line. It may be mistakenly attributed to the last project that finished building. cc @baronfel

I see what you mean, but just for illustration:
The example of output for DeviceFlow
image
and warning message
image

image

Since it's a part of the Restore target that contains the ProjectName, I am not fully confident we need to include this information in the immediate messages.

@ladipro
Copy link
Member

ladipro commented Nov 15, 2023

I see, it's always restore-only. In that case, please ignore me!

@YuliiaKovalova
Copy link
Member Author

I see, it's always restore-only. In that case, please ignore me!

Let's wait for @baronfel anyway :)

@baronfel
Copy link
Member

I agree with @YuliiaKovalova's finding here 👍

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.

TerminalLogger hides NuGet device-flow prompts
4 participants