cmd: improve user interface for help message by add default text #456

adamqqqplay · 2023-04-24T09:07:11Z

pkg: introduce internal/constant package
cmd: improve user interface for help message by add default text

Current help message:

NAME:
   containerd-nydus-grpc - Nydus remote snapshotter for containerd

USAGE:
   containerd-nydus-grpc [global options] command [command options] [arguments...]

COMMANDS:
   help, h  Shows a list of commands or help for one command

GLOBAL OPTIONS:
   --root value                                directory to store snapshotter data and working states (default: /var/lib/containerd-nydus)
   --address value                             remote snapshotter gRPC socket path (default: /run/containerd-nydus/containerd-nydus-grpc.sock)
   --config value                              path to nydus-snapshotter configuration (such as: config.toml)
   --nydus-image nydus-image                   path to nydus-image binary, default to search in $PATH (such as: /usr/local/bin/nydus-image)
   --nydusd nydusd                             path to nydusd binary, default to search in $PATH (such as: /usr/local/bin/nydusd)
   --nydusd-config value, --config-path value  path to nydusd configuration (such as: nydusd-config.json or nydusd-config-v2.toml) (default: /etc/nydus/nydusd-config.json)
   --daemon-mode value                         nydusd daemon working mode, possible values: "multiple", "shared" or "none" (default: multiple)
   --fs-driver value                           driver to mount RAFS filesystem, possible values: "fusedev", "fscache" (default: fusedev)
   --log-level value                           logging level, possible values: "trace", "debug", "info", "warn", "error" (default: info)
   --log-to-stdout                             print log messages to standard output (default: false)
   --version                                   print version and build information (default: false)
   --help, -h                                  show help

codecov-commenter · 2023-04-24T09:18:39Z

Codecov Report

Merging #456 (e835623) into main (2455464) will increase coverage by 0.05%.
The diff coverage is 88.88%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #456      +/-   ##
==========================================
+ Coverage   37.01%   37.07%   +0.05%     
==========================================
  Files          60       60              
  Lines        6886     6892       +6     
==========================================
+ Hits         2549     2555       +6     
  Misses       4042     4042              
  Partials      295      295

Impacted Files	Coverage Δ
config/config.go	`34.23% <ø> (ø)`
pkg/daemon/config.go	`0.00% <0.00%> (ø)`
config/default.go	`81.25% <86.66%> (ø)`
internal/flags/flags.go	`100.00% <100.00%> (ø)`

imeoer · 2023-04-27T02:35:42Z

