Subcommands #4420

exekias · 2017-05-30T14:33:59Z

This change introduces subcommand interface to all beats.

This give us several new features:

Isolated subcommands (version, setup, run...)
Command line help for all of them
Autocompletion and typo hinting
Freedom to add beat specific subcommands where needed

Usage examples:

  $ configtest  Test configuration settings
  $ help        Help about any command
  $ run         Run metricbeat
  $ setup       Setup index template and dashboards
  $ version     Show current version info

I've kept root command as an alias for run, so old style calls should be still working.

About flags:

Cobra supports persistent (global) and local flags for each command.
I've added config related flags as persistent, common to all
subcommands. While keeping runtime related flags under run, see the
full list here:

Flags:
  -N, --N                   Disable actual publishing for testing
      --configtest          Test configuration and exit.
      --cpuprofile string   Write cpu profile to file
  -e, --e                   Log to stderr and disable syslog/file output
  -h, --help                help for run
      --httpprof string     Start pprof http server
      --memprofile string   Write memory profile to this file
      --setup               Load the sample Kibana dashboards
  -v, --v                   Log at INFO level
      --version             Print the version and exit

Global Flags:
  -E, --E Flag                      Configuration overwrite (default null)
  -c, --c argList                   Configuration file, relative to path.config (default beat.yml)
  -d, --d string                    Enable certain debug selectors
      --path.config flagOverwrite   Configuration path
      --path.data flagOverwrite     Data path
      --path.home flagOverwrite     Home path
      --path.logs flagOverwrite     Logs path
      --strict.perms                Strict permission checking on config files (default true)

In the long term we should aim to deprecate a few of them (setup, version, configtest...) in
favor of subcommands. I'm keeping them for now to maintain backwards compatibilty with 5.X.

In progress:

Implement missing subcommand: setup, configtest
Revisit beat generator?

tsg · 2017-05-31T09:03:31Z

filebeat/cmd/run.go

+)
+
+// RunCmd runs the beat
+var RunCmd = &cobra.Command{


Any reason this is not also part of libbeat? Is there anything beat specific about RunCmd?

beater is the specific part, but now you ask, I could add that as a parameter to GenRootCmd

ruflin

Awesome stuff. Really looking forward to have this in the beats.

ruflin · 2017-06-01T00:29:24Z

filebeat/main.go

@@ -3,12 +3,9 @@ package main
 import (
 	"os"

-	"github.com/elastic/beats/filebeat/beater"
-	"github.com/elastic/beats/libbeat/beat"
+	"github.com/elastic/beats/filebeat/cmd"


This change brings up the question if we should also change the generator to support this?

I promise this was already there:

😄

ruflin · 2017-06-01T00:30:38Z

libbeat/beat/beat.go

 		if err != nil {
 			return err
 		}
 		return b.launch(bt)
 	}())
 }

-// newBeat creates a new beat instance
-func newBeat(name, v string) (*Beat, error) {
+// NewBeat creates a new beat instance


I wonder why lint did not complain :-)

Do we have to make this global?

ruflin · 2017-06-01T00:31:17Z

libbeat/cmd/root.go

