Fix: respect env config virtualenv_flags #427
Conversation
dan-blanchard
left a comment
There was a problem hiding this comment.
Thanks for the PR! Sorry this has been causing you issues.
I think there's a simpler fix, so if you could update your PR to try it and let me know if it works for you, I'd appreciate it.
| storm_options = resolve_options(options, env_config, topology_class, topology_name) | ||
| activate_env(env_name, storm_options, config_file=config_file) | ||
|
|
||
| virtualenv_flags = options.get('virtualenv_flags', env_config.get('virtualenv_flags')) |
There was a problem hiding this comment.
I'm pretty sure this won't be necessary if you fix the typo that is at the heart of the issue.
| execute(_create_or_update_virtualenv, env.virtualenv_root, virtualenv_name, | ||
| requirements_paths, | ||
| virtualenv_flags=options.get('virtualenv_flags'), | ||
| virtualenv_flags=virtualenv_flags, |
There was a problem hiding this comment.
Instead of this, change options.get to storm_options.get here and I think you should be good. Looks like I missed one when I was updating things.
There was a problem hiding this comment.
Thanks for the speedy response @dan-blanchard!
I've tried out this suggestion, but it doesn't quite work as intended.
From what I can gather, when the prioritisation of configs is worked out in common.py, it's only the options part that gets applied to storm_options, therefore not capturing the virtualenv config such as virtualenv_flags.
(As a side note, I kinda took the name storm_options to mean config only relevant to Storm.)
I figured that since virtualenv_flags is the only argument that gets passed explicitly to _create_or_update_virtualenv, it would be easier to determine its value at that point instead of further complicating config resolution in resolve_options.
Would you suggest making resolve_options aware of more keys in the env config?
|
Replaced by #453. |
This makes the
update_virtualenvCLI command respectvirtualenv_flagsfound in the environment config, while prioritising the respective flags found in options first.It looks like this commit took away the ability to configure
virtualenv_flagsat the environment level, which silently broke our configuration when creating new virtualenvs.