Parsing a config file with `Config` by pott-cho · Pull Request #130 · aicers/github-dashboard-server

pott-cho · 2025-04-22T04:08:04Z

Close: #106

danbi2990

Changelog needs to be updated.

README.md

src/settings.rs

codecov · 2025-04-28T04:53:51Z

Codecov Report

Attention: Patch coverage is 0% with 24 lines in your changes missing coverage. Please review.

Project coverage is 30.13%. Comparing base (c2b2c2c) to head (875cee9).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/main.rs	0.00%	13 Missing ⚠️
src/settings.rs	0.00%	10 Missing ⚠️
src/web.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #130      +/-   ##
==========================================
+ Coverage   28.42%   30.13%   +1.71%     
==========================================
  Files          10       10              
  Lines         387      365      -22     
==========================================
  Hits          110      110              
+ Misses        277      255      -22

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

src/settings.rs

danbi2990 · 2025-04-29T01:02:00Z

The changelog needs to be updated to reflect the CLI and config changes.

sehkone · 2025-04-29T23:57:46Z

CHANGELOG.md

  nor `last` is specified.
 - GraphQL API `issues` and `pullRequests` return an error if conflicting
  pagination arguments (e.g., `first` and `before`) are provided simultaneously.
+- Changed from file-based configuration loading to Config-based loading,


I don't think "Config-based" clearly conveys what we intended, since it doesn't specify that "Config" refers to the crate. It would be better to use a more explicit form, such as the config crate, or the config crate, or config-based.

Its official site refers to it as config, not Config.

In addition to Kone's comment, I believe the following three key changes should be included (with better wording):

Added CLI arguments: --key, --cert

Changed CLI format: -c <CONFIG_PATH>

Removed configuration file options for key and cert

Based on your feedback, I removed the phrases "Config-based" and "resulting in changes to the CLI execution method," and provided a more detailed description of the CLI changes. Please let me know if any further revisions are needed; otherwise, I’ll proceed with this version. Thank you!

- Replaced file-based config loading with the `config` crate. - Config file must now be specified with `-c <CONFIG_PATH>` - `--key` and `--cert` are now required as CLI options instead of being set in the config file

sehkone · 2025-04-30T00:10:43Z

The title now says Config, but that's not quite right. First, the official name is config, not Config. Second, Config looks like it refers to the Config struct provided by the config crate. We need to be careful when capitalizing the names of software or libraries. Some start with a capital letter, but others don't.

Cc. @danbi2990 @sophie-cluml

danbi2990 · 2025-04-30T00:58:13Z

src/settings.rs

