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

appsetting.json and NET5 #171

Open
jkone27 opened this issue Apr 28, 2022 · 9 comments
Open

appsetting.json and NET5 #171

jkone27 opened this issue Apr 28, 2022 · 9 comments

Comments

@jkone27
Copy link

jkone27 commented Apr 28, 2022

aspnetcore and dotnet core app config has been moved to appsettings.json for a while, does argu support binding on that too?

@Junocaen
Copy link

You can give this repo a try: https://github.com/Tarmil/Argu.MicrosoftExtensions.

@bartelink
Copy link
Member

Would either of you (or @toburger) consider doing a PR that means the average user will be aware there is a valid approach? Perhaps a PR that:

  • adds a doc page mentioning a good approach
  • maybe does something that might be a reasonable OOTB addition here?
    • Obv something as complete with DI integration etc like Tarmil's stuff is best in an external project
    • Obv Argu needs to stay netstandard2.0, and ideally we'd even lose the System.ConfigurationManager ref, not be adding more of course

(My concern here is this issue still sitting here in 2030 without anythign having moved forward - unpacking the above comments is definitely someone new to F# and/or .NET would not find trivial!)

@Numpsy
Copy link

Numpsy commented Dec 30, 2023

  • and ideally we'd even lose the System.ConfigurationManager ref, not be adding more of course

A quick play with the code suggests that moving AppSettingsConfigurationReader and such out or the main assembly should be doable, except for the default IConfigurationReader fall back at

| None -> ConfigurationReader.FromAppSettings()

that wouldn't work any more in that case.

(Note: I haven't used Argu yet, I was just looking at alternatives to (FSharp.)SystemCommandLine and thinking that a command line parser depending on the System.Configuration.ConfigurationManager stack isn't ideal)

@Numpsy
Copy link

Numpsy commented Dec 30, 2023

  • maybe does something that might be a reasonable OOTB addition here?

What sort of thing are you thinking of there?
It looks like the referenced Argu.MicrosoftExtensions is binding to Microsoft.Extensions.Configuration.IConfiguration rather than using Appsettings.json directly, but maybe that is a generic / round about way of getting both Json and TOML support? (i.e maybe you could have a lib that binds IConfiguration to Argu directly, but doesn't touch the other Microsoft.Extensions.DependencyInjection stuff)

@bartelink
Copy link
Member

What sort of thing are you thinking of there?

I personally have no interest in reading config files, much less having hardwired support for only one built in. My generic statement is intended to be open-ended, but I guess I was alluding to trade-offs like:

  • if I want to read secrets, but be able to override them at the command line, there's no real support for integrating that in a clean way
  • if I want to read environment variables, but be able to override them at the command line, the included support doesnt quite do the job for me
  • I want to be able to convey the fallbacks above in the help text in some clean way
  • there's a bit of boilerplate e.g. deriving the app name etc

I resolve those forces via https://github.com/jet/dotnet-templates/blob/master/feed-consumer/Program.fs#L94 but that's hard-won, and there's definitely scope to put something in the repo, with docs that's sufficiently extensible.

Given the build tooling is pretty decent, it would not be a problem to push out the config dependencies to an external Argu.AppSettings package

The problem though, is to work out an abstraction that covers the above, i.e.:

  • if a parameter is nested three deep, you want to be able to distinguish that from a different usage of something with the same name somewhere else in the arg tree
  • if its going to be extensible, there can't be a set of attributes per mechanism (esp if they have to be in the core)
  • there should probably be some way to hook generating of help text - perhaps if the base text is supplied along with the attribute context so one can go from "Specifies a Postgres connection string" to that + Default: Use value specified via MAIN_PG_CONNECTION or Default: obtain from APP_PG_CONN secret
  • maybe there should be a parse context that gets passed through in order to be able to fetch secrets (which would also be a good hook for testing)

Solving all of that is probably out of scope for this issue (some of these desires are covered in other issues). There's definitely room to so something "good enough for now" that moves things forward.

If you and/or others have appetite, we could potentially do a -beta release cycle that trials something in a useful direction. A small breaking change such as e.g. moving out app settings attributes into another assembly might be viable in such a context.

I'm happy to muck in a bit and will definitely review things, but I'm reasonably well loaded atm with things that Need To Be Done in other projects so am not in a position to drive it. I'd definitely be able to validate a wide variety of consumption scenarios.

In other words, if you have some capacity, there's a lot of potential - go for it!

@Numpsy
Copy link

Numpsy commented Dec 31, 2023

Hmm, I hadn't thought about anything to that detail (I was just looking at the command line parsing functionality, without any other sort of configuration), so I'd have to see if I have any time in the new year for anything further.

As far as the ConfigurationManager dependency goes, I was just playing around with something like Numpsy@5b94ad7, which seemed like it worked with the same public api, except the point about not working as a 'default' functionality any more

@bartelink
Copy link
Member

bartelink commented Feb 18, 2024

Sorry, only coming back to this now - your spike seems good.

I suspect the massive majority Argu consumption scenario is massively weighted toward only using explicit parsing of the command line.
It would obv not be surprising if making the file parsing opt-in is a perf win too.

The main concern is that the behavior change would be breaking - all this really means is that it would need to wait for a V7. So making a formal PR out of it might be good? Ideally there'd be some way to signal the change with a warning or obsoletion rather than relying on doc stuff that lots of people will miss and will then get bitten by. I personally won't have the time or appetite to go full in on doing a full job of letting people arbitrarily compose the read (and help generation) wiring (and ordering) as I alluded to earlier, but at least making the config parsing opt-in might be a good first step.

I normally have a helper in my Arguments type like this

    static member Parse argv =
        let parseResults = ArgumentParser.Create<Parameters>().ParseCommandLine argv
        Arguments(Args.Configuration(EnvVar.tryGet, EnvVar.getOr parseResults.Raise), parseResults)        

or a

module Arguments =
    let parse argv = ArgumentParser.Create<Arguments>().ParseCommandLine(argv)

If that became a one-liner such as Argu.ArgumentParser.Create().ParseCommandLineOnly<Parameters>(argv), then the secondary package could augment with Argu.ArgumentParser.Create().ParseAppSettingsFallback<Parameters>(argv), and we could obsolete the .Create with a mention of needing to ref the other package if you want the appsettings too. The Create can then remain (but Obsolete) with the old program name derivation logic until V8.

Other light breaking changes like #211 might be worth considering alongside for V7.

Then perhaps a V8 can do deeper stuff like making it pluggable, but as each supported has its own factory method, those can forward to a more generic pipeline factory thing.

It might also be nice to do an equivalent of fsprojects/FSharp.AWS.DynamoDB#73 in the V7 timeframe, as the work to make the pipeline pluggable might involve some diffs that could be painful to review if we also have formatting inconsistencies in the mix)

Any thoughts @nojaf ?

@bartelink
Copy link
Member

bartelink commented Feb 18, 2024

Also noticed that the argv is optional. For V7, there should be one with a clear name and a mandatory argv, and another named to convey the behavior if you omit. Also I am considering to flip the default true of the raiseOnUsage to false and renaming it to supressRaiseOnUsageRequest.
image

@nojaf
Copy link
Collaborator

nojaf commented Feb 19, 2024

Any thoughts @nojaf ?

Not many to be honest. It is probably worth exploring in v7 when breaking changes are on the table.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants