Setting Overhaul #105

da2ce7 · 2022-10-24T18:22:03Z

Move internal vocabulary from 'Configuration' with 'Settings'.
Make use of a 'config' folder. Will generate new config files when needed.
New Config is stored in json. And parsed without the need of the config crate.
Automatically finds and moves old configuration to new format, (keeping a backup).

josecelano · 2022-10-26T14:42:51Z

src/logging.rs


    if level == log::LevelFilter::Off {
        return;
    }

    INIT.call_once(|| {
-        stdout_config(level);
+        stdout_settings(level);


hi @da2ce7, in this case, "config" is a verb. The function stdout_config sets up the logging. I was tempted to rename it to redirect_logging_to_stdout or something like that.

josecelano · 2022-10-26T15:27:57Z

hi @da2ce7, it looks good to me. In the future, something that could be useful is to allow overwriting default settings from env vars instead of from a config/local.toml file.

I've seen that the crate we are using allows it.

I think the local.toml file is useful for development env and installations that are done manually. If you are using docker, it could be nice to overwrite the settings using env vars. Anyway, you can also mount the configuration as I did here:

https://github.com/josecelano/torrust-tracker/blob/issue-11-docker-support/bin/docker/run.sh#L6

And with K8S, you also have ConfigMaps. These are the ways that the container can get the configuration:

Inside a container command and args.
Environment variables for a container.
Add a file in read-only volume, for the application to read.
Write code to run inside the Pod that uses the Kubernetes API to read a ConfigMap.

There is also another thing to consider. Some settings are secrets. From the application point of view, I suppose it does not matter. If we are using docker, we can decide that the whole configuration is passed as a mounted read-only file or as env vars. I do not know if one could be considered safer than the other.

josecelano · 2022-10-26T15:28:32Z

ACK 698f181

da2ce7 · 2022-11-06T15:16:21Z

Needs Rebase.

.vscode/settings.json

josecelano · 2022-11-07T12:58:43Z

src/databases/sqlite.rs

 impl SqliteDatabase {
-    pub fn new(db_path: &str) -> Result<SqliteDatabase, r2d2::Error> {
-        let cm = SqliteConnectionManager::file(db_path);
+    pub fn new(settings: &Sqlite3DatabaseSettings) -> Result<SqliteDatabase, r2d2::Error> {
+        let cm = SqliteConnectionManager::file(settings.database_file_path.as_path());
        let pool = Pool::new(cm).expect("Failed to create r2d2 SQLite connection pool.");
        Ok(SqliteDatabase { pool })
    }


hi @da2ce7, I always have doubts about this kind of change. I have a "rule" which is: "do not pass to a function more than what it needs". In this case, the constructor only needs the path for the DB file. In the first version, SqliteDatabase did not know anything about the Settings, and now they are coupled. If you change the settings you also have to change this function.

Normally you would do that because you think in the future, the SqliteDatabase might need other values.

Anyway, I think it's a nice generalization since we have more than one database driver. I would expect all drivers to have a similar constructor regardless of their specific parameters.

On the other hand, would it make sense to add a wrapper for the DB path like this:

pub struct Sqlite3DatabaseSettings { pub database_file_path: Sqlite3DatabaseFilePath, } pub struct Sqlite3DatabaseFilePath { pub file_path: PathBuf, }

In general, I have another rule: "do not add a wrapper to a primitive type (or other types) if you do not have any rule (invariant) to enforce".

In this case, we do not have it unless we want to check that the file exists and contains SQLite data, which is, I think, a bad idea to do in the constructor. But maybe we can check that the path is for a file and not a directory.

But sometimes, I think these custom types are a good way to easily find where you are using them.

Anyway, I would probably do this:

pub fn new(db_file_path: &PathBuf) -> Result<SqliteDatabase, r2d2::Error> { let cm = SqliteConnectionManager::file(db_file_path.as_path()); let pool = Pool::new(cm).expect("Failed to create r2d2 SQLite connection pool."); Ok(SqliteDatabase { pool }) }

By the way, maybe we should use Path instead of PathBuf here. But I do not know very weel the difference. I think here we could use a Path because we do not need to change it.

josecelano · 2022-11-07T13:30:56Z

src/errors/settings.rs

+}
+
+#[derive(Error, Clone, Debug, Eq, Hash, PartialEq)]
+pub enum CommonSettingsError {


hi @da2ce7, what about GenericSettingsErrors?

josecelano · 2022-11-07T13:40:29Z

src/jobs/torrent_cleanup.rs


-pub fn start_job(config: &Configuration, tracker: Arc<TorrentTracker>) -> JoinHandle<()> {
+pub fn start_job(interval: u64, tracker: Arc<TorrentTracker>) -> JoinHandle<()> {


hi @da2ce7, this is an example where you decided to pass only what the function needs. I would like to know why you did it here.

josecelano · 2022-11-07T13:41:23Z

src/jobs/tracker_api.rs

-        .parse::<std::net::SocketAddr>()
-        .expect("Tracker API bind_address invalid.");
-    info!("Starting Torrust API server on: {}", bind_addr);
+pub fn start_job(settings: &ApiServiceSettings, tracker: Arc<TorrentTracker>) -> JoinHandle<()> {


hi @da2ce7, Another example where you reduce the data you pass to the function.

josecelano · 2022-11-07T13:56:37Z

src/settings/manager.rs

+    }
+
+    #[test]
+    fn it_should_write_and_read_and_write_default_config() {


@da2ce7 Do you mean "it should migrate config file from toml to json"?

And there are no assertions.

josecelano · 2022-11-07T13:59:49Z

src/settings/mod.rs

+}
+
+#[derive(Serialize, Deserialize, PartialEq, PartialOrd, Ord, Eq, Debug, Clone, Hash)]
+pub struct CommonSettings {


@da2ce7 what's the difference between "global" and "common"?

josecelano · 2022-11-07T14:01:47Z

src/settings/mod.rs

+
+#[derive(Serialize, Deserialize, PartialEq, PartialOrd, Ord, Eq, Debug, Copy, Clone, Hash, Display)]
+#[serde(rename_all = "snake_case")]
+pub enum LogFilterLevel {


@da2ce7 move to logging?

josecelano · 2022-11-07T15:53:19Z

src/tracker/mod.rs

+        CommonSettings, CommonSettingsBuilder, DatabaseSettingsBuilder, GlobalSettings, GlobalSettingsBuilder, LogFilterLevel,
+    };
+
+    pub struct TrackerArgs {


@da2ce7 I think this struct is useful now, but in the long term, I think we should extract some responsibilities from the Tracker, for example persistence. I think we could have:

One handler or controller fer each request type (announce, scrape, ...)

An app service (announce is the same from HTTP and udp). It could be also a command or query with its handler. For example: AnnounceCommand with AnnounceCommandHandler.

A TorrentService to add/update/remove torrents. We can trigger events at this level.

The TorrentService can use a TorrentRepository (DB: SQLite or MySQL) and a TorrentCache (the current in-memory structure).
But it's just a vague idea I have. I wanted to draft something, but I have not had time. And I also wanted to get to know the repo better before spending time proposing this kind of change.

josecelano · 2022-11-07T15:58:21Z

tests/udp.rs

@@ -149,8 +171,8 @@ mod udp_tracker_server {
    }

    /// Creates a new UdpTrackerClient connected to a Udp Tracker server
-    async fn new_connected_udp_tracker_client(remote_address: &str) -> UdpTrackerClient {
-        let udp_client = new_connected_udp_client(remote_address).await;
+    async fn new_connected_udp_tracker_client(remote_socket: &SocketAddr) -> UdpTrackerClient {


@da2ce7 I like this change. We should try to normalize this variable name. We are using:

remote_address

remote_socket

remote_ip

remote_client

client?

I think client_socket is better because it's more specific. But I have not changed to avoid introducing more versions.

And I've just realized in this case, it's the server_socket which makes my assumptions more valid that we should be more specific.

josecelano · 2022-11-07T16:02:35Z

hi @da2ce7, it looks good to me! I've just added some minor comments. I agree on all refactors, but I'm not sure why you want to change the config file from toml to json. I do not see any reason to do it but do not see any reason not to do it, either.

josecelano

It looks to me, although I still do not know the reason for using JSON instead of TOML. is it only because you want to remove the dependency?

da2ce7 · 2022-11-30T17:20:20Z

Considering to Rewrite this entire thing all-again. This time making full use of https://github.com/SergioBenitez/Figment

da2ce7 · 2023-02-10T19:10:28Z

Partial Rebase. Completed Database Settings.

da2ce7 · 2023-09-06T16:36:19Z

Closing, needs to be re-imagined, suggest using https://github.com/SergioBenitez/Figment as config framework library.

da2ce7 force-pushed the setting-overhaul branch 3 times, most recently from 7dc2402 to 698f181 Compare October 24, 2022 20:15

josecelano approved these changes Oct 26, 2022

View reviewed changes

da2ce7 marked this pull request as draft October 29, 2022 12:25

da2ce7 force-pushed the setting-overhaul branch from 698f181 to 8e5caac Compare November 6, 2022 15:15

da2ce7 force-pushed the setting-overhaul branch 2 times, most recently from 0fd33f2 to 3ab4828 Compare November 7, 2022 07:30

da2ce7 marked this pull request as ready for review November 7, 2022 10:50

da2ce7 requested review from josecelano and mickvandijke November 7, 2022 10:50

josecelano approved these changes Nov 7, 2022

View reviewed changes

josecelano approved these changes Nov 23, 2022

View reviewed changes

da2ce7 marked this pull request as draft November 30, 2022 17:17

josecelano mentioned this pull request Dec 15, 2022

Full docker support #123

Closed

17 tasks

da2ce7 added the Needs Rebase Base Branch has Incompatibilities label Dec 22, 2022

da2ce7 added 2 commits February 10, 2023 15:34

dev: settings overhall base

70773ca

dev: database-local settings

8fb6d05

da2ce7 force-pushed the setting-overhaul branch from 879cee7 to 8fb6d05 Compare February 10, 2023 19:05

da2ce7 removed the Needs Rebase Base Branch has Incompatibilities label Feb 10, 2023

dev: settings for api services

93c1dea

da2ce7 closed this Sep 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Setting Overhaul #105

Setting Overhaul #105

da2ce7 commented Oct 24, 2022 •

edited

Loading

josecelano Oct 26, 2022

josecelano commented Oct 26, 2022

josecelano commented Oct 26, 2022

da2ce7 commented Nov 6, 2022

josecelano Nov 7, 2022

josecelano Nov 7, 2022

josecelano Nov 7, 2022

josecelano Nov 7, 2022

josecelano Nov 7, 2022

josecelano Nov 7, 2022

josecelano Nov 7, 2022

josecelano Nov 7, 2022

josecelano Nov 7, 2022

josecelano commented Nov 7, 2022

josecelano left a comment

da2ce7 commented Nov 30, 2022

da2ce7 commented Feb 10, 2023

da2ce7 commented Sep 6, 2023


		pub fn start_job(config: &Configuration, tracker: Arc<TorrentTracker>) -> JoinHandle<()> {
		pub fn start_job(interval: u64, tracker: Arc<TorrentTracker>) -> JoinHandle<()> {

Setting Overhaul #105

Setting Overhaul #105

Conversation

da2ce7 commented Oct 24, 2022 • edited Loading

Choose a reason for hiding this comment

josecelano commented Oct 26, 2022

josecelano commented Oct 26, 2022

da2ce7 commented Nov 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josecelano commented Nov 7, 2022

josecelano left a comment

Choose a reason for hiding this comment

da2ce7 commented Nov 30, 2022

da2ce7 commented Feb 10, 2023

da2ce7 commented Sep 6, 2023

da2ce7 commented Oct 24, 2022 •

edited

Loading