search

TheThingsNetwork / lorawan-stack

The Things Stack, an Open Source LoRaWAN Network Server
https://www.thethingsindustries.com/stack/
Apache License 2.0
975 stars 306 forks source link

CLI's flags clash in end device commands #145

Closed htdvisser closed 5 years ago

htdvisser commented 5 years ago

Summary:

The CLI flag --application-server-address clashes for end-device list, get and set commands.

As reported on Slack by @Oliv4945

Steps to Reproduce: 

% ttn-lw-cli ed get foo foo --application-server-address="server.com:8884"

What do you see now?

invalid argument "server.com:8884" for "--application-server-address" flag: strconv.ParseBool: parsing "server.com:8884": invalid syntax
exit status 255

On set commands you may see something like below, as the --application-server-address is treated as a device field, not as CLI config.

  WARN grpc: addrConn.createTransport failed to connect to {localhost:8884 0  <nil>}. Err :connection error: desc = "transport: authentication handshake failed: x509: certificate is valid for remote.cli.net, not localhost". Reconnecting...

This problem was introduced when we started allowing flags to be specified with both dashes and underscores in https://github.com/TheThingsIndustries/lorawan-stack/pull/1498. As a result the flags --application_server_address (of the device) and --application-server-address (of the CLI itself) are now treated the same, and the CLI can no longer tell the difference.

What do you want to see instead?

We shouldn't have clashing flag names.

How do you propose to implement this?

We have a couple of options.

  1. Only allow config file and env to configure the CLI API addresses. You don't want to specify these flags on every command anyway.
  2. Renaming the the API address flags to something that won't occur in any of our entities.
  3. Support them both, but look at the position so that something like ttn-lw-cli --application-server-address="server:8885" end-devices get foo foo --application-server-address would work.

If we go for 1 or 2, then we should make sure that the other flags are never used in entities: ca, config, help, input-format, insecure, log.level, output-format.

I'm not sure if option 3 is supported by cobra/pflag

johanstokking commented 5 years ago
  1. Not very user friendly
  2. Best option if option 3 doesn't work
  3. I'd prefer this, although it may be confusing, this are rare circumstances

If we end up with 2 (or 1), I think it shouldn't be too hard to avoid collisions.