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

Config Overhaul #557

Closed
wants to merge 1 commit into from
Closed

Conversation

da2ce7
Copy link
Contributor

@da2ce7 da2ce7 commented Dec 28, 2023

Copy link

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
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 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 added a commit that referenced this pull request Jan 24, 2024
72c8348 udp: handle udp requests concurrently (Cameron Garnham)

Pull request description:

  Extracted from #557

  Upgrades the udp implementation to handle requests concurrently, no timeout is needed as udp is non-blocking.

  Hard-coded limit of 50 concurrent requests. With trivial changes can make this limit configurable, however I don't think that it would really be much benefit for it, the main concern is memory usage, and since udp is no blocking each request should return very quickly, could set to 5000 and it should still be fine.

ACKs for top commit:
  josecelano:
    ACK 72c8348

Tree-SHA512: 83c4d893d5a88a59655942dcafceed25ca3b620a73fbd70d43ade4684394348c170dbfa427dfb236bcb7a8175e307547c894ec79d3c0b992441e8aa1490885b8
@da2ce7 da2ce7 added the Needs Rebase Base Branch has Incompatibilities label Jan 29, 2024
@da2ce7 da2ce7 force-pushed the 20231227_config_overhaul branch from c7b28de to 0a0a2eb Compare February 1, 2024 01:26
@josecelano
Copy link
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(())
    }

@da2ce7 da2ce7 force-pushed the 20231227_config_overhaul branch from 0a0a2eb to a55c356 Compare March 26, 2024 03:21
@josecelano
Copy link
Member

Relates to: #790

@da2ce7
Copy link
Contributor Author

da2ce7 commented Jul 13, 2024

closed by #401

@da2ce7 da2ce7 closed this Jul 13, 2024
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