-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat: Pass through debug logging level to vfkit #26897
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
Move code to only call `logrus.IsLevelEnabled` once Move the `endpointArgs` append directly under it's definition Signed-off-by: Lewis Roy <[email protected]>
Signed-off-by: Lewis Roy <[email protected]>
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, LGTM
/approve
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Honny1, ninja-quokka The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
cmd.Args = append(cmd.Args, debugDevArgs...) | ||
cmd.Args = append(cmd.Args, "--gui") // add command line switch to pop the gui open | ||
|
||
cmd.Args = append(cmd.Args, "--log-level", "debug") // Pass through debug logging if enabled |
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.
This will break krunkit. It doesn't have a --log-level
option, rather it has --krun-log-level
. Krunkit also doesn't accept a string input, it accepts integer values, where --krun-log-level=4
would set the log level to debug.
If you want to do this for only vfkit you'll have to check the provider before you adjust the cmdline.
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.
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.
perhaps you could create a provider specific call GetDebugDevices
that calls GetDebugDevicesCMDArgs
or some such. And then the provider specific call could add the debug information itself. Then this will work for both krun and vfkit?
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.
Ah great, thanks guys. I incorrectly assumed the cli args were the same between krunkit and vfkit! I'll rework this patch.
Does this PR introduce a user-facing change?