You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
In #485, @stevemar, @ctreatma, and I discussed that while the precedent in metal-cli is for bool's to have --name=<true|false>, there is at least one exception with --app vs --sms for TFA.
We also discussed that in places where --name=false is used, we may not be handling it properly, where false values must be explicitly sent in API calls. These parameters are confusing to specify, and with Cobra, it is easy to miss incorrect handling during code review.
Metal CLI should adopt more obvious patterns for bools, such as named pairs (lock, unlock) or --no- prefixes.
It would be great to get these improvements from upstream project(s), but it can be implemented locally:
Is boolean the decided direction? I wouldn't be against a string like @ctreatmamentioned. I assume something like -x lock and -x unlock which feels intuitive.
Last month, Talos Linux spent almost an hour trying to figure out the iPXE boolean on Equinix Metal (here @ 51:52). However, it mustn't have been deployed as true during deployment, so they had to switch it inside the console.
In #485, @stevemar, @ctreatma, and I discussed that while the precedent in
metal-cli
is for bool's to have--name=<true|false>
, there is at least one exception with--app
vs--sms
for TFA.We also discussed that in places where
--name=false
is used, we may not be handling it properly, where false values must be explicitly sent in API calls. These parameters are confusing to specify, and with Cobra, it is easy to miss incorrect handling during code review.Metal CLI should adopt more obvious patterns for bools, such as named pairs (
lock
,unlock
) or--no-
prefixes.It would be great to get these improvements from upstream project(s), but it can be implemented locally:
no-
prefix spf13/cobra#958NoOptDefVal
, no--foo=true
syntax. spf13/pflag#214Originally posted by @displague in #485 (comment)
The text was updated successfully, but these errors were encountered: