-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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.
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
fix(cmd/influxd): suggest running with -h on error instead of printing usage #19995
Conversation
@@ -28,23 +29,25 @@ func main() { | |||
|
|||
influxdb.SetBuildInfo(version, commit, date) | |||
|
|||
rootCmd := launcher.NewInfluxdCommand(context.Background(), | |||
// FIXME | |||
//generate.Command, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue: #19429
rootCmd := launcher.NewInfluxdCommand(context.Background(), | ||
// FIXME | ||
//generate.Command, | ||
//restore.Command, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(As far as I can tell) this was replaced by the backup/restore commands in the influx
CLI, so it's fine to remove the FIXME without a tracking issue
bd795d4
to
02d677b
Compare
@@ -16,6 +16,7 @@ func NewExportIndexCommand() *cobra.Command { | |||
Long: ` | |||
This command will export all series in a TSI index to | |||
SQL format for easier inspection and debugging.`, | |||
Args: cobra.NoArgs, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the magic needed to trigger errors on unknown / typoed subcommands
@@ -178,7 +178,6 @@ var vaultConfig vault.Config | |||
|
|||
func setLauncherCMDOpts(l *Launcher, cmd *cobra.Command) { | |||
cli.BindOptions(cmd, launcherOpts(l)) | |||
cmd.AddCommand(inspect.NewCommand()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before this, the code was registering both influxd inspect
and influxd run inspect
commands. That felt like a mistake, but this technically a breaking change. I felt decently confident making the change because the public docs site only list influxd inspect
, but I can revert if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good with me; run
shouldn't have any subcommands.
@@ -101,7 +100,7 @@ const ( | |||
MaxInt = 1<<uint(strconv.IntSize-1) - 1 | |||
) | |||
|
|||
func NewInfluxdCommand(ctx context.Context, subCommands ...*cobra.Command) *cobra.Command { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The humble beginnings of my quest to simplify launcher.go
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And a noble quest it is!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are nice UX improvements 👏
@@ -178,7 +178,6 @@ var vaultConfig vault.Config | |||
|
|||
func setLauncherCMDOpts(l *Launcher, cmd *cobra.Command) { | |||
cli.BindOptions(cmd, launcherOpts(l)) | |||
cmd.AddCommand(inspect.NewCommand()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good with me; run
shouldn't have any subcommands.
@@ -101,7 +100,7 @@ const ( | |||
MaxInt = 1<<uint(strconv.IntSize-1) - 1 | |||
) | |||
|
|||
func NewInfluxdCommand(ctx context.Context, subCommands ...*cobra.Command) *cobra.Command { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And a noble quest it is!
Closes #19922
I'm not incredibly happy with this whack-a-mole approach, but it seemed like the most minimal approach. I'm going to open a separate issue for refactoring the CLI code here.