refactor: split config into multiple modules #676

Harsh1s · 2024-03-06T22:51:28Z

what problem are you trying to solve? (or if there's no problem, what's the motivation for this change?)
-> Fixes [Refactor]: config #604
-> Currently all configurations are in a single file, utils/src/config.rs，It also contains a lot of parse_xxx, xxx_format, default_xxx methods, It's messy and difficult to manage. This PR organizes these configurations into modules, each containing the configuration itself, its parse method, and default values.
what changes does this pull request make?
-> It adds a different folder structure and splits every config struct into a separate module. I plan on replacing new method with a more flexible builder method.
are there any non-obvious implications of these changes? (does it break compatibility with previous versions, etc)
-> Not yet, although a builder refactor could mean little rewrites in curp

Harsh1s · 2024-03-06T22:53:48Z

I'll refactor the new methods into builder methods into another commit. Before doing that just some wanted feedback if possible.

Phoenix500526

There are some code style problems, please format the code. BTW, please comment on the issue #604, so I can assign this issue to you.

mergify · 2024-04-18T06:51:22Z

@Harsh1s Convert your pr to draft since CI failed

mergify · 2024-04-18T06:51:25Z

@Harsh1s Your PR is in conflict and cannot be merged.

Phoenix500526 · 2024-05-10T01:28:05Z

Hi, @Harsh1s ! This pr has been stalled for 2 months. Would you like to update it? 😄

Harsh1s · 2024-05-10T09:18:16Z

Hi, @Harsh1s ! This pr has been stalled for 2 months. Would you like to update it? 😄

I am so very sorry, I assure you I'll update it within a day.

Harsh1s · 2024-05-14T08:04:43Z

@Phoenix500526 I redid the refactor with latest changes, I still have to rewrite new methods into builder pattern which I'll do by EOD. Could you please review if current refactor is fine? I'll do the builder rewrite in the meantime.

mergify · 2024-05-14T08:29:35Z

@Harsh1s Convert your pr to draft since CI failed

Signed-off-by: Harsh1s <thisishraj@gmail.com>

Signed-off-by: Phoeniix Zhao <Phoenix500526@163.com>

codecov · 2024-05-16T09:57:42Z

Codecov Report

Attention: Patch coverage is 89.13325% with 84 lines in your changes are missing coverage. Please review.

Project coverage is 75.54%. Comparing base (e35b35a) to head (3482952).
Report is 84 commits behind head on master.

❗ Current head 3482952 differs from pull request most recent head 93f0429

Please upload reports for the commit 93f0429 to get more accurate results.

Files	Patch %	Lines
crates/utils/src/config/metrics.rs	57.62%	22 Missing and 3 partials ⚠️
crates/utils/src/config/log.rs	60.37%	17 Missing and 4 partials ⚠️
crates/utils/src/config/compact.rs	57.57%	11 Missing and 3 partials ⚠️
crates/utils/src/config/curp.rs	85.93%	7 Missing and 2 partials ⚠️
crates/utils/src/config/client.rs	95.16%	1 Missing and 2 partials ⚠️
crates/utils/src/config/cluster.rs	94.44%	0 Missing and 3 partials ⚠️
crates/utils/src/config/mod.rs	99.40%	1 Missing and 1 partial ⚠️
crates/utils/src/config/server.rs	95.12%	0 Missing and 2 partials ⚠️
crates/utils/src/config/auth.rs	85.71%	0 Missing and 1 partial ⚠️
crates/utils/src/config/engine.rs	75.00%	0 Missing and 1 partial ⚠️
... and 3 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #676      +/-   ##
==========================================
- Coverage   75.55%   75.54%   -0.02%     
==========================================
  Files         180      198      +18     
  Lines       26938    27496     +558     
  Branches    26938    27496     +558     
==========================================
+ Hits        20353    20771     +418     
- Misses       5366     5446      +80     
- Partials     1219     1279      +60

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

Phoenix500526 · 2024-05-16T12:51:27Z

Hi, @Harsh1s I've rebased this pr and modify the code to pass the ci process.

Phoenix500526 · 2024-05-17T01:47:39Z

crates/utils/src/config/mod.rs

