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

Switch to Postgres storage #1234

Merged
merged 6 commits into from
Sep 28, 2023

Conversation

alfiedotwtf
Copy link
Contributor

This change switches out the storage API for the Scanner and Web Server from InMemory to Postgres

Comment on lines 50 to 79
/// Postgres host
#[clap(long, default_value = "localhost")]
pub postgres_host: String,

/// Postgres port
#[clap(long, default_value = "5432")]
pub postgres_port: u16,

/// Postgres user
#[clap(long, default_value = "postgres")]
pub postgres_user: String,

/// Postgres password
#[clap(long)]
pub postgres_password: Option<String>,

/// Postgres database
#[clap(long)]
pub postgres_database: Option<String>,

/// Postgres max connections
#[clap(long, default_value = "10")]
pub postgres_max_connections: u32,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make an attempt to group postgres-related configuration to in its own struct. I think clap supports composable configuration, not sure though.

Please note that we DO NOT want use commands for this. I know commands can do it, but it's the wrong tool. It would be sufficient to just be able to put these in a struct then throw the struct in here.

IF that works, then it would be great if we could have tree-like validation, where postgres settings only work if the database type is postgres. But that comes later, no need to spend too much time on it. Let's start with the struct.

If that struct works, then please put it in api-server-common, and use it for both the scanner-daemon and the web-server.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new changes have a separate struct for Postgres, and are parsed via the default(). If we want to be optional vs other backing storage types as discussed in the meeting, I'm not sure if Clap supports the Enum like you mentioned so you may have to detect manually after parse() the top-level?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can live with how it's now. We can beautify it later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to use an enum you can actually use a subcommand like:

#[derive(Subcommand, Clone, Debug)]
pub enum Command {
/// Run the mainnet node.
Mainnet(RunOptions),
/// Run the testnet node.
Testnet(RunOptions),
/// Run the regtest node.
Regtest(Box<RegtestOptions>),
}

@alfiedotwtf alfiedotwtf marked this pull request as draft September 27, 2023 08:47
@alfiedotwtf alfiedotwtf force-pushed the alfie/switch-to-postgres-storage branch from c55ca6d to 4b49754 Compare September 27, 2023 10:10
@alfiedotwtf alfiedotwtf marked this pull request as ready for review September 28, 2023 03:25
@alfiedotwtf alfiedotwtf marked this pull request as draft September 28, 2023 09:17
@alfiedotwtf alfiedotwtf force-pushed the alfie/switch-to-postgres-storage branch from 4b49754 to 30224ec Compare September 28, 2023 09:36
@alfiedotwtf alfiedotwtf marked this pull request as ready for review September 28, 2023 09:37
@TheQuantumPhysicist TheQuantumPhysicist merged commit fe8b6a9 into master Sep 28, 2023
23 checks passed
@TheQuantumPhysicist TheQuantumPhysicist deleted the alfie/switch-to-postgres-storage branch September 28, 2023 14:00
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.

None yet

4 participants