Add option to disable color in logs #416
Conversation
|
@PierreF Thanks for the PR! Yeah, since we cannot in place update the encoder I think we should define in For the subpackages (now only |
PierreF
force-pushed
the
disable-color
branch
from
e846a3e to
6ba9b46
Compare
PierreF
changed the title
|
Updated PR to have two loggers. By default the non-colored one and if condition are meet, replace logger with the colored one.
|
There was a problem hiding this comment.
@PierreF Thanks! I agree and think it's the correct logic (also thanks to pflags Changed() function). Just a small nit to move the common logic but if you see some problems doing this let's keep it as is.
Otherwise it LGTM!
P.S. Just noticed that semaphore output is still colored so it's acting as a tty.
cmd/keeper/keeper.go
Outdated
| @@ -115,6 +117,7 @@ func init() { | |||
| cmdKeeper.PersistentFlags().StringVar(&cfg.pgSUUsername, "pg-su-username", user, "postgres superuser user name. Used for keeper managed instance access and pg_rewind based synchronization. It'll be created on db initialization. Defaults to the name of the effective user running stolon-keeper. Must be the same for all keepers.") | |||
| cmdKeeper.PersistentFlags().StringVar(&cfg.pgSUPassword, "pg-su-password", "", "postgres superuser password. Only one of --pg-su-password or --pg-su-passwordfile must be provided. Must be the same for all keepers.") | |||
| cmdKeeper.PersistentFlags().StringVar(&cfg.pgSUPasswordFile, "pg-su-passwordfile", "", "postgres superuser password file. Only one of --pg-su-password or --pg-su-passwordfile must be provided. Must be the same for all keepers)") | |||
| cmdKeeper.PersistentFlags().BoolVar(&cfg.logColor, "log-color", false, "enable color in log output (default if attached to a terminal)") | |||
There was a problem hiding this comment.
Perhaps we could move this options and related logic inside the common command config: https://github.com/sorintlab/stolon/blob/master/cmd/common.go to avoid replicating it.
Signed-off-by: Pierre Fersing <pierre.fersing@bleemeo.com>
PierreF
force-pushed
the
disable-color
branch
from
6ba9b46 to
70464d3
Compare
|
@PierreF LGTM! Let me know if you have other pending changes or I can merge it. |
|
No, It's good for me. |
Add option to disable color in logs
|
@PierreF Thanks a lot! Merged. |
This PR will fix #415
It's not currently working (color are always disabled for now).
This issue is that I didn't find a way to pass option to pkg/log before logger is build. Logger initialization happens during init()/package import... while parsing option happens in main().
It does not seems to be dynamically updateable like is the level. To update the value, I need to build a new logger (which means that I would need to update all place where old logger is used).
I could think of the following way to solve this issue:
I'm not fan of either solutions that both looks hack. I'm open to better suggestion.