Skip to content
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

Subcommands enhancements #4510

Merged
merged 10 commits into from
Jun 26, 2017
Merged

Subcommands enhancements #4510

merged 10 commits into from
Jun 26, 2017

Conversation

exekias
Copy link
Contributor

@exekias exekias commented Jun 15, 2017

  • Mark configtest, setup and version flags as deprecated, this hides them from the help 76a3164
  • Add a new hidden completion subcommand. It dumps bash completion script to stdout, this can be handy to load it on the fly, while it would make sense to dump it into packages, will try to figure out how to do it with packer. Check https://blog.fabric8.io/enable-bash-completion-for-kubernetes-with-kubectl-506bc89fe79e for a good example of how this would be used
  • Add a new export subcommand, at the moment it allows to export:
    • config: Current config, good to ask for debugging info
    • template: Index template, this could replace index_template script

For examples check:

$ filebeat completion
$ filebeat export config
$ filebeat export template

@exekias exekias force-pushed the subcommands-enhancements branch 4 times, most recently from 8eb0d54 to 905e5c4 Compare June 15, 2017 21:48
@exekias exekias added the in progress Pull request is currently in progress. label Jun 16, 2017
@exekias exekias force-pushed the subcommands-enhancements branch 3 times, most recently from dc04512 to 7402fe8 Compare June 16, 2017 13:32
@@ -279,13 +279,17 @@ func (b *Beat) launch(bt Creator) error {

// If -configtest was specified, exit now prior to run.
if cfgfile.IsTestConfig() {
logp.Deprecate("6.0", "-configtest flag has been deprectad, use configtest subcommand")
Copy link
Member

@andrewkroh andrewkroh Jun 16, 2017

Choose a reason for hiding this comment

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

s/deprectad/deprecated/g (occurs multiple times)

@exekias exekias added review and removed review in progress Pull request is currently in progress. labels Jun 16, 2017
@exekias exekias force-pushed the subcommands-enhancements branch 2 times, most recently from 2f3cd83 to a1df699 Compare June 19, 2017 14:18
@exekias
Copy link
Contributor Author

exekias commented Jun 19, 2017

Fixed the typo and rebased to master, tests should pass now

@exekias
Copy link
Contributor Author

exekias commented Jun 21, 2017

jenkins retest this

@tsg
Copy link
Contributor

tsg commented Jun 21, 2017

I get a crash when trying the Packetbeat export template:

$ ./packetbeat export template                                                                      1 ↵
Error getting template settings: (assert) config is nil
Trace:goroutine 1 [running]:
runtime/debug.Stack(0x4cab66c, 0xb, 0xc42127fab0)
	/usr/local/Cellar/go/1.8.3/libexec/src/runtime/debug/stack.go:24 +0x79
github.com/elastic/beats/vendor/github.com/elastic/go-ucfg.raiseCritical(0x528f600, 0xc420019250, 0xc421195a60, 0x16, 0x0, 0xc42118bf20)
	/Users/tsg/src/github.com/elastic/beats/vendor/github.com/elastic/go-ucfg/error.go:132 +0x66
github.com/elastic/beats/vendor/github.com/elastic/go-ucfg.raiseNil(0x528f600, 0xc420019250, 0x3, 0xc420452380)
	/Users/tsg/src/github.com/elastic/beats/vendor/github.com/elastic/go-ucfg/error.go:235 +0x47
github.com/elastic/beats/vendor/github.com/elastic/go-ucfg.(*Config).Unpack(0x0, 0x4abfac0, 0xc42006f140, 0x52edf20, 0x3, 0x3, 0x1, 0xc42006f140)
	/Users/tsg/src/github.com/elastic/beats/vendor/github.com/elastic/go-ucfg/reify.go:108 +0x1fb
github.com/elastic/beats/libbeat/common.(*Config).Unpack(0x0, 0x4abfac0, 0xc42006f140, 0xc, 0xc420451a00)
	/Users/tsg/src/github.com/elastic/beats/libbeat/common/config.go:193 +0x63
github.com/elastic/beats/libbeat/cmd/export.GenTemplateConfigCmd.func1(0xc421408480, 0x58377a8, 0x0, 0x0)
	/Users/tsg/src/github.com/elastic/beats/libbeat/cmd/export/template.go:38 +0x338
github.com/elastic/beats/vendor/github.com/spf13/cobra.(*Command).execute(0xc421408480, 0x58377a8, 0x0, 0x0, 0xc421408480, 0x58377a8)
	/Users/tsg/src/github.com/elastic/beats/vendor/github.com/spf13/cobra/command.go:651 +0x23a
github.com/elastic/beats/vendor/github.com/spf13/cobra.(*Command).ExecuteC(0xc4213d6500, 0xc4213d6500, 0x0, 0x0)
	/Users/tsg/src/github.com/elastic/beats/vendor/github.com/spf13/cobra/command.go:726 +0x339
github.com/elastic/beats/vendor/github.com/spf13/cobra.(*Command).Execute(0xc4213d6500, 0xc4200001a0, 0xc4200001a0)
	/Users/tsg/src/github.com/elastic/beats/vendor/github.com/spf13/cobra/command.go:685 +0x2b
main.main()
	/Users/tsg/src/github.com/elastic/beats/packetbeat/main.go:13 +0x2f

@@ -15,6 +16,7 @@ func genConfigTestCmd(name, version string, beatCreator beat.Creator) *cobra.Com
Run: func(cmd *cobra.Command, args []string) {
b, err := beat.New(name, version)
if err != nil {
fmt.Fprintf(os.Stderr, "Error initializing beat: %s\n", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like it too much that we get these writes directly to stdout/stderr. Perhaps it would be possible to init logging with stderr on any non-run command?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's your exact concern? I tried to do it just the other way around, strip all log metadata as this is command line

It would be possible though

Copy link
Contributor

Choose a reason for hiding this comment

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

I just don't like it that logp and fmt.Println are interleaved in the code. But def. not a show stopper.


// Make it compatible with the sem versioning
if version == "2x" {
version = "2.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

If we have magic for 2x, we should also add 5x and 6x, I guess. But, tbh, I don't think it's worth the trouble.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say we remove it, or @ruflin, do you see it otherwise?

@tsg
Copy link
Contributor

tsg commented Jun 21, 2017

I think the export template command covers the functionality of the setup.template.output_to_file functionality. The latter was never released, so I think we should delete it before it makes it out in a release.

@tsg
Copy link
Contributor

tsg commented Jun 21, 2017

I have buyer remorse for removing --setup :). The reason is that:

./filebeat -e -setup --modules=nginx,system

Is just nicer and more intuitive than:

./filebeat -e setup --modules=nginx,system && ./filebeat -e --modules=nginx,system

This can get much worse when one has to overwrite multiple config file settings (e.g. ES output).

If it's not too much effort, I'd say we don't deprecate/hide the --setup flag for now.

@tsg
Copy link
Contributor

tsg commented Jun 21, 2017

Nice work @exekias, and exporting the config as well as the bash completion are really nice touches.

@exekias exekias force-pushed the subcommands-enhancements branch 3 times, most recently from 2e38d1c to bc2c7f2 Compare June 26, 2017 10:45
@exekias
Copy link
Contributor Author

exekias commented Jun 26, 2017

I have added some commits:

  • Fix export template issue in packetbeat (and others)
  • Show -setup flag back
  • Remove 2x mangling in export template
  • Remove output_to_file code in favor of export template

I think this should be ready to merge

Copy link
Contributor

@tsg tsg left a comment

Choose a reason for hiding this comment

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

WFG

@tsg tsg merged commit dc6e846 into elastic:master Jun 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants