CLI improvements proposal: positional argument #65

happyRip · 2023-01-05T13:42:54Z

Summary

Closes #56

Changes

Add an optional positional source argument (pkg/arg)
Print only source-related flags with those commands
- Removed prefixes from ttnv3 flags; to be followed with other sources
Split application and devices commands FlagSets
Improved ttnv3 config by utilising flags that save value to variables directly

Testing

local testing

The Things Stack
Chirpstack v3
ttnv2

Regressions

I do not expect any.

Notes for Reviewers

If we were planning to add more commands in the future, FlagSet splitting could be upgraded to a more sophisticated solution. Currently it's pretty simple.
It was suggested to use generated commands like tts-devices ETC, but I wanted to propose this solution which seems to work well and is pretty simple
When pflag is used to set ActiveSource the value is set after initialisation, so the flags cannot be separated easily (during FlagSet() function call)

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.

cmd/ttn-lw-migrate/main.go

pkg/source/ttnv3/config.go

happyRip · 2023-02-19T16:27:33Z

pkg/source/ttnv3/config/config.go

 	)

-	flags.StringVar(&config.AppID,
+	devices.StringVar(&config.AppID,


The --app-id flag was removed from application command of ttnv{2,3} sources since there was no use case for it.
Example:

ttn-lw-migrate ttnv3 application --app-id 'my-app'

If the tool was called like this without an argument it would expect to receive Application IDs from stdin so the flag would be ignored and the tool would 'hang unexpectedly' (waiting).

Since f33a68b the argument always overrides the flag. This combination makes the flag redundant or even confusing for the application command in those sources.

johanstokking

Is this flag magic really necessary? Why don't we have commands per source with their own flag sets?

Also I see some exotic use of switch/case while this is what simple if/else statements seem to be made for.

adriansmares · 2023-02-28T16:10:51Z

Is this flag magic really necessary? Why don't we have commands per source with their own flag sets?

@happyRip I think this actually may be better than our current hack. We would need to generate dynamically an application / device sub command per source, but I think it would indeed work better here.

adriansmares · 2023-02-28T15:59:26Z

CHANGELOG.md

@@ -8,24 +8,31 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),

 ### Added

- Added `--ttnv3.no-session` and `--ttnv3.delete-source-device` flags
+- positional `source` argument


Upper case first letter and dot at the end.

adriansmares · 2023-02-28T15:59:54Z

CHANGELOG.md


 ### Changed

-### Fixed
+- depreciated `--source` flag


Upper case first letter, deprecated and dot at the end.

adriansmares · 2023-02-28T16:00:04Z

CHANGELOG.md


 ### Removed

+- `--app-id` flag for `application` command


Dot at the end. Same below.

adriansmares · 2023-02-28T16:05:33Z

pkg/source/ttnv3/config/config.go

+		return err
+	}
+
+	// deleteSourceDevice not allowed when dryRun


DeleteSourceDevice is not allowed during a dry run.

pkg/source/ttnv3/util.go

adriansmares · 2023-04-07T17:06:42Z

What is the status here - is this blocked on anything ? Does it need any technical decision ?

happyRip · 2023-05-01T13:34:18Z

Subcommands approach was merged, so this PR can be closed.

happyRip self-assigned this Jan 5, 2023

happyRip force-pushed the feature/56-cli-redesign branch 3 times, most recently from cc5d0f6 to 618f047 Compare January 9, 2023 13:12

happyRip marked this pull request as ready for review January 9, 2023 14:09

happyRip requested review from johanstokking and adriansmares as code owners January 9, 2023 14:09

happyRip added this to the v0.10.0 milestone Jan 9, 2023

adriansmares reviewed Jan 9, 2023

View reviewed changes

cmd/ttn-lw-migrate/main.go Outdated Show resolved Hide resolved

adriansmares reviewed Jan 10, 2023

View reviewed changes

