apple / foundationdb

FoundationDB - the open source, distributed, transactional key-value store
https://apple.github.io/foundationdb/
Apache License 2.0
14.33k stars 1.3k forks source link

Unify fdbcli flags #4817

Closed johscheuer closed 2 years ago

johscheuer commented 3 years ago

fdbcli flags currently contain a mixture of _ and - (probably for historic reasons). I would propose to unify the flags and only use either _ or - personally I tend to hyphens like recommended in the POSXI standard (https://www.gnu.org/software/libc/manual/html_node/Argument-Syntax.html). Current fdbcli help output:

FoundationDB CLI 6.2 (v6.2.29)
usage: /usr/bin/fdb/6.2/fdbcli [OPTIONS]

  -C CONNFILE    The path of a file containing the connection string for the
                 FoundationDB cluster. The default is first the value of the
                 FDB_CLUSTER_FILE environment variable, then `./fdb.cluster',
                 then `/etc/foundationdb/fdb.cluster'.
  --log          Enables trace file logging for the CLI session.
  --log-dir PATH Specifes the output directory for trace files. If
                 unspecified, defaults to the current directory. Has
                 no effect unless --log is specified.
  --trace_format FORMAT
                 Select the format of the log files. xml (the default) and json
                 are supported. Has no effect unless --log is specified.
  --exec CMDS    Immediately executes the semicolon separated CLI commands
                 and then exits.
  --no-status    Disables the initial status check done when starting
                 the CLI.
  --tls_certificate_file CERTFILE
                 The path of a file containing the TLS certificate and CA
                 chain.
  --tls_ca_file CERTAUTHFILE
                 The path of a file containing the CA certificates chain.
  --tls_key_file KEYFILE
                 The path of a file containing the private key corresponding
                 to the TLS certificate.
  --tls_password PASSCODE
                 The passphrase of encrypted private key
  --tls_verify_peers CONSTRAINTS
                 The constraints by which to validate TLS peers. The contents
                 and format of CONSTRAINTS are plugin-specific.
  --knob_KNOBNAME KNOBVALUE
                 Changes a knob option. KNOBNAME should be lowercase.
  --debug-tls    Prints the TLS configuration and certificate chain, then exits.
                 Useful in reporting and diagnosing TLS issues.
  -v, --version  Print FoundationDB CLI version information and exit.
  -h, --help     Display this help and exit.

I think a consistent way will improve the user experience.

sfc-gh-abeamon commented 3 years ago

I too prefer hyphens, though it's worth noting that I think underscores tend to be the predominant choice in our other binaries (e.g. fdbserver, fdbbackup). Probably either way we'll need to include both options for a while for any parameter we change, so maybe choosing to switch isn't too big a deal.

johscheuer commented 3 years ago

Oh right, I didn't mentioned the other binaries but in general I think it's worth unifying all flags everything else is confusing (at least for me). 👍 on keeping both variants for a while in the commands, we probably want to print out a warning if someone uses an "old" version. I'm not sure about the version/compatibility guarantees but I guess we want to keep both versions until we release a new major version (if we want to be compatible with semver).

sfc-gh-abeamon commented 3 years ago

@sfc-gh-mpilman Proposed changing the parser to treat hyphen and underscore as the same and the updating the documentation to use all hyphens.

johscheuer commented 3 years ago

Cool @jzhou77 I would take a look and fix that, sounds like a reasonable part to work on.

sfc-gh-mpilman commented 3 years ago

I think the easiest way for fixing this would be to change all hyphens to underscores before passing it to the argument parser and then change the help text to only use hyphens. The reason it would be easier to only use underscores internally is because we have some weird composed argument types (like --knob_ and --locality) -- also not all arguments are documents (not sure whether this statement is also true for fdbcli -- it certainly is for fdbserver).

It would be great to do this for all command line tools (fdbserver, fdbcli, and fdbbackup and its variants). Then it would be consistent which would be great