-
-
Notifications
You must be signed in to change notification settings - Fork 91
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
Support environment variables as alternate config mechanism #2162
Conversation
8aad83a
to
d74d900
Compare
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.
Neat hack, but I think this needs a bit of work and most importantly tests.
@tenzir/vast how would you test this feature? |
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 left some final remarks inline. Other than that let's ship it!
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.
Not to derail the PR, but some additional minor issues.
Given that --verbosity was basically a non-existent config option, we should have been mapping the shorthands to --console-verbosity in the first place.
This is another stab at breaking up the giant monolith function and making it more side-effect free. This one might appear as zero-sum refactoring because we're adding a singleton, but the bigger evil is the giant parsing function that we need to break down first.
048fe80
to
4da964a
Compare
To the user, it doesn't matter whether locking takes place under the hood. They just want to get their environment worked. This is why the locked_ prefix no longer exists.
It turns out that other functions operate the same way, e.g., get_plugin_dirs takes also an actor_system_config. So we'll keep this as transport medium for now.
4da964a
to
3cd4bf5
Compare
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 investigated the last commit for a while, and it looks like the whole "convert settings based on the type in the configuration" is only done for options that come directly from caf. For the vast.
options it looks like the default YAML/vast::data parser does (accidentally?) the correct thing already.
The logic on this part is pretty complicated so there may be some config parsing bugs left here, but even if so they would apply to the existing code and not be introduced by this PR so merging this should be fine.
This PR adds new functionality to use environment variables as alternate input to a YAML config file.
📝 Checklist
VAST_
andCAF_
env vars into settings🎯 Review Instructions
Commit-by-commit.