Skip to content

Config Overhaul#557

Closed
da2ce7 wants to merge 1 commit intotorrust:developfrom
da2ce7:20231227_config_overhaul
Closed

Config Overhaul#557
da2ce7 wants to merge 1 commit intotorrust:developfrom
da2ce7:20231227_config_overhaul

Conversation

@da2ce7
Copy link
Copy Markdown
Contributor

@da2ce7 da2ce7 commented Dec 28, 2023

@codecov
Copy link
Copy Markdown

codecov bot commented Dec 28, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.71%. Comparing base (bf71687) to head (a55c356).

Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #557   +/-   ##
========================================
  Coverage    77.71%   77.71%           
========================================
  Files          158      158           
  Lines         8711     8711           
========================================
  Hits          6770     6770           
  Misses        1941     1941           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@josecelano
Copy link
Copy Markdown
Member

Relates to: #343

@da2ce7 da2ce7 force-pushed the 20231227_config_overhaul branch from 00c329c to 558c8c9 Compare December 30, 2023 07:39
@da2ce7 da2ce7 force-pushed the 20231227_config_overhaul branch from 558c8c9 to b2e22e7 Compare December 30, 2023 07:46
@da2ce7 da2ce7 force-pushed the 20231227_config_overhaul branch from b2e22e7 to 808a004 Compare December 30, 2023 08:28
@da2ce7 da2ce7 force-pushed the 20231227_config_overhaul branch from 808a004 to 04c5cae Compare December 30, 2023 09:34
@da2ce7 da2ce7 force-pushed the 20231227_config_overhaul branch from 04c5cae to b53a219 Compare December 31, 2023 11:46
@da2ce7 da2ce7 force-pushed the 20231227_config_overhaul branch from b53a219 to 186669f Compare January 2, 2024 05:10
@da2ce7 da2ce7 force-pushed the 20231227_config_overhaul branch from 186669f to ac3004c Compare January 2, 2024 05:48
@da2ce7 da2ce7 force-pushed the 20231227_config_overhaul branch from ac3004c to ab4e10c Compare January 3, 2024 13:19
@da2ce7 da2ce7 force-pushed the 20231227_config_overhaul branch from ab4e10c to f545d29 Compare January 3, 2024 14:28
da2ce7 added a commit that referenced this pull request Jan 4, 2024
13140f6 dev: cleanup service bootstraping (Cameron Garnham)

Pull request description:

  Extracted from:
  - #557

  Closes #560

ACKs for top commit:
  da2ce7:
    ACK 13140f6

Tree-SHA512: 70eb31e6a0c8bb93c48ca90227763c33c4c90055cacb11bc4dfc73ab9813c0833bcdfb7c731b85169d7366774671cdf72e2cc91e51b1ce289708494fc90e4e54
@da2ce7 da2ce7 force-pushed the 20231227_config_overhaul branch from f545d29 to c92ca99 Compare January 4, 2024 02:23
@da2ce7 da2ce7 force-pushed the 20231227_config_overhaul branch from c92ca99 to 571c27f Compare January 4, 2024 05:09
@da2ce7 da2ce7 force-pushed the 20231227_config_overhaul branch from ea992a8 to e6809bd Compare January 8, 2024 12:41
@da2ce7 da2ce7 force-pushed the 20231227_config_overhaul branch from 207a1eb to 39170fd Compare January 8, 2024 13:09
@da2ce7 da2ce7 force-pushed the 20231227_config_overhaul branch from 39170fd to d4b47b3 Compare January 11, 2024 08:29
@da2ce7 da2ce7 added the Needs Rebase Base Branch has Incompatibilities label Jan 11, 2024
@da2ce7 da2ce7 force-pushed the 20231227_config_overhaul branch from d4b47b3 to 53246e1 Compare January 12, 2024 03:08
@da2ce7 da2ce7 force-pushed the 20231227_config_overhaul branch from 53246e1 to 274374e Compare January 12, 2024 04:34
@da2ce7 da2ce7 removed the Needs Rebase Base Branch has Incompatibilities label Jan 12, 2024
@cgbosse cgbosse added this to the v3.0.0 milestone Jan 12, 2024
@da2ce7 da2ce7 force-pushed the 20231227_config_overhaul branch from 274374e to c47f548 Compare January 16, 2024 10:43
@da2ce7 da2ce7 force-pushed the 20231227_config_overhaul branch from c47f548 to 9d37ecc Compare January 17, 2024 10:02
@da2ce7 da2ce7 force-pushed the 20231227_config_overhaul branch from 9d37ecc to dccfb22 Compare January 17, 2024 11:03
josecelano added a commit that referenced this pull request Jan 17, 2024
3f0dcea dev: add tests to health check (Cameron Garnham)
b310c75 dev: extract config from health check (Cameron Garnham)
3b49257 dev: extract config from core::tracker (Cameron Garnham)

Pull request description:

  This is the first three commits from #557

  It includes an significant improvement of the Heath Check logic, as it now takes the active port from the binding, instead of the configuration.

ACKs for top commit:
  josecelano:
    ACK 3f0dcea

Tree-SHA512: 5076583618bf68dd7fe016b55da5a14490be2ec4e3416efe7d3bcd27c73fedbe5a219cd9f5bcc62c526007d1ae17fab7323b2199aa14fd9542bbe52eba2b6b38
@da2ce7 da2ce7 force-pushed the 20231227_config_overhaul branch from dccfb22 to c7b28de Compare January 24, 2024 10:22
@josecelano
Copy link
Copy Markdown
Member

josecelano commented Feb 26, 2024

Hi @da2ce7 I think in the past we have discussed having two types of configuration structs:

  • The DTO: the ones representing the information in the config files or in general the configuration injected into the app when it was started.
  • The domain configuration. After receiving the configuration in most cases the application only fails when it starts using the wrong values. For example, if the configuration for one service is wrong you do not know it unless you start that service. For example, if you want to use TLS for the API service and you don't provide the certificates you would not realise the configuration was wrong until you enable that service.

In the Index I've recently introduced a validation for the configuration:

https://github.com/torrust/torrust-index/blob/develop/src/config.rs#L571-L601

I wanted the application to fail if you use an invalid tracker configuration. In this case, you can't not have a public UDP tracker. We don't support it.

I'm still using the same struct. For the time being, I've just added a validate function.

    /// # Errors
    ///
    /// Will return an error if the configuration is invalid.
    pub async fn validate(&self) -> Result<(), ValidationError> {
        self.validate_tracker_config().await
    }

    /// # Errors
    ///
    /// Will return an error if the `tracker` configuration section is invalid.    
    pub async fn validate_tracker_config(&self) -> Result<(), ValidationError> {
        let settings_lock = self.settings.read().await;

        let tracker_mode = settings_lock.tracker.mode.clone();
        let tracker_url = settings_lock.tracker.url.clone();

        let tracker_url = match parse_url(&tracker_url) {
            Ok(url) => url,
            Err(err) => {
                return Err(ValidationError::InvalidTrackerUrl {
                    source: Located(err).into(),
                })
            }
        };

        if tracker_mode.is_close() && (tracker_url.scheme() != "http" && tracker_url.scheme() != "https") {
            return Err(ValidationError::UdpTrackersInPrivateModeNotSupported);
        }

        Ok(())
    }

@josecelano
Copy link
Copy Markdown
Member

Relates to: #790

@da2ce7
Copy link
Copy Markdown
Contributor Author

da2ce7 commented Jul 13, 2024

closed by #401

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs Rebase Base Branch has Incompatibilities

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants