Ensure we log into logrus on command error

[mlaventure]

urfave/cli now takes upon itself to log the error returned by the
command action directly. This means that by default the --log option
was ignored upon error.

This commit ensure that urfave/cli.ErrWriter will use logrus

Signed-off-by: Kenfe-Mickael Laventure mickael.laventure@gmail.com

[mlaventure]

So, urfave/cli also print the normal error message via ErrWriter, unfortunately there is no way to make a distinction between the provenance of the error from the io.Writer interface.

I've gotta go for the day, but I'll look at it tomorrow, if someone has an idea on how to handle both, let me know :)

[mlaventure]

I just realized, since this is only called on error from the cli, I can just also output it to stderr :)

[crosbymichael]

nit: instead of the global originalCliErrWriter add it to a field on FatalWriter

[mlaventure]

updated

On Fri, Sep 30, 2016 at 9:32 AM Michael Crosby notifications@github.com
wrote:

@crosbymichael commented on this pull request.

In main.go
#1089 (review)
:

@@ -129,7 +132,22 @@ func main() {
}
return nil
}

• // If the command returns an error, cli takes upon itself to print
• // the error on cli.ErrWriter and exit.
• // Use our own writer here to ensure the log gets sent to the right location.
• originalCliErrWriter = cli.ErrWriter
• cli.ErrWriter = &FatalWriter{}

nit: instead of the global originalCliErrWriter add it to a field on
FatalWriter

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#1089 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/APUifvmghyrQObp9gwylp__08ip-T2hxks5qvTmHgaJpZM4KKhz7
.

[crosbymichael]

LGTM

[mlaventure]

ping @cyphar @mrunalp

It's a small change, but it's preventing us to vendor a newer version of runc into docker :)

[cyphar]

This should just be f.cliErrWriter.Write(p) IMO. Why go through Fprintf (which in languages like C would be punished with arbitrary code execution 😉).

[cyphar]

In other words just make the code:

logrus.Error(string(p))
return f.cliErrWriter.Write(p)

[mlaventure]

Yep, totally right. Update incoming, thanks.

[cyphar]

Are you intentionally ignoring the error / actual number of bytes written?

[mlaventure]

I was initially because logrus doesn't return it. But I guess it makes sense to return the value from the cli writer

[cyphar]

LGTM.

[mrunalp]

LGTM