Skip to content
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: Environment variables do not overwrite Config.toml options #2433

Merged
merged 3 commits into from
Mar 30, 2024
Merged

Fix: Environment variables do not overwrite Config.toml options #2433

merged 3 commits into from
Mar 30, 2024

Conversation

GrumiumXD
Copy link
Contributor

Closes #2429

This is my first PR on this project and it turned out a bit bigger than expected. So I am not sure about some of the decisions I made.

  • Removed separator("_") from the Environment source setup, because this is intended for nested structures.
  • Added convert_case(Case::Kebab) to the Environment source setup to match the casing serde expects.
  • Removed "[leptos-options]" from toml string to let the settings be parsed as root level, otherwise the settings from the file and the ones set by the environment variables would be considered in different structures, therefore not affecting each other. Because of this get_config_from_str() now returns a LeptosOptions object instead of a ConfFile.

The test cases gave me some troubles as I had some inconsistent results. I assume (because tests are executed in parallel) that sometimes variables set in one test could interfere with a test that does not expect them (now that they could actually overwrite settings originated from a file source).
I added temp-env as a dev dependency to address my issues. Afterwards the tests were working consistently again and I added a new test to explicitly test the override functionality.

Because I changed the parsing from ConfFile to LeptosOptions, I think the serde::Deserialize trait could actually be removed from the ConfFile struct. Any opinions on that?

@benwis
Copy link
Contributor

benwis commented Mar 29, 2024

Nice work! I'd love to remove the Deserialize struct if that's possible.

@GrumiumXD
Copy link
Contributor Author

I was just talking about getting rid of the Deserialize trait for the ConfFile struct, but getting rid of the whole ConfFile struct and only working with LeptosOptions everywhere sounds even better, I guess.
I will see what I can do.

@benwis
Copy link
Contributor

benwis commented Mar 30, 2024

I was just talking about getting rid of the Deserialize trait for the ConfFile struct, but getting rid of the whole ConfFile struct and only working with LeptosOptions everywhere sounds even better, I guess. I will see what I can do.

To be honest that is my bad wording, We have both of those because of how the config crate works. At any rate, I think this looks good, and can merge and see if anything breaks

@benwis benwis merged commit d528cbd into leptos-rs:main Mar 30, 2024
60 checks passed
@GrumiumXD GrumiumXD deleted the environment-variables-are-ignored branch March 30, 2024 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Environment variables do not overwrite Config.toml options
2 participants