+use getset::Getters;
+use serde::Deserialize;
+
+use crate::config::{


Better use use self::{auth::AuthConfig, .....}

Phoenix500526 · 2024-05-17T01:48:19Z

crates/utils/src/config/mod.rs

+    /// Generates a new `XlineServerConfig` object
+    #[must_use]
+    #[inline]
+    #[allow(clippy::too_many_arguments)]


Shall we use a builder pattern to refactor it?

Phoenix500526 · 2024-05-17T01:53:04Z

crates/utils/src/config/mod.rs

@@ -0,0 +1,442 @@
+/// Xline auth configuration module


We can provide a prelude here or publicly use those XXConfig here. In doing so, we can use use utils::config::prelude::{AuthConfig, ClientConfig} or use utils::config::{AuthConfig, ClientConfig} instead of use utils::config::{auth::AuthConfig, client::ClientConfig}.

Phoenix500526 · 2024-05-17T01:58:03Z

crates/utils/src/config/metrics.rs

+    /// Enable or not
+    #[getset(get = "pub")]
+    #[serde(default = "default_metrics_enable")]
+    pub enable: bool,
+    /// The http port to expose
+    #[getset(get = "pub")]
+    #[serde(default = "default_metrics_port")]
+    pub port: u16,
+    /// The http path to expose
+    #[getset(get = "pub")]
+    #[serde(default = "default_metrics_path")]
+    pub path: String,
+    /// Enable push or not
+    #[getset(get = "pub")]
+    #[serde(default = "default_metrics_push")]
+    pub push: bool,
+    /// Push endpoint
+    #[getset(get = "pub")]
+    #[serde(default = "default_metrics_push_endpoint")]
+    pub push_endpoint: String,
+    /// Push protocol
+    #[getset(get = "pub")]
+    #[serde(with = "protocol_format", default = "default_metrics_push_protocol")]
+    pub push_protocol: MetricsPushProtocol,


Why do you change the visibility of these fields when you already have their public getter methods?

Actually, I prefer to change the visibility of fields rather than use a public getter in that both achieve the same result, but the getter method introduces additional dependencies.

Phoenix500526 · 2024-05-17T02:01:39Z

crates/utils/src/config/auth.rs

+    /// The public key file
+    #[getset(get = "pub")]
+    pub auth_public_key: Option<PathBuf>,
+    /// The private key file
+    #[getset(get = "pub")]
+    pub auth_private_key: Option<PathBuf>,


Same as above.

Phoenix500526 · 2024-05-17T02:18:38Z

crates/curp-test-utils/src/test_cmd.rs

@@ -422,7 +422,7 @@ impl TestCE {
        after_sync_sender: mpsc::UnboundedSender<(TestCommand, LogIndex)>,
        engine_cfg: EngineConfig,
    ) -> Self {
-        let engine_type = match engine_cfg {
+        let engine_type: EngineType = match engine_cfg {


NIT: please remove this EngineType.

Phoenix500526 · 2024-05-17T02:19:56Z

crates/utils/src/config/auth.rs

+    #[getset(get = "pub")]
+    pub auth_public_key: Option<PathBuf>,
+    /// The private key file
+    #[getset(get = "pub")]
+    pub auth_private_key: Option<PathBuf>,


Same as above

Phoenix500526 · 2024-05-17T02:35:23Z

crates/utils/src/config/tls.rs

+    #[getset(get = "pub")]
+    pub peer_ca_cert_path: Option<PathBuf>,
+    /// The public key file used by peer
+    #[getset(get = "pub")]
+    pub peer_cert_path: Option<PathBuf>,
+    /// The private key file used by peer
+    #[getset(get = "pub")]
+    pub peer_key_path: Option<PathBuf>,
+    /// The CA certificate file used by client to verify peer certificates
+    #[getset(get = "pub")]
+    pub client_ca_cert_path: Option<PathBuf>,
+    /// The public key file used by client
+    #[getset(get = "pub")]
+    pub client_cert_path: Option<PathBuf>,
+    /// The private key file used by client
+    #[getset(get = "pub")]
+    pub client_key_path: Option<PathBuf>,


Same as above

Signed-off-by: Harsh1s <thisishraj@gmail.com>

Harsh1s · 2024-05-17T09:39:06Z

Thanks for the help @Phoenix500526 , I've pulled the changes and I'll address all your reviews now. I also started working on builder patterns for all the structs, I'll make commits one by one.

mergify · 2024-05-17T09:49:20Z

@Harsh1s Convert your pr to draft since CI failed

Phoenix500526 · 2024-05-17T09:55:11Z

Hi, @Harsh1s! IMO, to implement the builder pattern for all these XXConfig structures may not be a good practice. I only recommend implementing the builder pattern for those XXConfig structures with too many arguments in their constructor. BTW, to pass the commit message validation ci task, please use "refactor(config)" or "refactor" to replace the "refactor-config" in your commit message. 😄

Harsh1s · 2024-05-17T09:56:58Z

Hi, @Harsh1s! IMO, to implement the builder pattern for all these XXConfig structures may not be a good practice. I only recommend implementing the builder pattern for those XXConfig structures with too many arguments in their constructor. BTW, to pass the commit message validation ci task, please use "refactor(config)" or "refactor" to replace the "refactor-config" in your commit message. 😄

Understood, thank you so much for all the help and being so patient with me!

Phoenix500526 · 2024-05-17T09:59:16Z

We also provide the .pre-commit-config.yaml in our project root. You can execute the pre-commit install command at the project root to install the git hook. It will automatically run the cargo fmt, cargo clippy and validate the commit message for you before you commit your code modifications. I think it's helpful.

Harsh1s · 2024-05-17T10:07:16Z

We also provide the .pre-commit-config.yaml in our project root. You can execute the pre-commit install command at the project root to install the git hook. It will automatically run the cargo fmt, cargo clippy and validate the commit message for you before you commit your code modifications. I think it's helpful.

Okay, will do. Thanks!

Phoenix500526 reviewed Mar 14, 2024

View reviewed changes

mergify bot marked this pull request as draft April 18, 2024 06:51

mergify bot requested a review from a team April 19, 2024 04:35

Harsh1s force-pushed the refactor-config branch from 267110c to 7993e50 Compare May 12, 2024 17:59

Harsh1s marked this pull request as ready for review May 12, 2024 18:01

mergify bot marked this pull request as draft May 14, 2024 08:29

mergify bot added the CI:fail CI has failed label May 14, 2024

Harsh1s added 3 commits May 16, 2024 15:54

refactor: split config into multiple modules

b835b24

Signed-off-by: Harsh1s <thisishraj@gmail.com>

refactor: split config into multiple modules

ef75856

Signed-off-by: Harsh1s <thisishraj@gmail.com>

refactor: fix tls_test to use refactored config

cb9497b

Signed-off-by: Harsh1s <thisishraj@gmail.com>

Phoenix500526 force-pushed the refactor-config branch from 33a8026 to cb9497b Compare May 16, 2024 08:07

style: format code style

d41a714

Signed-off-by: Phoeniix Zhao <Phoenix500526@163.com>

Phoenix500526 force-pushed the refactor-config branch 2 times, most recently from 9c1aaa9 to d1904bd Compare May 16, 2024 09:37

refactor: modify modules' name to meet the clippy lint

3482952

Signed-off-by: Phoeniix Zhao <Phoenix500526@163.com>

Phoenix500526 force-pushed the refactor-config branch from d1904bd to 3482952 Compare May 16, 2024 09:39

mergify bot marked this pull request as ready for review May 16, 2024 09:56

mergify bot removed the CI:fail CI has failed label May 16, 2024

Phoenix500526 reviewed May 17, 2024

View reviewed changes

mergify bot requested a review from a team May 17, 2024 02:29

Phoenix500526 reviewed May 17, 2024

View reviewed changes

Phoenix500526 requested a review from bsbds May 17, 2024 02:36

mergify bot requested a review from a team May 17, 2024 02:36

Phoenix500526 self-assigned this May 17, 2024

refactor-config: Implements builder pattern for AuthConfig struct

93f0429

Signed-off-by: Harsh1s <thisishraj@gmail.com>

mergify bot marked this pull request as draft May 17, 2024 09:49

mergify bot added the CI:fail CI has failed label May 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: split config into multiple modules #676

refactor: split config into multiple modules #676

Harsh1s commented Mar 6, 2024 •

edited

Harsh1s commented Mar 6, 2024 •

edited

Phoenix500526 left a comment

mergify bot commented Apr 18, 2024

mergify bot commented Apr 18, 2024

Phoenix500526 commented May 10, 2024

Harsh1s commented May 10, 2024

Harsh1s commented May 14, 2024

mergify bot commented May 14, 2024

codecov bot commented May 16, 2024 •

edited

Phoenix500526 commented May 16, 2024

Phoenix500526 May 17, 2024

Phoenix500526 May 17, 2024

Phoenix500526 May 17, 2024

Phoenix500526 May 17, 2024

Phoenix500526 May 17, 2024

Phoenix500526 May 17, 2024

Phoenix500526 May 17, 2024

Phoenix500526 May 17, 2024

Phoenix500526 May 17, 2024

Harsh1s commented May 17, 2024

mergify bot commented May 17, 2024

Phoenix500526 commented May 17, 2024

Harsh1s commented May 17, 2024

Phoenix500526 commented May 17, 2024

Harsh1s commented May 17, 2024

refactor: split config into multiple modules #676

Are you sure you want to change the base?

refactor: split config into multiple modules #676

Conversation

Harsh1s commented Mar 6, 2024 • edited

Harsh1s commented Mar 6, 2024 • edited

Phoenix500526 left a comment

Choose a reason for hiding this comment

mergify bot commented Apr 18, 2024

mergify bot commented Apr 18, 2024

Phoenix500526 commented May 10, 2024

Harsh1s commented May 10, 2024

Harsh1s commented May 14, 2024

mergify bot commented May 14, 2024

codecov bot commented May 16, 2024 • edited

Codecov Report

Phoenix500526 commented May 16, 2024

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

Harsh1s commented May 17, 2024

mergify bot commented May 17, 2024

Phoenix500526 commented May 17, 2024

Harsh1s commented May 17, 2024

Phoenix500526 commented May 17, 2024

Harsh1s commented May 17, 2024

Harsh1s commented Mar 6, 2024 •

edited

Harsh1s commented Mar 6, 2024 •

edited

codecov bot commented May 16, 2024 •

edited