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

Flag default value is overwritten by environment in help output. #1206

Closed
r10r opened this issue Nov 20, 2020 · 17 comments · Fixed by #1528
Closed

Flag default value is overwritten by environment in help output. #1206

r10r opened this issue Nov 20, 2020 · 17 comments · Fixed by #1528
Labels
area/v2 relates to / is being considered for v2 help wanted please help if you can! kind/bug describes or fixes a bug status/confirmed confirmed to be valid, but work has yet to start
Milestone

Comments

@r10r
Copy link

r10r commented Nov 20, 2020

my urfave/cli version is

v2.3.0

Describe the bug

Flag default value should never change in help output.

To reproduce

Build the following program against v2.3.0.

package main

import (
	"github.com/urfave/cli/v2"
	"os"
)

func main() {
	app := cli.NewApp()
	app.Name = "myapp"
	app.Flags = []cli.Flag{
		&cli.StringFlag{
			Name:        "myflag",
			Usage:       "my flag is my flag",
			EnvVars:     []string{"MYFLAG"},
			Value:       "defval",
		},
  }
  app.Run(os.Args)
}

Observed behavior

What did you do?

MYFLAG=foo ./app --help

What did you see happen immediately after the reproduction steps above?

NAME:
   myapp - A new cli application

USAGE:
   app [global options] command [command options] [arguments...]

COMMANDS:
   help, h  Shows a list of commands or help for one command

GLOBAL OPTIONS:
   --myflag value  my flag is my flag (default: "foo") [$MYFLAG]
   --help, -h      show help (default: false)

What did you expect to see?

[...]
GLOBAL OPTIONS:
   --myflag value  my flag is my flag (default: "defval") [$MYFLAG]
   --help, -h      show help (default: false)

Expected behavior

Default value should never change (in help output).

@r10r r10r added area/v2 relates to / is being considered for v2 kind/bug describes or fixes a bug status/triage maintainers still need to look into this labels Nov 20, 2020
@r10r
Copy link
Author

r10r commented Nov 20, 2020

Maybe related to #1181

@r10r
Copy link
Author

r10r commented Jan 12, 2021

ping

@vipally
Copy link
Contributor

vipally commented Feb 7, 2021

The default value can changes by Env value or config file, it is the design, not bug.
Refers to the doc
I don't know why no body deals with the issues and PRs.

@r10r
Copy link
Author

r10r commented Feb 7, 2021

it is the design, not bug.

@vipally Your comment is kinda confusing. According to your pull requests you do consider this as a bug ?

@r10r
Copy link
Author

r10r commented Feb 9, 2021

Kindly ping @lynncyrin. Can someone of the devs please comment whether this is a bug or not? Thanks!

@coilysiren
Copy link
Member

👀

@coilysiren
Copy link
Member

I don't know why no body deals with the issues and PRs.

Because nobody is paid to pay attention to them 🙂

@coilysiren
Copy link
Member

I think this is unambiguously a bug, the EnvVars attribute isn't intended to mock the default value

@coilysiren coilysiren added help wanted please help if you can! status/confirmed confirmed to be valid, but work has yet to start and removed status/triage maintainers still need to look into this labels Feb 10, 2021
@coilysiren
Copy link
Member

If the docs are currently expecting this behavior, then the docs should be changed too.

I think this is a bug from a logical point of view, but I think it's also important to consider whether or not fixing this would be a breaking change... hopefully the bug here is only with the help text display.

@r10r
Copy link
Author

r10r commented Feb 10, 2021

👀

Thanks for looking in :D

@vipally What about closing the duplicate issues and continue discussion here ?

@vipally
Copy link
Contributor

vipally commented Feb 10, 2021

it is the design, not bug.

@vipally Your comment is kinda confusing. According to your pull requests you do consider this as a bug ?

In fact, my PR pays no attention on whether ENV values change the default.
I found some strange of the design, because flag.Value is the default value makes confuse.
In my sense, default value may named as Default but not Value.

@vipally
Copy link
Contributor

vipally commented Feb 10, 2021

👀

Thanks for looking in :D

@vipally What about closing the duplicate issues and continue discussion here ?

Do you mean #1235 ?
I consider them as different and same kind of issues, but not duplicate ones.
I found lots of unexpected behavior on slice filags.

@meatballhat meatballhat added this to the Release 2.5.0 milestone Apr 21, 2022
@meatballhat meatballhat changed the title v2 bug: Flag default value is overwritten by environment in help output. Flag default value is overwritten by environment in help output. Apr 21, 2022
@dearchap
Copy link
Contributor

@urfave/cli What do you all think ? We should change this to have defval instead of val from env for default ?

@abitrolly
Copy link
Contributor

Because nobody is paid to pay attention to them slightly_smiling_face

If there was the option, I would use it right away. :D

What do you all think ? We should change this to have defval instead of val from env for default ?

I think this

MYFLAG=foo ./app --help

Should give this

   --myflag value  my flag is my flag (default: "defval"), env:MYFLAG ("foo")

@julian7
Copy link
Contributor

julian7 commented Oct 10, 2022 via email

@abitrolly
Copy link
Contributor

@julian7 users who can run --help can as well read secret from environment variable. However, if one is doing live coding, this can be rather sensitive indeed. I would still leave a possibility to toggle env variable output for not so critical variables, and leave out showing the value by default.

@Dokiys
Copy link
Member

Dokiys commented Oct 12, 2022

@urfave/cli What do you all think ? We should change this to have defval instead of val from env for default ?

I think it's not necessary to output env variable. For example when run rm -rf $SOMEFILES , we should explicitly know what SOMEFILES is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/v2 relates to / is being considered for v2 help wanted please help if you can! kind/bug describes or fixes a bug status/confirmed confirmed to be valid, but work has yet to start
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants