-
Notifications
You must be signed in to change notification settings - Fork 20
Add testing suite for configuration precedence and omission testing #97
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
base: main
Are you sure you want to change the base?
Conversation
Relates-to: wasmCloud#16 Signed-off-by: Jakob Beckmann <f4z3r-github@pm.me>
Relates-to: wasmCloud#16 Signed-off-by: Jakob Beckmann <f4z3r-github@pm.me>
Relates-to: wasmCloud#16 Signed-off-by: Jakob Beckmann <f4z3r-github@pm.me>
crates/wash/src/config.rs
Outdated
|
|
||
| // Environment variables with WASH_ prefix | ||
| figment = figment.merge(Env::prefixed("WASH_")); | ||
| figment = figment.merge(Env::prefixed("WASH_").split("_")); |
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 am not quite done with the testing, but wanted to make sure this is desired. I have the feeling this is the typical way to do this, instead of using the default from figment (which is a dot - .).
Not setting this does not allow configurations via the env vars as shown in the test below (https://github.com/wasmCloud/wash/pull/97/files#diff-f7f9d11319f33b9af91da4ef6ce4b7111c1baceef1b82db506c4612ba6f808b1R356). If this is removed, the test fails as the splitting does not work on underscores.
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 will need to add a test on how this works on "multi-word" configuration options though such as here:
wash/crates/wash/src/component_build.rs
Line 48 in 0d7cde2
| pub custom_command: Option<Vec<String>>, |
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 updated this to use two underscored (__) as the separator. Otherwise multi-word configurations cannot be parsed from the environment. This results in the following sample configuration via environment variables:
WASH_BUILD__RUST__CUSTOM_COMMAND="[cargo,build]"The only slightly weird stuff is the single underscore in the prefix. We could update that to a double underscore as well to have it be consistent. Or do you see any other separator instead of double underscore that might make sense (note single underscore will not work properly) @brooksmtownsend ? The figment default is a dot, but this is not supported properly in many shells AFAIK.
Relates-to: wasmCloud#16 Signed-off-by: Jakob Beckmann <f4z3r-github@pm.me>
|
Is it possible that the tests are slightly flaky? I am unsure I understand why the tests on Ubuntu fail. Regarding the tests on Windows, this is extremely weird. While the test CLI context creates a new temporary directory for every test that should host all files, it seems that on Windows some configurations are found in the test CLI context...? |
Related Issues
#16
Consumer Impact
None