Add log-level flag for skopeo #2514
Conversation
|
First time actually pushing a cobra change and making sure a global log level is applied to the containers org, would appreciate fact-checking to make sure I didn't break anything. |
Keep the debug flag for backwards compatibility, but support other log levels. Signed-off-by: Robb Manes <rmanes@redhat.com>
robbmanes
force-pushed
the
add-log-level-arg
branch
from
0556e50 to
8d9ca07
Compare
mtrmac
left a comment
mtrmac
left a comment
There was a problem hiding this comment.
Thanks! Yes, having this option makes sense.
skopeo.1.mdneeds updating- Non-blocking: Unit tests for the log level parsing (perhaps split from
.beforeinto a smaller function?), covering (--log-level={valid,invalid},--debug, both set, neither) would be nice.
|
|
||
| type globalOptions struct { | ||
| debug bool // Enable debug output | ||
| logLevel string // Set log level at or above debug (trace, etc) |
There was a problem hiding this comment.
The comment is incorrect – --log-level=fatal would also work.
| var dummyVersion bool | ||
| rootCommand.Flags().BoolVarP(&dummyVersion, "version", "v", false, "Version for Skopeo") | ||
| rootCommand.PersistentFlags().BoolVar(&opts.debug, "debug", false, "enable debug output") | ||
| rootCommand.PersistentFlags().StringVar(&opts.logLevel, "log-level", defaultLogLevel, fmt.Sprintf("Log messages above specified level (%s)", strings.Join(logLevels, ", "))) |
There was a problem hiding this comment.
Contrary to Podman’s practice, I think it makes much more sense to not set default option values. With option values defaulted at the CLI level, the code later has difficulty telling apart actual user input from the hard-coded defaults (or from values that come from config files).
| var gitCommit = "" | ||
|
|
||
| // Supported logrus log levels | ||
| var logLevels = []string{"trace", "debug", "info", "warn", "warning", "error", "fatal", "panic"} |
There was a problem hiding this comment.
This should be built from logrus.AllLevels, not hard-coded in Skopeo.
There was a problem hiding this comment.
… second thought, all of logrus is deprecated. Nowadays the standard-library logging defines fewer levels: https://pkg.go.dev/log/slog#Level .
We probably do want “trace”; but maybe we can avoid committing to fatal/panic for now?
|
|
||
| // Supported logrus log levels | ||
| var logLevels = []string{"trace", "debug", "info", "warn", "warning", "error", "fatal", "panic"} | ||
| var defaultLogLevel = "warn" |
There was a problem hiding this comment.
The default of logrus is actually info, but, anyway, see elsewhere, this should not need to exist.
| var found bool | ||
| for _, l := range logLevels { | ||
| if l == strings.ToLower(logLevel) { | ||
| found = true | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if !found { | ||
| logrus.Fatal("Log Level %s is not supported, choose from: %s\n", logLevel, strings.Join(logLevels, ", ")) | ||
| } | ||
|
|
There was a problem hiding this comment.
This is unnecessary, logrus.ParseLevel already fails if the input is invalid.
|
|
||
| logrus.SetLevel(level) | ||
| if logrus.IsLevelEnabled(logrus.InfoLevel) { | ||
| logrus.Infof("Filtering at log level %s", logrus.GetLevel()) |
There was a problem hiding this comment.
I’d prefer not starting every command output with such a line. (“info” is the default level).
- There’s the UNIX tradition of “in simple cases, on success, print nothing”.
- Some (mistaken?) users fail callers if anything has been printed to
stderr - Anyway, longer scripts that include calls to Skopeo would probably want to collect full
stderroutput, and having that contain lines like this (with no context and possibly no other output following) seems unclean to me.
With a strict enough parsing, the users should be confident that the output exactly matches their expectations. We don’t have ls dir confirming “yes, I am reading dir”.
| } | ||
|
|
||
| logrus.SetLevel(level) | ||
| if logrus.IsLevelEnabled(logrus.InfoLevel) { |
There was a problem hiding this comment.
Infof already includes an IsLevelEnabled check; this is optimizing out only the GetLevel call, a single atomic load. I think that’s not worth the extra code.
|
|
||
| // before is run by the cli package for any command, before running the command-specific handler. | ||
| func (opts *globalOptions) before(cmd *cobra.Command, args []string) error { | ||
| // Set logging level based on debug or logLevel flags |
There was a problem hiding this comment.
Using both --log-level and --debug should fail because the user has probably made a mistake.
One way to do that would be to use NewOptionalStringValue.
|
A friendly reminder that this PR had no activity for 30 days. |
|
@robbmanes have you had a chance to address @mtrmac 's concerns? |
|
A friendly reminder that this PR had no activity for 30 days. |
3 participants
PR containers/image#2713 should really be type
Tracefrather thanDebugf, butskopeodoesn't support any log level except what the--debugflag offers.More-or-less copy the
--log-levelbehavior frompodmanfor compatibility and this should allow forTracelogruslevels to work.Keep the debug flag for backwards compatibility as well and it make it mutually exclusive with
--debug.