Add support for env variables and drop config.toml #100
Conversation
Summary of ChangesHello @niteshbalusu11, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the Noah server's configuration management by transitioning from TOML-based files to environment variables. This change aims to simplify deployment, especially in containerized environments, by leveraging standard environment variable practices. The update involves modifying how configuration is loaded, removing dynamic file-watching capabilities, and adjusting Docker-related files to align with the new approach. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the configuration management to use environment variables instead of a TOML file, removing the hot-reloading capability. The changes are consistently applied across the server, CLI, and Docker setup. The new approach simplifies the configuration logic significantly.
I've provided a few suggestions to further improve the implementation:
- Avoiding
unsafewhen configuring AWS credentials. - Simplifying the loading of
.envfiles. - Improving the loading logic for required configuration variables.
- Reducing code duplication between the server and CLI binaries by refactoring into a shared library.
Overall, this is a solid refactoring. Addressing these points will enhance the code's safety and maintainability.
| fn validate(&self) -> Result<()> { | ||
| if self.postgres_url.is_empty() { | ||
| anyhow::bail!("NOAH_POSTGRES_URL is required"); | ||
| } | ||
| if self.expo_access_token.is_empty() { | ||
| anyhow::bail!("NOAH_EXPO_ACCESS_TOKEN is required"); | ||
| } | ||
| if self.ark_server_url.is_empty() { | ||
| anyhow::bail!("NOAH_ARK_SERVER_URL is required"); | ||
| } | ||
| if self.s3_bucket_name.is_empty() { | ||
| anyhow::bail!("NOAH_S3_BUCKET_NAME is required"); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
This validate function can be removed by changing how required environment variables are loaded. Instead of loading with unwrap_or_default() and then checking for emptiness here, you can load required variables in load() to fail fast if they are missing. This would be more direct and consistent with how server/src/bin/cli.rs handles its configuration.
For example, for postgres_url:
// In Config::load()
let postgres_url = std::env::var(format!("{}{}", ENV_PREFIX, "POSTGRES_URL"))
.context("NOAH_POSTGRES_URL is required")?;
// ... then in the struct initialization
let config = Self {
postgres_url,
// ... other fields
}You can apply this pattern to all required variables (NOAH_POSTGRES_URL, NOAH_EXPO_ACCESS_TOKEN, NOAH_ARK_SERVER_URL, NOAH_S3_BUCKET_NAME) and then remove the validate function and its call on line 88.
niteshbalusu11
changed the title
Config files are terrible for production deployments to cloud providers. env variables are just simpler and easy.
This also removes a lot of code and makes stuff easier.