fix: replace environment variables in webserver headers #844
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
Pull Request Test Coverage Report for Build 07ae82188-PR-844
💛 - Coveralls |
|
Related issue: #597 |
gdey
reviewed
May 3, 2022
| return nil, err | ||
| } | ||
|
|
||
| d[k] = *s |
There was a problem hiding this comment.
would it make sense to check if the v if of type map[string]interface{} and if so, call ParseDict on it as well?
There was a problem hiding this comment.
i dont think v can be of type map[string]interface{} or can it?
There was a problem hiding this comment.
@iwpnd I'm pretty sure it can be. I don't believe TOML has a max limit of nesting. For the tegola config we don't really have much nesting, but it seems to me the ParseDict() function should likely account for all primitives as well as a map[string]interface{} (which would just call ParseDict() again)
fix: let dict implement UnmarshalTOML
fix: move ParseDict to parse
chore: remove extra space
test: test missing env
feat: parse map[string]interface{} in ParseDict as well
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.
Using environment variables in
config.Webserver.Headerswould not replace environment variables currently. I therefore added an additional step inconfig.Parsethat would iterate over the header and resolve the environment variables if possible. I opted to otherwise raise if the environment variable is not present similar to the behaviour we have right now if any other env is missing. This could also just handle the error and use the original value.I updated the tests accordingly and added some more to test in isolation.
As mentioned on slack I'm not sure if that is the correct spot to fix it, so your input is greatly appreciated as usual. :)