commandline icon indicating copy to clipboard operation
commandline copied to clipboard

Add environment variable support

Open jannickj opened this issue 5 years ago • 29 comments

Fixes: https://github.com/commandlineparser/commandline/issues/677

Allows for enviroment variables to be part of a cli setup the order is Args > Env > Default NB. Currently does not support sequences (Future work - will instead crash with a helpful error msg)

I also changed 0/1 to be parsable booleans as that's the language agnotisc / most common way to set booleans via environment variables.

jannickj avatar Oct 05 '20 18:10 jannickj

Hello?

jannickj avatar Oct 19 '20 20:10 jannickj

:wave:

jannickj avatar Nov 12 '20 12:11 jannickj

:cloud_with_rain: :walking_man:

jannickj avatar Jan 14 '21 21:01 jannickj

@ericnewton76 Could this be part of the 2.9.0 release, please? 🙂

SommerEngineering avatar Jan 22 '21 06:01 SommerEngineering

:sleeping_bed:

jannickj avatar Feb 09 '21 10:02 jannickj

:sun_behind_small_cloud:

jannickj avatar Feb 22 '21 11:02 jannickj

:sun_with_face:

jannickj avatar Mar 16 '21 11:03 jannickj

@ericnewton76 & @moh-hassan would be great to get this feature :)

BoBiene avatar Apr 15 '21 10:04 BoBiene

It looks like @moh-hassan has left this project (he's not listed in the org anymore), and @ericnewton76 has been inactive on GitHub since November. It effectively looks like this project has been abandoned, which is a shame :(

johnjaylward avatar Apr 15 '21 23:04 johnjaylward

So there is no one able to merge pullrequests to this repo? @gsscoder you are listed as contact, are you able to maintain this great project?

BoBiene avatar Apr 16 '21 06:04 BoBiene

:city_sunrise:

jannickj avatar May 28 '21 00:05 jannickj

:cityscape:

jannickj avatar Aug 09 '21 22:08 jannickj

:cricket:

jannickj avatar Dec 13 '21 12:12 jannickj

:skull:

jannickj avatar Jan 25 '22 22:01 jannickj

I created a NuGet package based on your branch: https://www.nuget.org/packages/CommandLineParserWithEnvironmentVariables/2.8.0

devedse avatar Apr 18 '22 14:04 devedse

@jannickj , I did find one issue though. If Required = true and you did provide the value for the variable through an EnvironmentVariable it's still showing the error that a required-option is missing.

Not sure if that is desirable.

devedse avatar Apr 18 '22 14:04 devedse

@devedse thx :D, also would say that is in my view expected behavior :)

jannickj avatar Apr 19 '22 08:04 jannickj

@jannickj , the thing is that this results in me having to check all variables in the code to see if they are not null. Even though it is required, thus, should be provided.

But I'm not sure what the best solution would be.

devedse avatar Apr 19 '22 09:04 devedse

yeah it's a bit annoying the way to get around it is to just set it equal to null! ex.


[Option("host",
	HelpText = "The host for the client",
	Default = "localhost:80")]
public string Host { get; set; } = null!;

jannickj avatar Apr 19 '22 10:04 jannickj

That's not really what I mean. What I mean is that if you set Required = true, you need to provide the variable by passing in the flag.

For example, let's assume we have:

[Option("data",
        Required = true,
	HelpText = "The data",
        Env = "DATA"
)]
public string Data { get; set; } = null!;
#This would work:
testapp --data 123
#This would not work:
SET DATA=123
testapp

#Now we're getting the message that --data is missing.

devedse avatar Apr 19 '22 11:04 devedse

right... I wasn't clear in my first message then, I tried to argue that it could make sense since required sort of strongly requests that you set the field. That said I understand what you mean and the more I think about the more I'm convinced that it's the behavior you expect.

jannickj avatar Apr 19 '22 12:04 jannickj

That's what I thought first as well. But I'm doubting it a bit now since if you look at it from the consumer end it just requires the data. It doesn't matter where it comes from. I can't think of a scenario where actually passing it as a flag is required if there is also an environment variable.

But either way is fine. I'll just work with the way is implemented now.

devedse avatar Apr 19 '22 12:04 devedse

That said I understand what you mean and the more I think about the more I'm convinced that it's the behavior you expect.

just read what I wrote, what I meant to say is that the behavior you (@devedse ) expected is the correct behavior, i.e. required accepts environment variables :sweat_smile:

jannickj avatar Apr 19 '22 13:04 jannickj

@jannickj , are you planning on implementing this?

devedse avatar Apr 21 '22 14:04 devedse

@devedse probably not, I made this PR 1.5 years ago, so at this point I'm okay with it, if someone revived this git-repo then maybe but not on a half-dead project.

jannickj avatar Apr 21 '22 14:04 jannickj

I'll see if I can make a quick fix then. I'll just release a new version of my NuGet Package

devedse avatar Apr 21 '22 14:04 devedse

:muscle:

jannickj avatar Apr 21 '22 14:04 jannickj

I think I fixed it. I actually moved all the code for setting the Value from the BuildMutable Method to the OptionMapper.MapValues as this is probably the place where we'd want to add this stuff. https://github.com/devedse/commandline/commit/9ef1591563d24d981352b0df089277f839fba64b

I also wrote some test scenario's that test Required = true in combination with providing the value through Environment values or arguments or neither.

I've released this version to NuGet: https://www.nuget.org/packages/CommandLineParserWithEnvironmentVariables/2.8.1

devedse avatar Apr 21 '22 21:04 devedse

Feel free to do a code review, Incase I missed something.

devedse avatar Apr 22 '22 10:04 devedse