pkg/source/ttnv3/config.go Outdated Show resolved Hide resolved

happyRip marked this pull request as draft January 10, 2023 11:11

happyRip force-pushed the feature/56-cli-redesign branch 2 times, most recently from b1813a4 to 0f8a43e Compare January 17, 2023 16:10

happyRip force-pushed the feature/56-cli-redesign branch 3 times, most recently from 5a8c694 to b93c2e5 Compare February 19, 2023 16:21

happyRip commented Feb 19, 2023

View reviewed changes

happyRip marked this pull request as ready for review February 20, 2023 16:00

happyRip requested a review from adriansmares February 20, 2023 16:00

johanstokking reviewed Feb 23, 2023

View reviewed changes

adriansmares reviewed Feb 28, 2023

View reviewed changes

happyRip mentioned this pull request Mar 2, 2023

Fix ttnv3 source related bugs #68

Merged

3 tasks

happyRip force-pushed the feature/56-cli-redesign branch from 6750f9c to bc29d66 Compare March 8, 2023 12:37

happyRip marked this pull request as draft March 8, 2023 12:39

happyRip force-pushed the feature/56-cli-redesign branch from bc29d66 to af21b65 Compare March 8, 2023 12:39

happyRip added 4 commits March 8, 2023 13:40

dev: Add positional source argument to root command

808d0b1

dev: Remove prefix from ttnv3

457ae74

dev: Add command-specific flag sets

71e9e48

dev: Append source name to flag names when no ActiveSource

3e1cae4

happyRip and others added 11 commits March 8, 2023 13:40

dev: Move arg to cmd package

36ff1f9

dev: Avoid depending on initialization order

7e3fb40

dev: Use root config and remove unused global flags

ea8d66b

dev: Update go.mod

20ef7e9

dev: Update chirpstack source

9ad639e

dev: Update ttnv2 source

0a68f80

dev: Update ttnv3 source

3543f6a

dev: Improve source flags

3c91e78

dev: Fix ttnv3 TLS

e6bc443

dev: Update copyright notices

b902185

dev: Fix chirpstack source

66d0239

happyRip force-pushed the feature/56-cli-redesign branch from af21b65 to 66d0239 Compare March 8, 2023 12:40

This was referenced Apr 13, 2023

Improve source configuration #70

Merged

CLI improvements proposal: subcommands #71

Merged

happyRip changed the title ~~Migration tool CLI improvements~~ CLI improvements proposal: positional argument May 1, 2023

happyRip closed this May 1, 2023

johanstokking deleted the feature/56-cli-redesign branch May 2, 2023 12:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLI improvements proposal: positional argument #65

CLI improvements proposal: positional argument #65

happyRip commented Jan 5, 2023 •

edited

Loading

happyRip Feb 19, 2023

johanstokking left a comment

adriansmares commented Feb 28, 2023

adriansmares Feb 28, 2023

adriansmares Feb 28, 2023

adriansmares Feb 28, 2023

adriansmares Feb 28, 2023

adriansmares commented Apr 7, 2023

happyRip commented May 1, 2023

CLI improvements proposal: positional argument #65

CLI improvements proposal: positional argument #65

Conversation

happyRip commented Jan 5, 2023 • edited Loading

Summary

Changes

Testing

Regressions

Notes for Reviewers

Checklist

happyRip Feb 19, 2023

Choose a reason for hiding this comment

johanstokking left a comment

Choose a reason for hiding this comment

adriansmares commented Feb 28, 2023

adriansmares Feb 28, 2023

Choose a reason for hiding this comment

adriansmares Feb 28, 2023

Choose a reason for hiding this comment

adriansmares Feb 28, 2023

Choose a reason for hiding this comment

adriansmares Feb 28, 2023

Choose a reason for hiding this comment

adriansmares commented Apr 7, 2023

happyRip commented May 1, 2023

happyRip commented Jan 5, 2023 •

edited

Loading