+}
+
+impl Database {
+    pub(crate) fn get_path(&self) -> &Path {


I'm not sure this method is necessary.
If it is, the method name should be something other than get_path, in accordance with our naming guidelines. You can find details in Notion by searching for "get".

Thank you. I removed this function.

danbi2990 · 2025-04-30T01:01:22Z

src/settings.rs

+    pub(crate) key: PathBuf,
+}
+
+#[derive(Clone, Debug, Serialize, Deserialize)]


All Clone traits in this file are unnecessary.

danbi2990 · 2025-04-30T01:04:30Z

src/settings.rs

+        let config_path_str = config_path
+            .to_str()
+            .ok_or_else(|| ConfigError::Message("Invalid config path".into()))?;
+
+        let settings = default_config_builder()
+            .add_source(File::with_name(config_path_str))


#130 (comment)
As I suggested, the conversion from &Path to &str is unnecessary if we use File::from method.

I'm sorry for overlooking that. Thank you for your meticulous code review.

src/settings.rs

danbi2990 · 2025-04-30T01:10:20Z

src/settings.rs

+{
+    let addr = String::deserialize(deserializer)?;
+    addr.parse()
+        .map_err(|e| D::Error::custom(format!("invalid address \"{addr}\": {e}")))


Let's maintain consistency in error messages by capitalizing the first letter.

danbi2990 · 2025-05-07T02:07:05Z

src/settings.rs

+    pub(crate) address: SocketAddr,
+}
+
+#[derive(Clone, Debug, Serialize, Deserialize)]


Clone trait seems unnecessary.

danbi2990 · 2025-05-07T02:41:39Z

How about making commit message a bit more meaningful?
The current message does not seem to clearly reflect the intent of the commit.
Using the PR title as the commit message (with lower case config, as Kone mentioned) might be sufficient and more descriptive.
Additionally, it would be helpful to include "Closes #{issue_number}" in the commit body for better traceability.

danbi2990 · 2025-05-09T02:41:55Z

src/settings.rs

+    config::Config::builder()
+        .set_default("address", DEFAULT_ADDR)
+        .expect("valid default address")
+        .set_default("database", DEFAULT_DATABASE_NAME)


The default value for the database does not work. It should be set using "database.db_path".

danbi2990 · 2025-05-09T04:07:22Z

README.md

@@ -30,8 +28,6 @@ db_path = "db_path"
 ```

 - `address`: The address of web server.


Maybe we could add a bit more detail about what the default value is and how it works?
The same applies to db_path.

danbi2990 · 2025-05-13T04:50:53Z

README.md

  - To provide an SSH passphrase, set the `SSH_PASSPHRASE` environment variable.
- `db_path`: The path to the database for creation/connection.
+- `db_path`: Folder where the sled database files are stored. Created
+  automatically if it doesn’t exist. (Default: github-dashboard)


I believe the default value for db_path should be absolute path - or it might be better not to provide a default at all.
It's not a big issue in debug builds since we typically run the program from the project root. However, in release builds, the working directory is not guaranteed, which can lead to the database being created in arbitrary locations.

I believe the default value for db_path should be absolute path - or it might be better not to provide a default at all.

@danbi2990, could you explain why db_path needs to be an absolute path?

Our software—like any other—should work with both absolute and relative paths. Software generally does not assume that a path is always absolute.

I'm not saying that db_path must be an absolute path in all cases, but I believe the default value for db_path should be an absolute path.

The reason is to ensure a consistent and predictable database location. If the default is relative to the working directory, a new database might be created in different locations depending on where the program is run. I'd appreciate your thoughts on this.

I still disagree with the idea that default paths should be absolute.

Of course, using absolute paths is preferable during deployment to avoid unexpected outcomes. However, when software suggests default path values, they are typically not absolute. Using absolute paths belongs to the domain of deployment and operations—not the software codebase itself.

For that reason, general documentation such as README.md tends to use relative paths as defaults, unless the path has a specific or special meaning. This also aligns with the decoupling principle: the software should not assume or be tightly bound to specific environments or file system layouts.

Okay, I see your point. Let's keep it as a relative path. Thank you.

danbi2990

@pott-cho
Could you rebase this onto the main branch?

pott-cho · 2025-05-14T08:11:24Z

@danbi2990
I've completed the rebase.

danbi2990 · 2025-05-14T08:16:02Z

@sophie-cluml Could you also review this PR?

sophie-cluml · 2025-05-16T11:00:01Z

README.md

- `address`: The address of web server.
- `key`: The TLS key path for the web server.
- `cert`: The TLS certificate path for the web server.
+- `address`: IP address and port the web server listens on. (Default: 127.0.0.1:8000)


마크다운파일에 대한 VS code 포매팅 세팅이 최신이 아닌 것 같습니다. 확인 부탁드립니다.

I fixed it.

sophie-cluml · 2025-05-16T11:02:44Z

src/checkout.rs

 use tracing::{error, info};

-use crate::conf::RepoInfo;
+use crate::settings::Repository as RepoInfo;


굳이 alias로 use 할 이유가 없는 상황 같아서 alias 없애면 어떨까요?

If I remove the alias, a conflict will occur between git2::{Cred, FetchOptions, RemoteCallbacks, Repository} and crate::settings::Repository.

Thank you for clarification, certainly I overlooked that conflict.

sophie-cluml · 2025-05-16T11:05:43Z

src/settings.rs

+    }
+}
+
+/// Creates a new `ConfigBuilder` instance with the default configuration.


/// 는 rustdoc comment 이므로, private function에는 사실상 ineffective한 효과를 발생시킬 것 같습니다. 이 함수를 설명해주고 싶다면 단순 // comment로 하는 것을 제안드리고 싶습니다. 한편, 코드로 충분히 다 전달이 되는 내용을 반복할 뿐이라면 주석이 없는 것도 하나의 방향일 것 같습니다.

I removed this.

sophie-cluml · 2025-05-22T08:24:49Z

CHANGELOG.md

+- Config file must now be specified with `-c <CONFIG_PATH>`
+- `--key` and `--cert` are now required as CLI options instead of being set in
+  the config file


문장 뒤에 마침표 부탁드립니다.

sophie-cluml · 2025-05-22T08:24:53Z

README.md


-To run the application without specifying the configuration file path, save the
-file to one of the following locations:
+<!-- markdownlint-disable -->


요거 꼭 필요한가요?

danbi2990 · 2025-05-28T00:53:23Z

@pott-cho Could you rebase it onto main please?

Closes #106

sophie-cluml requested a review from danbi2990 April 22, 2025 14:39

danbi2990 reviewed Apr 25, 2025

View reviewed changes

README.md Show resolved Hide resolved

src/settings.rs Outdated Show resolved Hide resolved

src/settings.rs Outdated Show resolved Hide resolved

pott-cho force-pushed the pott/settings branch from 7e4df17 to 14d227c Compare April 28, 2025 04:51

danbi2990 reviewed Apr 29, 2025

View reviewed changes

src/settings.rs Outdated Show resolved Hide resolved

src/settings.rs Outdated Show resolved Hide resolved

sehkone reviewed Apr 29, 2025

View reviewed changes

danbi2990 reviewed Apr 30, 2025

View reviewed changes

pott-cho force-pushed the pott/settings branch 2 times, most recently from f626cd5 to 8d59ec8 Compare April 30, 2025 04:53

danbi2990 reviewed May 7, 2025

View reviewed changes

pott-cho force-pushed the pott/settings branch from 8d59ec8 to dee9861 Compare May 8, 2025 01:04

danbi2990 reviewed May 9, 2025

View reviewed changes

pott-cho force-pushed the pott/settings branch 2 times, most recently from 2480f37 to c5503a1 Compare May 12, 2025 04:59

danbi2990 reviewed May 13, 2025

View reviewed changes

danbi2990 approved these changes May 14, 2025

View reviewed changes

pott-cho force-pushed the pott/settings branch from c5503a1 to 072272d Compare May 14, 2025 08:04

danbi2990 requested a review from sophie-cluml May 14, 2025 08:15

sophie-cluml reviewed May 16, 2025

View reviewed changes

pott-cho force-pushed the pott/settings branch from 072272d to 841801c Compare May 19, 2025 00:38

danbi2990 requested a review from sophie-cluml May 21, 2025 00:29

sophie-cluml reviewed May 23, 2025

View reviewed changes

pott-cho force-pushed the pott/settings branch from 841801c to 56c600e Compare May 26, 2025 00:24

sophie-cluml approved these changes May 27, 2025

View reviewed changes

Parsing a config file with config

875cee9

Closes #106

pott-cho force-pushed the pott/settings branch from 56c600e to 875cee9 Compare May 28, 2025 04:31

danbi2990 merged commit 803a0a6 into main May 28, 2025
11 of 12 checks passed

danbi2990 deleted the pott/settings branch May 28, 2025 04:37

pott-cho mentioned this pull request May 30, 2025

Refactor settings error handling #172

Closed

		@@ -30,8 +28,6 @@ db_path = "db_path"
		```

		- `address`: The address of web server.

Conversation

pott-cho commented Apr 22, 2025

Uh oh!

danbi2990 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

danbi2990 commented Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sehkone commented Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danbi2990 commented May 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sehkone May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danbi2990 left a comment

Choose a reason for hiding this comment

Uh oh!

pott-cho commented May 14, 2025

Uh oh!

danbi2990 commented May 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pott-cho May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pott-cho May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

codecov bot commented Apr 28, 2025 •

edited

Loading

danbi2990 commented Apr 29, 2025 •

edited

Loading

sehkone commented Apr 30, 2025 •

edited

Loading

sehkone May 13, 2025 •

edited

Loading

pott-cho May 19, 2025 •

edited

Loading

pott-cho May 19, 2025 •

edited

Loading