Connor/default config location #1494

connormullett · 2022-03-22T18:18:18Z

Summary of changes
Changes introduced in this pull request:

Forest will look for a default config file in $XDG_CONFIG_HOME (~/.config) or in the users home directory if not available.
If a config file is not passed to Forest when the node is started and a config file is not found, it will use a default configuration.

Reference issue to close (if applicable)

Closes

Other information and links

codecov · 2022-03-22T19:16:43Z

Codecov Report

Merging #1494 (33d8b8a) into main (cf49ba7) will increase coverage by 0.07%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1494      +/-   ##
==========================================
+ Coverage   44.30%   44.37%   +0.07%     
==========================================
  Files         323      319       -4     
  Lines       28414    28253     -161     
==========================================
- Hits        12588    12537      -51     
+ Misses      15826    15716     -110

Impacted Files	Coverage Δ
forest/src/cli/mod.rs	`0.00% <0.00%> (ø)`
blockchain/message_pool/src/msg_chain.rs	`80.24% <0.00%> (-0.41%)`	⬇️
vm/actor/src/builtin/miner/mod.rs	`33.49% <0.00%> (-0.05%)`	⬇️
types/src/lib.rs	`57.14% <0.00%> (ø)`
types/src/sector/post.rs	`27.27% <0.00%> (ø)`
types/src/sector/registered_proof.rs
types/src/sector/mod.rs
types/src/randomness.rs
encoding/src/bytes.rs

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf49ba7...33d8b8a. Read the comment docs.

lemmih · 2022-03-23T09:19:21Z

Let's use https://crates.io/crates/directories/ instead of querying XDG_CONFIG_HOME directly.

lemmih

If FOREST_CONFIG_PATH isn't available, use https://crates.io/crates/directories/ to query the config path in a cross platform way.

LesnyRumcajs · 2022-03-23T09:31:01Z

If FOREST_CONFIG_PATH isn't available, use https://crates.io/crates/directories/ to query the config path in a cross platform way.

I didn't know this one, looks pretty good. Probably we should use it in some other places around Forest we want to strive for being cross-platform without containterization.

LesnyRumcajs · 2022-03-23T09:37:50Z

forest/src/cli/mod.rs

@@ -224,6 +217,31 @@ impl CliOpts {
    }
 }

+pub fn find_default_config() -> Option<Config> {
+    // check forest config path first, then look at other default locations


From a user perspective, it may be nice to check all those places and display a piece of information if e.g. forest will use config from XDG_CONFIG_HOME and not what is there in HOME_DIR. Otherwise, we could at least document it somewhere in README.

Not sure how others feel about it @lemmih @elmattic

Yes, I think it would help to have some info log that displays to the user the loaded config file location.

And in case of doubt the user still can add config dump to command-line to see the computed config.

Added logging with config locations and errors that occured while trying to load the config. Resorts to default on error

LesnyRumcajs · 2022-03-23T09:45:24Z

forest/src/cli/mod.rs

+    };
+
+    if path.exists() {
+        let toml = match read_file_to_string(&path) {


Perhaps you could omit match and if let later on with ? and ok()??

Gets tricky with Result<> into Option<>, this is the simplest way i can see it currently. I've refactored to DRY and reduce repeated code

elmattic · 2022-03-23T10:00:05Z

If FOREST_CONFIG_PATH isn't available, use https://crates.io/crates/directories/ to query the config path in a cross platform way.

I didn't know this one, looks pretty good. Probably we should use it in some other places around Forest we want to strive for being cross-platform without containterization.

What are the pros of using directories here instead of the lower level dirs?

lemmih · 2022-03-23T10:05:19Z

If FOREST_CONFIG_PATH isn't available, use https://crates.io/crates/directories/ to query the config path in a cross platform way.

I didn't know this one, looks pretty good. Probably we should use it in some other places around Forest we want to strive for being cross-platform without containterization.

What are the pros of using directories here instead of the lower level dirs?

It's easier to follow OS conventions with directories. On mac and windows, we don't just want to create a forest folder in the config directory.

forest/src/cli/mod.rs

LesnyRumcajs

Let's put the available default locations and their priority to the config section of README, what do you think?

LesnyRumcajs · 2022-03-24T08:27:01Z

forest/src/cli/mod.rs

+            None => match find_default_config() {
+                Some(cfg) => cfg,
+                None => Config::default(),
+            },


Suggested change

None => match find_default_config() {

Some(cfg) => cfg,

None => Config::default(),

},

None => find_default_config().unwrap_or_default()

I think that's a good idea, will sneak it in now

forest/src/cli/mod.rs

LesnyRumcajs

Thanks! It's really useful!

connormullett added 7 commits March 15, 2022 14:03

scaffold default config path

71e3774

slight refactoring; thinking through chunks of config

f053b5b

debugging default config

69f445a

refactor default config search

76aa639

finish logic for default config

bdf80ee

clippy lint

991c30f

Merge branch 'main' into connor/default-config-location

661f28a

connormullett requested review from ec2, noot, elmattic, lemmih and LesnyRumcajs as code owners March 22, 2022 18:18

lemmih requested changes Mar 23, 2022

View reviewed changes

LesnyRumcajs requested changes Mar 23, 2022

View reviewed changes

elmattic reviewed Mar 23, 2022

View reviewed changes

forest/src/cli/mod.rs Outdated Show resolved Hide resolved

connormullett added 2 commits March 23, 2022 15:50

refactor to use cross compatible config/home dirs

85590f7

remove home dir; opt for project dirs as requested

10d2c10

LesnyRumcajs reviewed Mar 24, 2022

View reviewed changes

suggested change; update readme

96ca092

lemmih requested changes Mar 24, 2022

View reviewed changes

forest/src/cli/mod.rs Outdated Show resolved Hide resolved

remove redundant path prefix

33d8b8a

lemmih approved these changes Mar 24, 2022

View reviewed changes

LesnyRumcajs approved these changes Mar 25, 2022

View reviewed changes

lemmih merged commit 1bad495 into main Mar 25, 2022

lemmih deleted the connor/default-config-location branch March 25, 2022 08:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Connor/default config location #1494

Connor/default config location #1494

connormullett commented Mar 22, 2022

codecov bot commented Mar 22, 2022 •

edited

lemmih commented Mar 23, 2022

lemmih left a comment

LesnyRumcajs commented Mar 23, 2022

LesnyRumcajs Mar 23, 2022

LesnyRumcajs Mar 23, 2022

elmattic Mar 23, 2022

elmattic Mar 23, 2022

connormullett Mar 23, 2022

LesnyRumcajs Mar 23, 2022

connormullett Mar 23, 2022

elmattic commented Mar 23, 2022

lemmih commented Mar 23, 2022

LesnyRumcajs left a comment

LesnyRumcajs Mar 24, 2022

connormullett Mar 24, 2022

LesnyRumcajs left a comment

Connor/default config location #1494

Connor/default config location #1494

Conversation

connormullett commented Mar 22, 2022

codecov bot commented Mar 22, 2022 • edited

Codecov Report

lemmih commented Mar 23, 2022

lemmih left a comment

Choose a reason for hiding this comment

LesnyRumcajs commented Mar 23, 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

elmattic commented Mar 23, 2022

lemmih commented Mar 23, 2022

LesnyRumcajs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LesnyRumcajs left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 22, 2022 •

edited