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 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 added the Needs Rebase Base Branch has Incompatibilities label Jan 11, 2024
@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
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
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
@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(())
    }

@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