cli
cli copied to clipboard
urfave
Unable to explicitly set flag to 'zero' value using altsrc
The following check prevents one from explicitly setting a bool flag to false when using an altsrc input:
https://github.com/urfave/cli/blob/c11460548df8d468af55d678ccccf50e970e71b1/altsrc/flag.go#L129
This can cause issues (for example) if the default value from the command line is true and you want to explicitly set the flag to false in a config file.
This is not just true for bools (as the link points to), but other flag types as well
(most notably string with its check on "").
The same too but it took me a while to figure out where my bug came from 😅 Is there any plan to fix this?
@klueska sorry for my long delay! Given some recent related work by @joeycumines at the top-level package, I'd love to see if there's a solution to altsrc/ that can follow the same rules. @joeycumines WDYT?
@klueska sorry for my long delay! Given some recent related work by @joeycumines at the top-level package, I'd love to see if there's a solution to
altsrc/that can follow the same rules. @joeycumines WDYT?
@meatballhat I am probably lacking some context, but this appears to be an issue stemming from the use of a default value guard, to check if values are set. It seems to me that following through on https://github.com/urfave/cli/pull/1376, and removing the guards for all cases (as I think @klueska and @sjp00556 are suggesting) would resolve this issue.
Side note, #1376 is a breaking change. I'd have instead used an error as a sentinel, to indicate not set. This would avoid implementers of altsrc.InputSourceContext needing to add a new method ~(preserving backwards compatibility)~, although it would require updating urfave/cli in tandem with updating input sources with the new behavior.
EDIT: It's more complicated than I thought, see below https://github.com/urfave/cli/issues/1395#issuecomment-1159608895.
While I'm on the topic, I'd suggest establishing clear (and ideally consistent) rules regarding how the different input sources are combined. Notably, in some cases the "is set" check for environment variables include similar guards, checking against an empty string. In #1409, I opted to keep this behavior for Float64SliceFlag, and added it to Int64SliceFlag and IntSliceFlag. The reason I felt comfortable doing this was due to the env var being the lowest priority, and the zero string case always being an error ("" fails to parse as any of those). If altsrc is lower priority than env vars, that may have been a mistake.
Idk what https://github.com/urfave/cli/blob/c11460548df8d468af55d678ccccf50e970e71b1/altsrc/flag.go#L226 was referring to, but it doesn't seem relevant to anything here.
Mmmm actually, addressing this issue (w/o a breaking change) is more difficult than I'd thought.
Regardless of the mechanism used to indicate "not set", removal of the default value checks will still (potentially) break input source implementations that don't support said mechanism. Even assuming altsrc is the lowest priority, this seems like it would effectively break default values.
Alternate solution: Leave the default value guards in, but only use them if the InputSourceContext doesn't implement interface { IsSet(name string) bool }, and remove isSet(name string) bool from InputSourceContext.
Disclaimer: I only just noticed the IsSet -> isSet change https://github.com/urfave/cli/commit/fe1468cc86b1cab489b2038a7f83090198b72af8 , but that doesn't change my previous conclusions.
@joeycumines I hope I'm understanding you correctly, but making altsrc lower precedence than the default defined in code would be different ordering than is claimed in the documentation:
https://cli.urfave.org/v2/#precedence
In any case, I think I'm in favor of:
Alternate solution: Leave the default value guards in, but only use them if the InputSourceContext doesn't implement interface { IsSet(name string) bool }, and remove isSet(name string) bool from InputSourceContext.
My bad, when I said
Even assuming altsrc is the lowest priority, this seems like it would effectively break default values.
I wasn't including default. Sorry, that was unclear.
My bad, when I said
Even assuming altsrc is the lowest priority, this seems like it would effectively break default values.
I wasn't including default. Sorry, that was unclear.
No worries! Thank you!
@klueska Are you able to test with latest code ? There have been lots of fixes in that area since this issue was raised.
