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

Separate the DB configuration #3198

Closed
wants to merge 1 commit into from
Closed

Conversation

m-lord-renkse
Copy link
Contributor

Description

Separate the DB configuration into base URL, user and password.

The reason of doing this is to add an extra layer of safety handing the DB credentials.

How to test

  1. Unit test

@@ -54,6 +54,10 @@ pub struct Arguments {
#[clap(long, env, default_value = "postgresql://")]
pub db_url: Url,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be removed once the config is migrated to the new one.

@@ -40,6 +40,10 @@ pub struct Arguments {
#[clap(long, env, default_value = "postgresql://")]
pub db_url: Url,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ditto

@m-lord-renkse m-lord-renkse force-pushed the separate-db-configuration branch 2 times, most recently from fc2a04e to 0a34b0c Compare December 31, 2024 13:18
@m-lord-renkse m-lord-renkse force-pushed the separate-db-configuration branch from 0a34b0c to ff72371 Compare December 31, 2024 13:18
Copy link
Contributor

@MartinquaXD MartinquaXD left a comment

Choose a reason for hiding this comment

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

The reason of doing this is to add an extra layer of safety handing the DB credentials.

Could you provide more details how this gives us extra safety?
Also why is a change in the services repo needed? Wouldn't it be sufficient to handle these things separately in the infra repo and then assemble the individual parts to a single URL?

#[derive(Clone, clap::Parser)]
pub struct Db {
/// Base Url of the Postgres database
pub db_base_url: Option<Url>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Set this URL non Option with a default. TODO after the configuration is migrated to the new one.

impl Db {
/// Returns the DB URL with credentials
/// Returns `None` if the URL is not configured
pub fn to_url(&self) -> Option<Url> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After the configuration is changed, this will return directly an Url

@m-lord-renkse m-lord-renkse deleted the separate-db-configuration branch December 31, 2024 15:04
@github-actions github-actions bot locked and limited conversation to collaborators Dec 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants