-
Notifications
You must be signed in to change notification settings - Fork 20.4k
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
cmd/utils: don't enumerate usb when --usb isn't set #22130
Conversation
cmd/utils/flags.go
Outdated
if ctx.GlobalIsSet(USBFlag.Name) { | ||
cfg.NoUSB = !ctx.GlobalBool(USBFlag.Name) | ||
} | ||
cfg.NoUSB = !ctx.GlobalIsSet(USBFlag.Name) || !ctx.GlobalBool(USBFlag.Name) |
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.
I think this is not right and it will break the config file.
What needs to happen is: the default of NoUSB in the struct needs to be made true
.
Then, if the flag is set, it needs to be changed to false
.
If someone has NoUSB = false
in the config file, it should still work.
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.
Since it doesn't seem to set default values in the deserializer, the default has to be set either before or after deserializing. How do you propose to distinguish the case when NoUSB
is absent from the TOML file, from the case when the user explicitly set NoUSB = false
?
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.
It is very unlikely that people will want to set NoUSB = false
because until now, NoUSB = false
was the default and now if they want to enable USB by default they don't have an option for that yet. What I suggest is also add a USB
command in the config that will default to false
, and that people can set to true
.
@fjl I iintroduced a |
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.
LGTM
I think there are a few more places you could clean up.
|
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.
LGTM
USB enumeration still occured. Make sure it will only occur if --usb is set. This also deprecates the 'NoUSB' config file option in favor of a new option 'USB'.
USB enumeration still occured. Make sure it will only occur if
--usb
is set.