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
fix ability to mix/merge json env variables with env_nested_delimiter… #3640
Conversation
please review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get the request, but I'm not sure about the best way to proceed.
try: | ||
env_val = settings.__config__.json_loads(env_val) | ||
except JSONDecodeError: | ||
... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... | |
pass |
if env_val: | ||
try: | ||
env_val = settings.__config__.json_loads(env_val) | ||
except JSONDecodeError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
except JSONDecodeError: | |
except ValueError: |
More consistent to just use ValueError
env_var[last_key] = env_val | ||
|
||
if env_val: | ||
try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem here is we're calling json.loads
on every field which:
- could have some weird side effects - e.g. you want
"foobar"
butjson_loads
converts it tofoobar
- It could effect performance
We could:
- not do this and document it?
- check if
env_val
starts with{
or[
and only parse json in that case
please update. |
I think this overlaps a lot with #4216, but that solution is more complete, even that we're deferring to V2. |
Change Summary
fix ability to mix/merge json env variables with variables defined according to env_nested_delimiter
Related issue number
Found a corner-case when we are mixing 2 types of nested environment variables definition.
#3159
Checklist
changes/<pull request or issue id>-<github username>.md
file added describing change(see changes/README.md for details)