Feature request: Allow an extension for different sources

[glerchundi]

We have been happy users of this package since you created it and everything filled our needs up until now.

Recently we added Vault to our toolchain and I tried to sketch what would be the simplest way to integrate it in our products.

One of the ideas I come up with was to add an extension point to this package so that whenever it tries to solve a flag and if no other method was able to find it (flag, env, config), use this extension/resolver as a last resource.

Would you be open to include such a feature?

[peterbourgon]

Hmm, can you propose an API?

[glerchundi]

I was thinking on something like this:

func WithResolver(r Resolver) Option {
    return func(c *Context) {
        c.resolver = r
    }
}

type Resolver func(name string) (string, error)

This will be the fourth in the priority chain and it will try to resolve just those parameters that haven't been resolved before. That is, it will follow the very same strategy the others follow.

Depending on what we want to do with the errored vs undefined parameters, we can use the ignoreUndefined for config as well as for this resolver. This will require to slightly change the API (i guess) to something like this:

type Resolver func(name string) (string, bool, error)

We can also create a specific error to indicate that the flag wasn't found but it seems like we'll be creating too much surface for the little value we'll gain.

WDYT?

[peterbourgon]

I've been thinking about this. I'm ambivalent.

At some level I think it's OK to have another source of information for flag data, and I think a version of this would compose pretty well with the existing code.

But this represents a generalization that sits lower than any of the current mechanisms for getting flag data. Meaning that if we had a Resolver, we should really re-implement all of the existing mechanisms for sourcing flag data as types of Resolvers, so to minimize the interaction between ff and flag.FlagSet to just the one thing. I'm not sure I like where that would put the package; I kind of like that it is so coupled to the concrete stuff it interacts with right now. I'm not sure why. Maybe that's a dumb opinion.

The other thing I'm hung up on the idea that flag parsing could involve, like, arbitrary work. Specifically that it could involve a network roundtrip. That opens ff.Parse up to way more failure modes than it has currently, which feels... inappropriate? somehow. Also, Flag parsing should be like the first thing that happens in a program, but I imagine most Resolvers will need some kind of runtime config (like the address of the Consul server) and then where does that put us? Two-phase flag parsing? Taking CONSUL_ADDR from the environment separately?

[glerchundi]

Thanks for you detailed answer @peterbourgon.

At some level I think it's OK to have another source of information for flag data, and I think a version of this would compose pretty well with the existing code.

Yep, from a technical standpoint it can be included easily and cleanly.

But this represents a generalization that sits lower than any of the current mechanisms for getting flag data. Meaning that if we had a Resolver, we should really re-implement all of the existing mechanisms for sourcing flag data as types of Resolvers, so to minimize the interaction between ff and flag.FlagSet to just the one thing. I'm not sure I like where that would put the package; I kind of like that it is so coupled to the concrete stuff it interacts with right now. I'm not sure why. Maybe that's a dumb opinion.

Could be but IMHO the introduction of this feature doesn't justifies the cost of doing that generalization, at least not in the initial phase.

The other thing I'm hung up on the idea that flag parsing could involve, like, arbitrary work. Specifically that it could involve a network roundtrip.

This is what worries me the most. This is a flag parsing library not a config retrieval library and probably people wouldn't expect this parsing were doing network roundtrips.

Also, Flag parsing should be like the first thing that happens in a program, but I imagine most Resolvers will need some kind of runtime config (like the address of the Consul server) and then where does that put us? Two-phase flag parsing? Taking CONSUL_ADDR from the environment separately?

Except for the runtime config & two-phase flag parsing that I would strongly advise against it, the other uses cases are legitimate IMO. In our use case Vault is always in the same endpoint, and it's a corporate service which is not gonna change. Retrieving the address from CONSUL_ADDR seems to be a clean way to process.

Perhaps it's just a matter of redefining a term, and therefore the use case, that better fits that the current proposal.

I have mixed feelings with the proposal but I'm a little bit more lean in favor of having an extension point and document the reasoning behind in order promote the best practices. Thus avoiding those extreme needs like two-phase parsings.

WDYT?

Thanks again for your time.

[peterbourgon]

Retrieving the address from CONSUL_ADDR seems to be a clean way to process.

So your func main looks like

c, err := consul.NewClient(os.Getenv("CONSUL_ADDR"))
if err != nil {
    log.Fatal(err)
}

r := newConsulResolver(c)

fs := flag.NewFlagSet(...)
var (
    ...
)
ff.Parse(fs, os.Args[1:], ff.WithResolver(r))

...

is that right?

[peterbourgon]

Agh, no, sorry, the answer is staring me right in the face.

ff stands for flags-first,

I'm not fundamentally opposed to the idea of extension points, but I'm vetoing the Resolver design. Any extension of ff will need to arrive wearing the clothes of a FlagSet.

That's not really feasible at the moment, as ff operates on a concrete type. So step one would be coming up with a FlagSet abstraction that permits arbitrary extension, and also works transparently with a *flag.FlagSet. I want to have this for other reasons, too. But I haven't figured it out yet.

Have an idea about that? Open another issue, I'm happy to brainstorm.

[glerchundi]

No worries, thanks for your time anyway :)