-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Exec: take EchoOff into account when command fails #5962
Conversation
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.
This change looks great, thanks! I'm unsure about what the exit code-sensitive strings should look like, though. Printing The command "EchoOff" exited with code...
looks a little awkward to me, but it's good to note that the command wasn't printed...maybe The command exited with code {0}. (Command omitted because EchoOff was set to true.)
?
I think the parentheses at the end may easily go unnoticed. @Forgind do you like one of these?
|
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 didn't actually know that Exec
overrode the default ToolTask
behavior in this way
Lines 374 to 378 in 743c545
//Dont print the command line if Echo is Off. | |
if (!EchoOff) | |
{ | |
base.LogToolCommand(Command); | |
} |
Given that, I think this makes sense.
I favor the "***"
approach, or a new localized string like [Redacted because EchoOff was set]
. The former obviously being easier to implement :)
// Don't log when EchoOff and IgnoreExitCode. | ||
if (!EchoOff) | ||
{ | ||
Log.LogMessageFromResources(MessageImportance.Normal, "Exec.CommandFailedNoErrorCode", Command, ExitCode); |
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.
Hmm. Is it worth preserving this "exited with code" message if we use the scrubbed command from below? It may be useful information, depending on situation--if it's an "always fails with the same code, just go with it" thing you probably don't need that; if it's a "sometimes fails so this is just best effort" it might help chase down a later failure caused by the failure.
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 omitted the logging because it enables the user to take action on its own based on the output ExitCode of the Exec task.
This could be custom messages, or other a fail back action.
I'm fine changing this to use the "..." placeholder.
Or we can lower the importance when EchoOff (in addition to using the "...").
@rainersigwald what do you prefer?
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 think as-is is ok; we'll see if anyone complains.
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.
Thanks!
// Don't log when EchoOff and IgnoreExitCode. | ||
if (!EchoOff) | ||
{ | ||
Log.LogMessageFromResources(MessageImportance.Normal, "Exec.CommandFailedNoErrorCode", Command, ExitCode); |
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 think as-is is ok; we'll see if anyone complains.
Thanks, @tmds! |
@rainersigwald
EchoOff is documented to: