Uh oh!
There was an error while loading. Please reload this page.
Add default database settting, and env var overrides - #73
Conversation
👋 Thanks for assigning @tnull as a reviewer! |
tnull
commented
Dec 8, 2025
485243d to
5f38982Compareldk-reviews-bot
commented
Dec 20, 2025
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
ldk-reviews-bot
commented
Dec 22, 2025
🔔 2nd Reminder Hey @tnull! This PR has been waiting for your review. |
ldk-reviews-bot
commented
Dec 24, 2025
🔔 3rd Reminder Hey @tnull! This PR has been waiting for your review. |
ldk-reviews-bot
commented
Dec 27, 2025
🔔 4th Reminder Hey @tnull! This PR has been waiting for your review. |
ldk-reviews-bot
commented
Dec 29, 2025
🔔 5th Reminder Hey @tnull! This PR has been waiting for your review. |
ldk-reviews-bot
commented
Dec 31, 2025
🔔 6th Reminder Hey @tnull! This PR has been waiting for your review. |
ldk-reviews-bot
commented
Jan 3, 2026
🔔 7th Reminder Hey @tnull! This PR has been waiting for your review. |
ldk-reviews-bot
commented
Jan 5, 2026
🔔 8th Reminder Hey @tnull! This PR has been waiting for your review. |
5f38982 to
1c1500fComparetankyleo
commented
Jan 10, 2026
ldk-reviews-bot
commented
Jan 12, 2026
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
tnull
left a comment
There was a problem hiding this comment.
Generally looks good, some minor comments.
| host = "localhost" | ||
| port = 5432 | ||
| database = "postgres" | ||
| default_database = "postgres" |
There was a problem hiding this comment.
Might make sense to add comment here to specify that this will only be used for the initial connection. Otherwise it could be confusing why the user needs to specify two separate databases.
| format!("postgresql://{}:{}@{}:{}", username, password, self.host, self.port) | ||
| } | ||
| pub(crate) struct Configuration { |
There was a problem hiding this comment.
So now we have ConfigandConfiguration? That seems rather confusing. Can they be merged, or at least one renamed appropriately to better distinguish their intended use?
| toml::from_str(&config_file) | ||
| .map_err(|e| format!("Failed to parse configuration file: {}", e))?; | ||
| macro_rules! read_env { |
There was a problem hiding this comment.
Do we really need macros for this? Maybe we can inline these or make them methods?
tankyleo
commented
Jan 14, 2026
Thanks tnull addressed your comments let me know if I can rebase |
tnull
left a comment
There was a problem hiding this comment.
Thanks tnull addressed your comments let me know if I can rebase
Please go ahead and squash & rebase. While you're at it, you might address the one nit below.
| // environment variable. | ||
| #[derive(Deserialize, Default)] | ||
| struct Config { | ||
| struct TomlConfigTable { |
There was a problem hiding this comment.
nit: When you squash, can we drop the Table here?
Allow only a subset of `JwtAuthConfig` fields to be set in config file, as we allow any members of the configuration tables to not be set in the file, and instead be set by the corresponding environment variable.
...just like we do for the RSA public key used to authenticate incoming JWTs. Also parse the root certificate in `PostgresTlsBackend::new`. This makes `PostgresTlsBackend` consistent with `JWTAuthorizer`; both structs take cryptographic objects as `&str`'s and parse them in their constructors.
Not all postgres hosted services use `postgres` Also rename `database` config parameter to `vss_database`
This allows users to configure VSS-Server entirely through environment variables, without a configuration file. We hence remove the requirement that a path to a configuration file always be passed as an argument. If a file is passed as an argument, and it does not exist, we now abort startup. Also consolidate host and port settings into single socket address setting.
b8d53a3 to
f153b49Comparetankyleo
commented
Jan 14, 2026
Rebased and squashed with the new |
Based on #72