Implement Correct Usage Flags for `forest` and `forest-cli` #2175

jdjaustin · 2022-11-10T15:36:56Z

Summary of changes
Changes introduced in this pull request:

Change flags available for forest and forest-cli to only those that make sense and are usable.

Reference issue to close (if applicable)

Closes associated tasks in issue #2076

Other information and links

Task to change the config dump behavior will be covered under a separate PR.

…flags

forest/shared/src/cli/mod.rs

…flags

forest/shared/src/cli/mod.rs

lemmih

Getting closer. You still need to review all the cli options, though. It's not too difficult to see if a command-line option is used or not. You can either use git grep or simply delete the field and see what breaks.

forest/shared/src/cli/mod.rs

…flags

lemmih

There's a lot more that can be done to clean up the command-line handling for forest and forest-cli but this is a good start.

forest/shared/src/cli/mod.rs

LesnyRumcajs · 2022-11-23T16:32:12Z

forest/shared/src/cli/mod.rs

+    /// A TOML file containing relevant configurations
+    #[structopt(short, long)]
+    pub config: Option<String>,


It would be nice to take this opportunity and change Option<String> to Option<PathBuf>. Better-typed variables are better than comments saying that this should be a path.

LesnyRumcajs · 2022-11-23T16:37:33Z

forest/shared/src/cli/mod.rs

+    /// Address used for metrics collection server. By defaults binds on localhost on port 6116.
+    #[structopt(long)]
+    pub metrics_address: Option<SocketAddr>,


I don't think we use it anywhere in the forest-cli unless I'm missing something.

Suggested change

/// Address used for metrics collection server. By defaults binds on localhost on port 6116.

#[structopt(long)]

pub metrics_address: Option<SocketAddr>,

LesnyRumcajs · 2022-11-23T16:38:36Z

forest/shared/src/cli/mod.rs

+    // env_logger-0.7 can only redirect to stderr or stdout. Version 0.9 can redirect to a file.
+    // However, we cannot upgrade to version 0.9 because pretty_env_logger depends on version 0.7
+    // and hasn't been updated in quite a while. See https://github.com/seanmonstar/pretty-env-logger/issues/52
+    // #[structopt(
+    // help = "Specify a filename into which logging should be appended"
+    // )]
+    // pub log_file: Option<PathBuf>,


Suggested change

// env_logger-0.7 can only redirect to stderr or stdout. Version 0.9 can redirect to a file.

// However, we cannot upgrade to version 0.9 because pretty_env_logger depends on version 0.7

// and hasn't been updated in quite a while. See https://github.com/seanmonstar/pretty-env-logger/issues/52

// #[structopt(

// help = "Specify a filename into which logging should be appended"

// )]

// pub log_file: Option<PathBuf>,

LesnyRumcajs · 2022-11-23T16:42:53Z

forest/shared/src/cli/mod.rs

