Optionally use Environment Variables for Config #57
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
That's great - thanks Steve! Will check this out ASAP 👍 |
stevekirks
changed the title
stevekirks
changed the title
nblumhardt
reviewed
Oct 8, 2020
| @@ -45,8 +48,28 @@ public static SeqForwarderConfig ReadOrInit(string filename) | |||
| } | |||
|
|
|||
| var content = File.ReadAllText(filename); | |||
| return JsonConvert.DeserializeObject<SeqForwarderConfig>(content, SerializerSettings) ?? | |||
| throw new ArgumentException("Configuration content is null."); | |||
| var seqForwarderConfig = JsonConvert.DeserializeObject<SeqForwarderConfig>(content, SerializerSettings); | |||
There was a problem hiding this comment.
I think throwing when DeserializeObject returns null might be safer here, since it'd indicate either a bug or malformed configuration file, both of which we might want to know about. The later check won't be triggered if any environment variable is set.
There was a problem hiding this comment.
That makes sense. I thought the config file might be optional but on second thoughts just the env vars are. I've made these two changes now (1be8759)
|
For the Config command, I've changed it so it only reads from the config file (env vars aren't loaded). More what the user expects? (6744940) |
|
Looks great, thanks Steve 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi Nick
I've had a go at implementing Environment Variables for the config. Hopefully it's what you had in mind but you may have a better strategy. Here's some explanations for the pull request;
Example Environment Variable names:
FORWARDER_OUTPUT_RAWPAYLOADLIMITBYTESFORWARDER_DIAGNOSTICS_INTERNALLOGGINGLEVELIts using the
Microsoft.Extensions.Configuration.ConfigurationBuilderto load the Environment Variables and theGetValue()method to get each variable the right type.Its renamed property
EncodedApiKeytoApiKeyso that the environment variableFORWARDER_OUTPUT_APIKEYwould match. (Also updated the check that excludes it inConfigCommand).With these changes, the Config command lists the config file with environment variables overwritten, and writes update the config file with any changes as well as any overwritten environment variables. Not sure if that is the expectation.
If you have ideas for improvement I'm all ears. If you think a different way would be better that's ok too.
Addresses #55