internal/flags/flags.go

 			Destination: &args.SnapshotterConfigPath,
 		},
 		&cli.StringFlag{
 			Name:        "nydusd-config",
 			Aliases:     []string{"config-path"},
-			Usage:       "path to the nydusd configuration",
+			Usage:       "path to nydusd configuration (such as: nydusd-config.json or configuration_v2.toml)",


nydusd-config.json or nydusd-config-v2.toml

changweige · 2023-05-05T02:01:09Z

cmd/containerd-nydus-grpc/main.go

@@ -94,7 +94,7 @@ func main() {
 				return errors.Wrap(err, "set up logger")
 			}

-			log.L.Infof("Start nydus-snapshotter. PID %d Version %s FsDriver %s DaemonMode %s",
+			log.L.Infof("Start nydus-snapshotter. PID: %d Version :%s FsDriver: %s DaemonMode: %s",


Should we make the colon very close to "Version" like Version: PID: ?

OK, I will fix it.

changweige · 2023-05-05T02:04:48Z

internal/flags/flags.go

 			Destination: &args.RootDir,
+			DefaultText: "/var/lib/containerd-nydus",


This can't work and may violate the current theory of how nydus-snapshotter merges configurations from 1. default values, 2. TOML configuration file 3. CLI parameters. The way assigning a default value to CLI parameters will always override values from TOML configuration file

@changweige "DefaultText" will not affect the parsing of parameters, it is just a prompt message to let users know the value when no CLI parameters and configuration files are specified. The "Value" field is the real default value of the CLI.

Please refer to: https://cli.urfave.org/v2/examples/flags/#default-values-for-help-output

I see. Thanks for your elaboration. Can we use the default values defined in config/default.go as the DefaultText value? In order to avoid cyclic dependency, we can move function ParseParameters to flags package?

I see. Thanks for your elaboration. Can we use the default values defined in config/default.go as the DefaultText value? In order to avoid cyclic dependency, we can move function ParseParameters to flags package?

Good idea, I will try to do it.

changweige

In summary, we can't give a default value for CLI parameters unless we can judge if it is ever specified by users in ParseParameters()

changweige

Need rebase 🥹

changweige · 2023-05-06T03:48:59Z

internal/constant/cmd.go

+
+package constant
+
+const (


Why the source file is named as cmd.go ?

I think the constants in this file are prepared for the cmd and config package, what name do you think is better?

how about values.go or constants.go?

values.go looks good.

Move some string constants in CLI config to a new package for easier future refactoring. Signed-off-by: Qinqi Qu <quqinqi@linux.alibaba.com>

1. Fix missing default values in the --help after v0.6.1. 2. Sort out some usage information. Signed-off-by: Qinqi Qu <quqinqi@linux.alibaba.com>

changweige · 2023-05-06T06:24:48Z

@sctb512 Please check the optimizer E2E test. It randomly fails, which is somewhat upset.

changweige

LGTM

adamqqqplay force-pushed the improve-help-message branch from 7216120 to 4521807 Compare April 24, 2023 09:08

adamqqqplay force-pushed the improve-help-message branch 2 times, most recently from 72a19f5 to b0bfbf2 Compare April 25, 2023 08:11

imeoer reviewed Apr 27, 2023

View reviewed changes

adamqqqplay force-pushed the improve-help-message branch from b0bfbf2 to 9c479a3 Compare April 27, 2023 06:22

adamqqqplay requested a review from imeoer April 28, 2023 08:00

imeoer approved these changes Apr 28, 2023

View reviewed changes

changweige reviewed May 5, 2023

View reviewed changes

changweige requested changes May 5, 2023

View reviewed changes

changweige reviewed May 6, 2023

View reviewed changes

adamqqqplay force-pushed the improve-help-message branch from 9c479a3 to b731369 Compare May 6, 2023 03:33

changweige reviewed May 6, 2023

View reviewed changes

adamqqqplay added 2 commits May 6, 2023 14:14

pkg: introduce internal/constant package

e80f79c

Move some string constants in CLI config to a new package for easier future refactoring. Signed-off-by: Qinqi Qu <quqinqi@linux.alibaba.com>

cmd: improve user interface for help message by add default text

e835623

1. Fix missing default values in the --help after v0.6.1. 2. Sort out some usage information. Signed-off-by: Qinqi Qu <quqinqi@linux.alibaba.com>

adamqqqplay force-pushed the improve-help-message branch from b731369 to e835623 Compare May 6, 2023 06:17

changweige approved these changes May 6, 2023

View reviewed changes

changweige merged commit e732202 into containerd:main May 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd: improve user interface for help message by add default text #456

cmd: improve user interface for help message by add default text #456

adamqqqplay commented Apr 24, 2023 •

edited

Loading

codecov-commenter commented Apr 24, 2023 •

edited

Loading

imeoer Apr 27, 2023

adamqqqplay Apr 27, 2023

changweige May 5, 2023

adamqqqplay May 5, 2023

changweige May 5, 2023

adamqqqplay May 5, 2023

changweige May 6, 2023

adamqqqplay May 6, 2023

changweige left a comment •

edited

Loading

changweige left a comment

changweige May 6, 2023

adamqqqplay May 6, 2023

changweige May 6, 2023

adamqqqplay May 6, 2023

changweige commented May 6, 2023

changweige left a comment

		Destination: &args.RootDir,
		DefaultText: "/var/lib/containerd-nydus",

cmd: improve user interface for help message by add default text #456

cmd: improve user interface for help message by add default text #456

Conversation

adamqqqplay commented Apr 24, 2023 • edited Loading

codecov-commenter commented Apr 24, 2023 • edited Loading

Codecov Report

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

changweige left a comment • edited Loading

Choose a reason for hiding this comment

changweige 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

changweige commented May 6, 2023

changweige left a comment

Choose a reason for hiding this comment

adamqqqplay commented Apr 24, 2023 •

edited

Loading

codecov-commenter commented Apr 24, 2023 •

edited

Loading

changweige left a comment •

edited

Loading