-
-
Notifications
You must be signed in to change notification settings - Fork 22
feat(config): use global config #200
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?
feat(config): use global config #200
Conversation
If I understand the docstring above correctly, the environment variables should be overridden by the other sources. The tests attempt to assert that, but they fail. If that is the case, I can easily fix it in this PR! /// This function attempts to load the configuration from the following sources, in order of precedence:
///
/// 1. A TOML file specified by the `file` argument.
/// 2. A TOML file named `mago.toml` in the current directory.
/// 3. Environment variables with the prefix `MAGO_`.
|
8fd2063
to
af13fe2
Compare
Actually, the doc is wrong. env vars should always override any other configuration. it is useful to have it this way to allow users to override a specific config for testing purposes, e.g env vars can also be used in CI env to specify configurations that will always apply regardless of the content of the configuration file. |
@JungoQuentin could you please fix the documentation to specify that env vars always override other sources, and update the tests accordingly? thank you. |
Priority should ideally be:
|
3563616
to
837ac0d
Compare
Thanks for the clarification @azjezz ! For adding the global configuration file, I propose the following logic:
|
837ac0d
to
0ced867
Compare
0ced867
to
1962035
Compare
📌 What Does This PR Do?
Add test to cover order of precedence for configuration sources
🔍 Context & Motivation
In order to treat this issue : #191
In order to address this issue (#191), I first added tests to ensure it works as intended
🛠️ Summary of Changes
📂 Affected Areas
🔗 Related Issues or PRs
related to #191
📝 Notes for Reviewers