Skip to content

Fix update of flags from the environment #117

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
KrishnaIyer merged 5 commits into from
Mar 13, 2024
Merged

Fix update of flags from the environment #117

KrishnaIyer merged 5 commits into from
Mar 13, 2024

Conversation

@KrishnaIyer
Copy link
Member

@KrishnaIyer KrishnaIyer commented Mar 13, 2024 •
edited
Loading

Summary

Refs #112 (comment)

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

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

@KrishnaIyer KrishnaIyer added this to the Mar 2024 milestone Mar 13, 2024
@KrishnaIyer KrishnaIyer self-assigned this Mar 13, 2024
@KrishnaIyer KrishnaIyer marked this pull request as ready for review March 13, 2024 11:52
adriansmares
adriansmares reviewed Mar 13, 2024
View reviewed changes
Copy link
Contributor

@adriansmares adriansmares left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think what the original commented requested was clear.

  1. The environment variables should be used as defaults for the flags - that is fine. It should stay that way everywhere.
  2. The only cases in which they shouldn't be used as defaults is for secrets.
  3. The flag value should have a higher priority than the environment variable value. This means that if you provide a secret via the command line and via the environment, the command line one is considered.

pkg/source/chirpstack/config/config.go Outdated
if apiKey := os.Getenv("CHIRPSTACK_API_KEY"); apiKey != "" {
c.apiKey = apiKey
}
if url := os.Getenv("CHIRPSTACK_API_URL"); url != "" {
Copy link
Contributor

@adriansmares adriansmares Mar 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why aren't these non secret parameters built using the defaults here ?

pkg/source/firefly/config.go Outdated

if c.appID = os.Getenv("APP_ID"); c.appID == "" {
return errNoAppID.New()
if appID := os.Getenv("APP_ID"); appID == "" {
Copy link
Contributor

@adriansmares adriansmares Mar 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question here.

@KrishnaIyer
Copy link
Member Author

KrishnaIyer commented Mar 13, 2024

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
Copy link
Contributor

adriansmares commented Mar 13, 2024

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
Copy link
Member Author

KrishnaIyer commented Mar 13, 2024

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

adriansmares
adriansmares reviewed Mar 13, 2024
View reviewed changes
@KrishnaIyer KrishnaIyer merged commit 37b8acd into master Mar 13, 2024
@KrishnaIyer KrishnaIyer deleted the fix/env branch March 13, 2024 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@happyRip happyRip Awaiting requested review from happyRip happyRip is a code owner

@johanstokking johanstokking Awaiting requested review from johanstokking johanstokking is a code owner

1 more reviewer

@adriansmares adriansmares adriansmares approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@KrishnaIyer KrishnaIyer

Labels

None yet

Projects

None yet

Milestone

Mar 2024

Development

Successfully merging this pull request may close these issues.

2 participants

@KrishnaIyer @adriansmares