+	return rootCmd
+}
+
+func init() {


I would recommend to put the init on top, at least that is what we did so far.

ruflin · 2017-06-01T00:31:58Z

libbeat/cmd/version.go

+		Use:   "version",
+		Short: "Show current version info",
+		Run: func(cmd *cobra.Command, args []string) {
+			beat, err := beat.New(name, "")


Ok, I see why we need it global :-)

tsg · 2017-06-08T08:32:24Z

I wonder how useful is the configtest command. I see we also have the --configtest flag. The command has the drawback that it can't take the -modules flag, for example.

Here is an example session to show the issue:

$ ./filebeat configtest
Exiting: No modules or prospectors enabled and configuration reloading disabled. What files do you want me to watch?

$ ./filebeat configtest --modules=nginx                                       
Error: unknown flag: --modules

$ ./filebeat --modules=nginx --configtest                                    
Config OK

tsg · 2017-06-08T08:33:08Z

Interesting that both -modules and --modules work, btw. I kind of like supporting both options. Did you add some compatibility code for that, or is it simply supported by Corba?

exekias · 2017-06-08T08:47:23Z

@tsg:

Yes, with this change we would have duplicated functionality between subcommands and flags, but purely for backwards compatibility, I'm more in favor of subcommands for all flags that modify run behavior to do a "one shoot" thing. I can include modules flag in configtest if it's missing

I did some black magic for backwards compatibility, Cobra flags use UNIX style, so it would require -- for long names: https://github.com/elastic/beats/pull/4420/files#diff-4111a17b440e3fa32c5c0b8255bd512bR15. In the long run I would aim to use Cobra style in all our documentation and scripts (init scripts, dev tooling...), but I'm ok with keeping that conversion code

tsg · 2017-06-08T08:58:38Z

I did some black magic for backwards compatibility, Cobra flags use UNIX style, so it would require -- for long names: https://github.com/elastic/beats/pull/4420/files#diff-4111a17b440e3fa32c5c0b8255bd512bR15. In the long run I would aim to use Cobra style in all our documentation and scripts (init scripts, dev tooling...), but I'm ok with keeping that conversion code

Ok, I think that's worth a discussion, we could also cut the bandaid with the 6.0 release.

Yes, with this change we would have duplicated functionality between subcommands and flags, but purely for backwards compatibility, I'm more in favor of subcommands for all flags that modify run behavior to do a "one shoot" thing. I can include modules flag in configtest if it's missing

Generally I agree that things that make the Beat stop are better as commands. But the configtest is somehow special as you might want to check that config + CLI flags result in a working config. So either configtest accepts all flags that run does, or we keep the config test as a flag in run.

tsg · 2017-06-08T09:40:41Z

I played with it and it feels great to me, nice work. Dumping some thoughts / wishlist, but for future PRs:

The setup command can export more flags, making it similar in experience with the import_dashboards program that we have now. Then we can drop the import_dashboards from the packages, reducing the byte size significantly.
The setup.template.output_to_file option, to generate the template into a file, could become it's own command.
A modules command, that just lists the existing modules, would already be useful for filebeat & metricbeat. Enabling/disabling them is probably more work, we should discuss if we can do it for 6.0.

tsg · 2017-06-08T09:45:06Z

CHANGELOG.asciidoc

@@ -44,6 +44,8 @@ https://github.com/elastic/beats/compare/v6.0.0-alpha1...master[Check the HEAD d

 *Affecting all Beats*

+- New cli subcommands interface. {pull}4420[4420]


We'll likely need to deprecate some things, but that can go in a meta ticket.

exekias · 2017-06-08T10:14:35Z

Thanks for the input, great new ideas!, I didn't think about import_dashboards and output_to_file, definitely something cool to have. I'm ok with doing it in follow up PRs

exekias · 2017-06-08T12:15:14Z

I'll review flags for configtest and push a change so we can move forward

dedemorton · 2017-06-08T21:34:16Z

@exekias There will be some doc updates required for this, too (the topic about CLI options and the places where we show commands). Feel free to create a separate issue to track this work, and assign it to me, if you'd like. TBH, when I look at your description above, the syntax is not entirely clear to me. I'm sure it will make more sense when I can play around with the commands in a build (or have time to look more closely at the code).

Also, I’m wondering if we’ve taken consistency with other Elastic products (and their command line syntax) into consideration here. Do customers get frustrated when we aren't consistent across products, or am I overthinking this?

exekias · 2017-06-09T09:44:38Z

great @dedemorton I will open a new ticket, also input is more than welcomed if you find better naming for any of the added commands.

I didn't though about consistency, but it's a good point, I'm going to check how the rest of projects handle this. While I think it's not that important (different projects ==> different ways of using them) perhaps we can achieve something more consistent, which is always good

tsg · 2017-06-09T21:51:49Z

@exekias Can you rebase this one, I think it will fix some of the test failures.

This change introduces subcommand interface to all beats. This give us several new features: * Isolated subcommands (version, setup, run...) * Command line help for all of them * Autocompletion and typo hinting * Freedom to add beat specific subcommands where needed Usage examples: ``` $ metricbeat help $ metricbeat version $ metricbeat run -h $ metricbeat run -e -v ``` I've kept root command as an alias for `run`, so old style calls should be still working. About flags: Cobra supports persistent (global) and local flags for each command. I've added config related flags as persistent, common to all subcommands. While keeping runtime related flags under `run`, see the full list here: ``` Flags: -N, --N Disable actual publishing for testing --configtest Test configuration and exit. --cpuprofile string Write cpu profile to file -e, --e Log to stderr and disable syslog/file output -h, --help help for run --httpprof string Start pprof http server --memprofile string Write memory profile to this file --setup Load the sample Kibana dashboards -v, --v Log at INFO level --version Print the version and exit Global Flags: -E, --E Flag Configuration overwrite (default null) -c, --c argList Configuration file, relative to path.config (default beat.yml) -d, --d string Enable certain debug selectors --path.config flagOverwrite Configuration path --path.data flagOverwrite Data path --path.home flagOverwrite Home path --path.logs flagOverwrite Logs path --strict.perms Strict permission checking on config files (default true) ``` In the log term we should aim to deprecate a few of them (setup, version, configtest...) in favor of subcommands. I'm keeping them for now to maintain backwards compatibilty with 5.X.

exekias added discuss Issue needs further discussion. enhancement in progress Pull request is currently in progress. libbeat v6.0.0-beta1 labels May 30, 2017

exekias force-pushed the subcommands branch from bd45da3 to 40308a4 Compare May 30, 2017 15:33

tsg reviewed May 31, 2017

View reviewed changes

exekias force-pushed the subcommands branch 4 times, most recently from 60b565c to 7eaf56e Compare June 1, 2017 00:03

ruflin reviewed Jun 1, 2017

View reviewed changes

exekias force-pushed the subcommands branch 11 times, most recently from 5209732 to f68b962 Compare June 5, 2017 13:43

exekias added review and removed discuss Issue needs further discussion. in progress Pull request is currently in progress. labels Jun 5, 2017

exekias force-pushed the subcommands branch 4 times, most recently from 0fbe954 to fe28b23 Compare June 6, 2017 12:49

tsg reviewed Jun 8, 2017

View reviewed changes

exekias force-pushed the subcommands branch 2 times, most recently from c863e3f to f985346 Compare June 12, 2017 09:43

exekias added 12 commits June 12, 2017 12:36

Add spf13/cobra vendor dependency

18b0997

Autogenerate run command

aa548c3

Add backwards compatibility for -flags

79ef640

Add specific beats flags

b79e79e

Add setup command

5889431

Add subcommands tests

99b8944

Add testconfig subcommand

f9b2e3e

Update beat generator to follow new subcommands structure

1c91626

Update CHANGELOG

82caa1e

Expose subcommands from root struct

0a4b28c

Honor modules flag in configtest command

d67e93f

exekias force-pushed the subcommands branch from f985346 to d67e93f Compare June 12, 2017 10:36

tsg merged commit 4e34814 into elastic:master Jun 12, 2017

exekias mentioned this pull request Jun 12, 2017

Document new command line interface #4490

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Subcommands #4420

Subcommands #4420

exekias commented May 30, 2017 •

edited by monicasarbu

Loading

tsg May 31, 2017

exekias May 31, 2017

ruflin left a comment

ruflin Jun 1, 2017

exekias Jun 1, 2017

ruflin Jun 1, 2017

ruflin Jun 1, 2017

ruflin Jun 1, 2017

tsg commented Jun 8, 2017

tsg commented Jun 8, 2017

exekias commented Jun 8, 2017

tsg commented Jun 8, 2017

tsg commented Jun 8, 2017

tsg Jun 8, 2017

exekias commented Jun 8, 2017

exekias commented Jun 8, 2017

dedemorton commented Jun 8, 2017 •

edited

Loading

exekias commented Jun 9, 2017

tsg commented Jun 9, 2017

		@@ -44,6 +44,8 @@ https://github.com/elastic/beats/compare/v6.0.0-alpha1...master[Check the HEAD d

		Affecting all Beats

		- New cli subcommands interface. {pull}4420[4420]

Subcommands #4420

Subcommands #4420

Conversation

exekias commented May 30, 2017 • edited by monicasarbu Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruflin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsg commented Jun 8, 2017

tsg commented Jun 8, 2017

exekias commented Jun 8, 2017

tsg commented Jun 8, 2017

tsg commented Jun 8, 2017

Choose a reason for hiding this comment

exekias commented Jun 8, 2017

exekias commented Jun 8, 2017

dedemorton commented Jun 8, 2017 • edited Loading

exekias commented Jun 9, 2017

tsg commented Jun 9, 2017

exekias commented May 30, 2017 •

edited by monicasarbu

Loading

dedemorton commented Jun 8, 2017 •

edited

Loading