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.

Sign up for GitHub

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

Add log-level flag for skopeo #2514

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add log-level flag for skopeo #2514

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 33 additions & 1 deletion cmd/skopeo/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,15 @@ import (
// and will be populated by the Makefile
var gitCommit = ""

// Supported logrus log levels
var logLevels = []string{"trace", "debug", "info", "warn", "warning", "error", "fatal", "panic"}
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be built from logrus.AllLevels, not hard-coded in Skopeo.

Copy link
Contributor

Choose a reason for hiding this comment

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

… 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?

var defaultLogLevel = "warn"
Copy link
Contributor

Choose a reason for hiding this comment

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

The default of logrus is actually info, but, anyway, see elsewhere, this should not need to exist.


var defaultUserAgent = "skopeo/" + version.Version

type globalOptions struct {
debug bool // Enable debug output
logLevel string // Set log level at or above debug (trace, etc)
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment is incorrect – --log-level=fatal would also work.

tlsVerify commonFlag.OptionalBool // Require HTTPS and verify certificates (for docker: and docker-daemon:)
policyPath string // Path to a signature verification policy file
insecurePolicy bool // Use an "allow everything" signature verification policy
Expand Down Expand Up @@ -79,6 +84,7 @@ func createApp() (*cobra.Command, *globalOptions) {
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, ", ")))
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

rootCommand.PersistentFlags().StringVar(&opts.policyPath, "policy", "", "Path to a trust policy file")
rootCommand.PersistentFlags().BoolVar(&opts.insecurePolicy, "insecure-policy", false, "run the tool without any policy check")
rootCommand.PersistentFlags().StringVar(&opts.registriesDirPath, "registries.d", "", "use registry configuration files in `DIR` (e.g. for container signature storage)")
Expand All @@ -93,6 +99,7 @@ func createApp() (*cobra.Command, *globalOptions) {
rootCommand.PersistentFlags().StringVar(&opts.tmpDir, "tmpdir", "", "directory used to store temporary files")
flag := commonFlag.OptionalBoolFlag(rootCommand.Flags(), &opts.tlsVerify, "tls-verify", "Require HTTPS and verify certificates when accessing the registry")
flag.Hidden = true
rootCommand.MarkFlagsMutuallyExclusive("log-level", "debug")
rootCommand.AddCommand(
copyCmd(&opts),
deleteCmd(&opts),
Expand All @@ -114,9 +121,34 @@ func createApp() (*cobra.Command, *globalOptions) {

// 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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

logLevel := opts.logLevel
if opts.debug {
logrus.SetLevel(logrus.DebugLevel)
logLevel = "debug"
}

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, ", "))
}

Comment on lines +130 to +141
Copy link
Contributor

Choose a reason for hiding this comment

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

This is unnecessary, logrus.ParseLevel already fails if the input is invalid.

level, err := logrus.ParseLevel(logLevel)
if err != nil {
logrus.Fatal(err.Error())
}

logrus.SetLevel(level)
if logrus.IsLevelEnabled(logrus.InfoLevel) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

logrus.Infof("Filtering at log level %s", logrus.GetLevel())
Copy link
Contributor

Choose a reason for hiding this comment

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

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 stderr output, 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”.

}

if opts.tlsVerify.Present() {
logrus.Warn("'--tls-verify' is deprecated, please set this on the specific subcommand")
}
Expand Down