Fix update of flags from the environment

KrishnaIyer commented 6 months ago

Summary

Refs https://github.com/TheThingsNetwork/lorawan-stack-migrate/pull/112#discussion_r1519974219

Changes

For the ChirpStack, Wanesy and Firefly sources, make sure that env is used only when set.
Refactor the tts config to match other sources and don't print defaults to the env.

Testing

TTS

Check that secrets are not printed.

./main tts device --help
Export devices by Device ID

Usage:
  ttn-lw-migrate tts device ... [flags]

Aliases:
  device, end-devices, end-device, devices, devs, dev, d

Flags:
      --app-api-key string                       TTS Application Access Key (with 'devices' permissions)
      --app-id string                            TTS Application ID
      --application-server-grpc-address string   TTS Application Server GRPC Address
      --ca-file string                           TTS Path to a CA file (optional)
      --default-grpc-address string              TTS default GRPC Address (optional)
      --delete-source-device                     TTS delete exported devices
  -h, --help                                     help for device
      --identity-server-grpc-address string      TTS Identity Server GRPC Address
      --insecure                                 TTS allow TCP connection
      --join-server-grpc-address string          TTS Join Server GRPC Address
      --network-server-grpc-address string       TTS Network Server GRPC Address
      --no-session                               TTS export devices without session

Global Flags:
      --dev-id-prefix string         (optional) value to be prefixed to the resulting device IDs
      --dry-run                      Do everything except resetting root and session keys of exported devices
      --frequency-plans-url string   URL for fetching frequency plans (default "https://raw.githubusercontent.com/TheThingsNetwork/lorawan-frequency-plans/master")
      --verbose                      Verbose output

Others

Check that env is only used when set.

./main chirpstack device 0004a30b001bc8a7 --dry-run --api-key test --api-url localhost:8081
Error: connection error: desc = "transport: error while dialing: dial tcp 127.0.0.1:8081: connect: connection refused"
connection error: desc = "transport: error while dialing: dial tcp 127.0.0.1:8081: connect: connection refused"
    correlation_id=0fad85a72890410c82abb6964662db80

export CHIRPSTACK_API_URL="localhost:8080"

./main chirpstack device 0004a30b001bc8a7 --dry-run --api-key test --api-url localhost:8081
Error: connection error: desc = "transport: error while dialing: dial tcp 127.0.0.1:8080: connect: connection refused"
connection error: desc = "transport: error while dialing: dial tcp 127.0.0.1:8080: connect: connection refused"
    correlation_id=0fad85a72890410c82abb6964662db80

Regressions

Tested export using the tts source.

./main tts device 'test-device-ade' --dry-run
{"ids":{"device_id":"test-device-ade","application_ids":{"application_id":"sec"},"dev_eui":"70B3D57ED8002A47","join_eui":"AFBAD1928EEE2938"},"created_at":"2024-03-13T10:46:57.421090786Z","updated_at":"2024-03-13T10:46:57.421090786Z","version_ids":{"brand_id":"adeunis","model_id":"analog","hardware_version":"1.04","firmware_version":"1.03","band_id":"EU_863_870"},"lorawan_version":"MAC_V1_0_2","lorawan_phy_version":"PHY_V1_0_2_REV_A","frequency_plan_id":"EU_863_870_TTN","supports_join":true,"root_keys":{"app_key":{"key":"8B9C4D0EE0AF31371BBDEFAAA14F2A63"}},"mac_settings":{"supports_32_bit_f_cnt":true},"formatters":{"up_formatter":"FORMATTER_REPOSITORY","down_formatter":"FORMATTER_REPOSITORY"}}

Haven't tested ttnv2 since that's not so easy.

Checklist

[x] Scope: The referenced issue is addressed, there are no unrelated changes.
[ ] Documentation: Relevant documentation is added or updated.
[x] Changelog: Significant features, behavior changes, deprecations and fixes are added to CHANGELOG.md.

KrishnaIyer commented 6 months ago

Ok for some reason I always thought that it's a general convention that env takes preference over flags, but that's not the case: https://github.com/spf13/viper#why-viper

The environment variables should be used as defaults for the flags

I don't really see the reason or value in this. These are not really defaults. It's just echoing the value set.

adriansmares commented 6 months ago

I don't really see the reason or value in this. These are not really defaults. It's just echoing the value set.

The reason is that we don't need for each individual flag manual code that says 'if flag is empty, check environment variable'. Since we don't use viper here, for every single flag we would need to code this by hand, which is unnecessary if we use the defaults.

KrishnaIyer commented 6 months ago

Ok cool. I don't have an argument against it.

TheThingsNetwork / lorawan-stack-migrate