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

required parameters don't work with altsrc #849

Closed
costela opened this issue Aug 8, 2019 · 5 comments
Closed

required parameters don't work with altsrc #849

costela opened this issue Aug 8, 2019 · 5 comments
Assignees
Labels
kind/bug describes or fixes a bug status/confirmed confirmed to be valid, but work has yet to start

Comments

@costela
Copy link

costela commented Aug 8, 2019

Hi,

Unless I'm somehow overlooking something obvious, it seems the new Required support introduced in #819 doesn't work with altsrc.
The following code (based on the example for altsrc) complains that someflag wasn't supported if Required: true, but works if you comment that line out.

package main

import (
	"fmt"
	"log"
	"os"

	"github.com/urfave/cli"
	"github.com/urfave/cli/altsrc"
)

func main() {
	app := cli.NewApp()

	flags := []cli.Flag{
		altsrc.NewIntFlag(cli.IntFlag{
			Name:     "someflag",
			Required: true,
		}),
		cli.StringFlag{
			Name:  "file",
			Value: "test_cli_yaml.yaml",
		},
	}

	app.Action = func(c *cli.Context) error {
		fmt.Printf("test: %s\n", c.String("someflag"))
		return nil
	}

	app.Before = altsrc.InitInputSourceWithContext(flags, altsrc.NewYamlSourceFromFlagFunc("file"))
	app.Flags = flags

	err := app.Run(os.Args)
	if err != nil {
		log.Fatal(err)
	}
}

And the accompanying YAML file (test_cli_yaml.yaml):

---

someflag: 1

@lynncyrin, I suspect your question in #825 might be related to this, right?

@coilysiren coilysiren self-assigned this Aug 8, 2019
@coilysiren coilysiren added status/confirmed confirmed to be valid, but work has yet to start kind/bug describes or fixes a bug labels Aug 8, 2019
@coilysiren
Copy link
Member

Yeah I discovered after my required flags PR that "altsrc" is a completely different code path, so you'd essentially have to implement every feature twice. Once in main, and once in altsrc 😩

I'm, having some negatives feelings about the architecture of altsrc :/ I'm gonna investigate moving some code from altsrc to the main package, which would fix this.

@coilysiren
Copy link
Member

I'm making this as a bug since externally it's a bug. The PR to fix this is probably gonna look like a new feature implementation PR though

@costela
Copy link
Author

costela commented Aug 8, 2019

I'm gonna investigate moving some code from altsrc to the main package, which would fix this.

that sounds great! I was wondering myself why using altsrc feels so clunky 😉

@coilysiren
Copy link
Member

coilysiren commented Aug 17, 2019

After thinking on this a bunch more, I've come to the conclusion that I'm definitely going to fix this by adding all of the altsrc features into the main package, and removing altsrc entirely. My current idea is that that'll happen in v3, I'm tracking that here #833.

So I'm gonna close this, since I'm closing all of the "altsrc doesn't work like the main package" issues. Apologies if this is rude, but I'm trying to burn down github issues. (I want to make sure I'm reading all of them! which means I've gotta close a bunch)

@costela
Copy link
Author

costela commented Aug 17, 2019

waiting for v3 sucks, but getting a proper solution in the end sucks a bit less 😉

subpop added a commit to RedHatInsights/yggdrasil that referenced this issue Apr 8, 2021
cert-file is used to derive certain required identifiers within the MQTT and HTTP clients. This effectively makes the cert-file a required field, but we have to manually check for their non-empty values because of limitations in the cli package. See urfave/cli#849.

Fixes RHCLOUD-12333

Signed-off-by: Link Dupont <[email protected]>
geoknee added a commit to statechannels/go-nitro that referenced this issue May 12, 2023
We can't use the Required: true API from "github.com/urfave/cli/v2"
since that will complain even if the flag is set in a config file.

urfave/cli#849

In future we may want to force these keys to be supplied at the command line (best practices would steer away from storing sensitive information in a config file).

This commit also enhances the output of the constructor.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug describes or fixes a bug status/confirmed confirmed to be valid, but work has yet to start
Projects
None yet
Development

No branches or pull requests

2 participants