Unable to explicitly set flag to 'zero' value using altsrc

[klueska]

The following check prevents one from explicitly setting a bool flag to false when using an altsrc input:

cli/altsrc/flag.go

Line 129 in c114605

if value {

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 "").

[sjp00556]

we meet the same issue. should drop if value { condition

[asiffer]

The same too but it took me a while to figure out where my bug came from 😅 Is there any plan to fix this?

[meatballhat]

@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?

[joeycumines]

@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 #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 #1395 (comment).

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

cli/altsrc/flag.go

Line 226 in c114605

// TODO: Can't use this for bools as

was referring to, but it doesn't seem relevant to anything here.

[joeycumines]

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 fe1468c , but that doesn't change my previous conclusions.

[meatballhat]

@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.

[joeycumines]

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.

[meatballhat]

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!

[dearchap]

@klueska Are you able to test with latest code ? There have been lots of fixes in that area since this issue was raised.

[dearchap]

Not present in latest releases.