-
Notifications
You must be signed in to change notification settings - Fork 467
Add debug-level log option for ctk-installer #1551
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Jactor Sue <jactor_sue@live.com>
|
|
||
| var availableRuntimes = map[string]struct{}{"docker": {}, "crio": {}, "containerd": {}} | ||
| var defaultLowLevelRuntimes = []string{"runc", "crun"} | ||
| var ( |
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.
If you're going to make these changes please make them in a separate commit.
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.
Nothing changed here, auto-formated by the gofumpt formatter.
Signed-off-by: Evan Lezar <elezar@nvidia.com>
elezar
left a comment
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 for the contribution.
I have made a proposal for using log-verbosity instead as well as some other changes that better align with some things that I've been considering recently.
Please feel free to squash / clean up as required.
| Usage: "enable debug-level log", | ||
| Destination: &options.debug, | ||
| Sources: cli.EnvVars("TOOLKIT_DEBUG"), | ||
| }, |
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.
One thing that we have been considering is switching to an explicit verbosity-based approach for this component so as to prepare for a switch to klog at some point. Could we changes this to:
| }, | |
| &cli.IntFlag{ | |
| Name: "log-versbosity", | |
| Aliases: []string{"v"}, | |
| Destination: &options.logVerbosity, | |
| Sources: cli.EnvVars("LOG_VERBOSITY"), | |
| }, |
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.
So you want to change logger from logrus to klog for whole toolkit stack or only for installer?
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.
The idea would be to move to something like logr.Logger internally, and then use klog in the installer and logrus for the CLI components. This may be longer term though, so a simpler change may be to ensure that the installer can have debug logging enabled as per your original change.
Deprecating the --debug flag, or mapping it to a well-defined log-verbosity at some point in the future would also be possible.
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.
(feel free to rebase and drop my commit so that I can review this).
Enable debug-level log for nvidia-ctk-installer cmd by
--debugor-d.