Skip to content
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

Context.IsSet not working with values set from environment variables #294

Closed
varun06 opened this issue Nov 12, 2015 · 9 comments
Closed

Context.IsSet not working with values set from environment variables #294

varun06 opened this issue Nov 12, 2015 · 9 comments
Assignees
Labels

Comments

@varun06
Copy link

@varun06 varun06 commented Nov 12, 2015

I used IsSet() to check for some values. It works the string flag is passed from command line but not when the flag values are set with environment values.

@jszwedko jszwedko added the type: bug label Nov 24, 2015
@jszwedko

This comment has been minimized.

Copy link
Collaborator

@jszwedko jszwedko commented Nov 24, 2015

Indeed, this appears to be true, thanks for raising the issue @varun06 !

@AaronO

This comment has been minimized.

Copy link

@AaronO AaronO commented Mar 16, 2016

@jszwedko I just encountered this bug too. Any update on this ?

@jszwedko

This comment has been minimized.

Copy link
Collaborator

@jszwedko jszwedko commented Mar 20, 2016

@AaronO unfortunately I haven't had the time to address this yet, but PRs are welcome!

@bweston92

This comment has been minimized.

Copy link

@bweston92 bweston92 commented Apr 4, 2016

Also just come across this, I'm a Go newb so my PR wouldn't look good.

@meatballhat meatballhat self-assigned this May 17, 2016
@meatballhat

This comment has been minimized.

Copy link
Member

@meatballhat meatballhat commented May 17, 2016

tl;dr - this is not straightforward, and a proper solution may require reimplementing a lot of stuff

My first adventures with this have me hitting some unpleasantness. A travelogue, all prefixed with a strong AFAICT :

cli.Context.IsSet accepts a string, which is currently used to look up a matching stdlib *flag.Flag via flag.FlagSet.Visit, which only calls the given func on flags that have been set via stdlib flag parsing (not env vars). There is not currently a mapping of stdlib *flag.Flag to cli.Flag available to a given context, so getting at the right env vars for a given flag when all we have is one of the names it uses isn't straightforward. The guts of altsrc/flag.go include a method for checking via both cli.Context.IsSet and directly looking at the env via os.Getenv, but that's because at that level it's accessing a full cli.Flag.

I started down the path of building a name ➡️ cli.Flag mapping inside *cli.Context, but there's issues there as well. For example, the cli.Context.Command is not set during cli.NewContext, but assigned during cli.Command.Run, which means that we'd have to update the index at assignment time, namespacing the keys of whatever index to distinguish between flags from cli.Context.Command and cli.Context.App. Then there's the issues of avoiding index staleness, handling potential Command or App reassignment (they're public, after all,) and any bugs lurking in the fact that throughout the library is a mix of struct pointers and struct copies.

@varun06

This comment has been minimized.

Copy link
Author

@varun06 varun06 commented May 17, 2016

Thanks for looking into it.

@jszwedko

This comment has been minimized.

Copy link
Collaborator

@jszwedko jszwedko commented Sep 18, 2016

This is fixed by #502 (some additional, related, work is being done in #530 to address #517).

@jszwedko jszwedko closed this Sep 18, 2016
@sarahhodne

This comment has been minimized.

Copy link

@sarahhodne sarahhodne commented Feb 14, 2017

This still/again appears to be broken, c.IsSet("foo") returns false, but c.String("foo") returns a value with the following flag:

&cli.StringFlag{
    Name: "foo",
    EnvVars: []string{"FOO"},
}

Using urfave/cli e485446 (v2).

@jszwedko

This comment has been minimized.

Copy link
Collaborator

@jszwedko jszwedko commented Feb 15, 2017

@henrikhodne aha, yep, this wasn't ported correctly when I merged these changes into the v2 branch, thank you for bringing this up. Opened #597 to correct.

ernoaapa added a commit to ernoaapa/eliot that referenced this issue Dec 19, 2017
Command `eli create -f somefile.yml`, returned error that
must define either --file or --image. This because the
clicontext.IsSet() were returning false if using shorthand aliases.
There is fix coming in urfave/cli v2.0:
urfave/cli#294

But actually we don't need to use that in our use cases so removed
the extra IsSet checks.
ernoaapa added a commit to ernoaapa/eliot that referenced this issue Dec 19, 2017
Command `eli create -f somefile.yml`, returned error that
must define either --file or --image. This because the
clicontext.IsSet() were returning false if using shorthand aliases.
There is fix coming in urfave/cli v2.0:
urfave/cli#294

But actually we don't need to use that in our use cases so removed
the extra IsSet checks.
asahasrabuddhe added a commit that referenced this issue Sep 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.