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

Using altsrc + required flags on Command doesn't seem to work #1725

Closed
3 tasks done
zllovesuki opened this issue Apr 23, 2023 · 7 comments
Closed
3 tasks done

Using altsrc + required flags on Command doesn't seem to work #1725

zllovesuki opened this issue Apr 23, 2023 · 7 comments
Labels
area/v2 relates to / is being considered for v2 status/triage maintainers still need to look into this
Milestone

Comments

@zllovesuki
Copy link

My urfave/cli version is

v2.25.1

Checklist

  • Are you running the latest v2 release? The list of releases is here.
  • Did you check the manual for your release? The v2 manual is here
  • Did you perform a search about this problem? Here's the GitHub guide about searching.

Dependency Management

  • My project is using go modules.

Describe the bug

If the altsrc flag is required on the Command instead of the App, it will say that the required flag is not set

To reproduce

Example code:

func main() {
	Command := cli.Command{
		Name:      "command",
		ArgsUsage: " ",
		Flags: []cli.Flag{
			&cli.StringFlag{
				Name:    "config",
				Aliases: []string{"c"},
			},
			altsrc.NewStringFlag(&cli.StringFlag{
				Name:     "sup",
				Value:    "bleh",
				Required: true,
			}),
		},
		Before: func(ctx *cli.Context) error {
			loadFunc := altsrc.InitInputSourceWithContext(ctx.Command.Flags, altsrc.NewYamlSourceFromFlagFunc("config"))
			if err := loadFunc(ctx); err != nil {
				return err
			}
			return nil
		},
		Action: func(ctx *cli.Context) error {
			fmt.Printf("%+v", ctx.String("sup"))
			return nil
		},
	}
	App := cli.App{
		Name:            "rnc",
		HideHelpCommand: true,
		Flags: []cli.Flag{
			&cli.BoolFlag{
				Name:  "verbose",
				Value: false,
				Usage: "enable verbose logging",
			},
		},
		Commands: []*cli.Command{
			&Command,
		},
	}

	ctx, cancel := context.WithCancel(context.Background())
	defer cancel()

	if err := App.RunContext(ctx, os.Args); err != nil {
		fmt.Fprintln(os.Stderr, err)
	}
}

Observed behavior

cat sup.yaml
sup: altsrc
go run ./cmd/rnd command -c ./sup.yaml
NAME:
   rnc command

USAGE:
   rnc command [command options]

OPTIONS:
   --config value, -c value
   --sup value               (default: "bleh")
   --help, -h                show help
Required flag "sup" not set

Expected behavior

Flags parsed from file correctly

Run go version and paste its output here

go version go1.20.3 linux/amd64

Run go env and paste its output here

GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/rachel/.cache/go-build"
GOENV="/home/rachel/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/rachel/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/rachel/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.20.3"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/rachel/code/specter/go.mod"
GOWORK=""
CGO_CFLAGS="-O2 -g"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-O2 -g"
CGO_FFLAGS="-O2 -g"
CGO_LDFLAGS="-O2 -g"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build829362637=/tmp/go-build -gno-record-gcc-switches"
@zllovesuki zllovesuki 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 Apr 23, 2023
@tg123
Copy link

tg123 commented May 10, 2023

#849

@dearchap
Copy link
Contributor

@zllovesuki @tg123 The problem is the ordering. The required flags are checked prior to the "Before" handler running. @zllovesuki in your case you could have a Before handler in App to initialize the altsrc. That should fix your issue.

@zllovesuki
Copy link
Author

@dearchap if I do have the handler in Before to have handle altsrc in App, does that affect other Subcommands? Also, that means I need to specify the config file flag in the App but no Command, which may cause issues

@dearchap
Copy link
Contributor

When you say it affects subcommands it will if they define altsrc flags as well.

@meatballhat meatballhat added this to the Release 2.x milestone Jun 21, 2023
@zav8
Copy link

zav8 commented Nov 29, 2023

@zllovesuki @tg123 The problem is the ordering. The required flags are checked prior to the "Before" handler running. @zllovesuki in your case you could have a Before handler in App to initialize the altsrc. That should fix your issue.

Hi, I suppose the ordering refers to here, and wonder if reversing it would be reasonable.

Since if I put a main command in App.Action, then App.Flags would not be set from the config file before checkRequiredFlags(), so an errRequiredFlags error will be returned.

@dearchap
Copy link
Contributor

It might be fine but at this point v2 is strictly in maintenance mode. No feature updates or anything. We could consider this for v3. See #833

@dearchap dearchap removed the kind/bug describes or fixes a bug label Nov 29, 2023
@dearchap
Copy link
Contributor

Also #1273 . Will close this issue.

@dearchap dearchap closed this as not planned Won't fix, can't repro, duplicate, stale Dec 31, 2023
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 status/triage maintainers still need to look into this
Projects
None yet
Development

No branches or pull requests

5 participants