+
+impl CliOpts {
+    pub fn to_config(&self) -> Result<(Config, Option<ConfigPath>), anyhow::Error> {
+        let path = find_config_path(&self.config);


This should not be shared between the forest and forest-cli. Imagine having a config file in the default location. If it is forest config file, forest-cli would fail on start. If it is a forest-cli config file, forest would die. In general, it'd be fantastic if those configurations, at least user-facing ones, would live in separate modules, not visible to each other.

There's no such thing as a forest-cli config, though, and there probably never will be.

Given that there is apparently no consensus about this, I think we should close this PR and move the discussion to an issue.

How come there isn't? The CliOpts is the forest-cli config, it's what gets shown to the user via structopt, or am I missing something? If we don't want to have it and implement every flag at subcommand level, let's just get rid of this the CliOpts in this PR.

Command-line options are different from a configuration file. In forest they are the same (which is a mistake, I believe, as it leads to some unintuitive behaviour). But for forest-cli there is no reason to have a configuration file.

Let's discuss in a separate issue.

I'm okay with having no configuration file for forest-cli and removing this logic. What I am not okay with is having the same logic for the configuration file between forest and forest-cli.

Why not? We want the forest-cli tool to read the configuration file for the forest node, don't we?

I mean the same logic for loading two different files, including their default location. :)

LesnyRumcajs · 2022-11-23T16:43:35Z

forest/shared/src/cli/mod.rs

+        if let Some(metrics_address) = self.metrics_address {
+            cfg.client.metrics_address = metrics_address;
+        }
+


Why do we need a metrics server in the forest-cli?

Suggested change

if let Some(metrics_address) = self.metrics_address {

cfg.client.metrics_address = metrics_address;

}

Co-authored-by: Hubert <hubert@chainsafe.io>

LesnyRumcajs · 2022-11-23T17:01:28Z

One other thing, DaemonOpts should not have the token field. As mentioned in the comment above it, it's for the client, i.e., forest-cli.

jdjaustin added 6 commits November 10, 2022 09:10

draft cli flag changes

8ebb060

changes to satisfy cargo test

a1c04ba

Merge branch 'main' into jdjaustin/issue-2076-rm-incorrect-cli-usage-…

5a13188

…flags

Merge branch 'main' into jdjaustin/issue-2076-rm-incorrect-cli-usage-…

3142d79

…flags

Merge branch 'main' into jdjaustin/issue-2076-rm-incorrect-cli-usage-…

3e94092

…flags

Merge branch 'main' into jdjaustin/issue-2076-rm-incorrect-cli-usage-…

edb7fe3

…flags

jdjaustin changed the title ~~[WIP] Implement Correct Usage Flags for forest and forest-cli~~ Implement Correct Usage Flags for forest and forest-cli Nov 16, 2022

jdjaustin marked this pull request as ready for review November 16, 2022 16:16

jdjaustin requested review from elmattic, lemmih, LesnyRumcajs, tyshko5, hanabi1224, creativcoder and sudo-shashank as code owners November 16, 2022 16:16

lemmih requested changes Nov 17, 2022

View reviewed changes

lemmih added the Status: Needs Changes label Nov 17, 2022

jdjaustin added 4 commits November 18, 2022 14:33

Merge branch 'main' into jdjaustin/issue-2076-rm-incorrect-cli-usage-…

d4c5b04

…flags

fix strange format changes

199ff2f

merge find_config_path fns

0f00561

fix lint error

d3cf26f

lemmih requested changes Nov 21, 2022

View reviewed changes

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

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

resolve redundant find_config_path logic

8c1df76

lemmih requested changes Nov 22, 2022

View reviewed changes

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

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

jdjaustin added 4 commits November 22, 2022 09:28

Merge branch 'main' into jdjaustin/issue-2076-rm-incorrect-cli-usage-…

c33f9a8

…flags

code review changes

7e911c2

Merge branch 'main' into jdjaustin/issue-2076-rm-incorrect-cli-usage-…

9739af7

…flags

rm rpc flag from CliOpts

71fef83

jdjaustin removed the Status: Needs Changes label Nov 22, 2022

Merge branch 'main' into jdjaustin/issue-2076-rm-incorrect-cli-usage-…

81931d6

…flags

jdjaustin added the Status: Needs Review label Nov 23, 2022

lemmih approved these changes Nov 23, 2022

View reviewed changes

LesnyRumcajs requested changes Nov 23, 2022

View reviewed changes

Update forest/shared/src/cli/mod.rs

6bf639b

Co-authored-by: Hubert <hubert@chainsafe.io>

lemmih closed this Nov 24, 2022

LesnyRumcajs deleted the jdjaustin/issue-2076-rm-incorrect-cli-usage-flags branch April 25, 2023 09:56

LesnyRumcajs mentioned this pull request Jul 13, 2023

Tracking issue: Forest executable re-organization #3172

Closed

31 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Correct Usage Flags for `forest` and `forest-cli` #2175

Implement Correct Usage Flags for `forest` and `forest-cli` #2175

jdjaustin commented Nov 10, 2022 •

edited

Loading

lemmih left a comment

lemmih left a comment

LesnyRumcajs Nov 23, 2022

LesnyRumcajs Nov 23, 2022

LesnyRumcajs Nov 23, 2022

LesnyRumcajs Nov 23, 2022

lemmih Nov 23, 2022

LesnyRumcajs Nov 23, 2022

lemmih Nov 23, 2022

LesnyRumcajs Nov 23, 2022

lemmih Nov 23, 2022

LesnyRumcajs Nov 23, 2022

LesnyRumcajs Nov 23, 2022

LesnyRumcajs commented Nov 23, 2022

	/// Address used for metrics collection server. By defaults binds on localhost on port 6116.
	#[structopt(long)]
	pub metrics_address: Option<SocketAddr>,

	if let Some(metrics_address) = self.metrics_address {
	cfg.client.metrics_address = metrics_address;
	}

Implement Correct Usage Flags for forest and forest-cli #2175

Implement Correct Usage Flags for forest and forest-cli #2175

Conversation

jdjaustin commented Nov 10, 2022 • edited Loading

lemmih left a comment

Choose a reason for hiding this comment

lemmih left a 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

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

LesnyRumcajs commented Nov 23, 2022

Implement Correct Usage Flags for `forest` and `forest-cli` #2175

Implement Correct Usage Flags for `forest` and `forest-cli` #2175

jdjaustin commented Nov 10, 2022 •

edited

Loading