Improve custom config flag handlers #5543

urso · 2017-11-08T15:49:24Z

move support for flags collecting usage into a []string to libbeat/common
fix printing default value of -c flag
move flags manipulating config objects from config.go to flags.go
change flag namings to always end with xxxFlag
introduce unit tests for config flags
check the flags usage string in unit tests
fix potential panic when printing defaults on flag collecting into []string (nil pointer being passed)
fix printing defaults and setting types
add Type method improving cobra flag parameter in help message

Old flags usage messages for filebeat -h:

  -E, --E Flag                      Configuration overwrite (default null)
  -M, --M Flag                      Module configuration overwrite (default null)
  -N, --N                           Disable actual publishing for testing
  -c, --c argList                   Configuration file, relative to path.config (default beat.yml)
      --cpuprofile string           Write cpu profile to file
  -d, --d string                    Enable certain debug selectors
  -e, --e                           Log to stderr and disable syslog/file output
  -h, --help                        help for filebeat
      --httpprof string             Start pprof http server
      --memprofile string           Write memory profile to this file
      --modules string              List of enabled modules (comma separated)
      --once                        Run filebeat only once until all harvesters reach EOF
      --path.config flagOverwrite   Configuration path
      --path.data flagOverwrite     Data path
      --path.home flagOverwrite     Home path
      --path.logs flagOverwrite     Logs path
      --setup                       Load the sample Kibana dashboards
      --strict.perms                Strict permission checking on config files (default true)
  -v, --v                           Log at INFO level

New flags usage messages for filebeat -h:

  -E, --E setting=value      Configuration overwrite
  -M, --M setting=value      Module configuration overwrite
  -N, --N                    Disable actual publishing for testing
  -c, --c string             Configuration file, relative to path.config (default "filebeat.yml")
      --cpuprofile string    Write cpu profile to file
  -d, --d string             Enable certain debug selectors
  -e, --e                    Log to stderr and disable syslog/file output
  -h, --help                 help for filebeat
      --httpprof string      Start pprof http server
      --memprofile string    Write memory profile to this file
      --modules string       List of enabled modules (comma separated)
      --once                 Run filebeat only once until all harvesters reach EOF
      --path.config string   Configuration path
      --path.data string     Data path
      --path.home string     Home path
      --path.logs string     Logs path
      --setup                Load the sample Kibana dashboards
      --strict.perms         Strict permission checking on config files (default true)
  -v, --v                    Log at INFO level

houndci-bot · 2017-11-08T15:49:36Z

libbeat/common/flags.go

+	return f.List()
+}
+
+func (f *StringsFlag) List() []string {


exported method StringsFlag.List should have comment or be unexported

houndci-bot · 2017-11-08T15:49:36Z

libbeat/common/flags.go

+	return nil
+}
+
+func (f *StringsFlag) Get() interface{} {


exported method StringsFlag.Get should have comment or be unexported

houndci-bot · 2017-11-08T15:49:36Z

libbeat/common/flags.go

+	}
+}
+
+func (f *StringsFlag) Set(v string) error {


exported method StringsFlag.Set should have comment or be unexported

houndci-bot · 2017-11-08T15:49:37Z

libbeat/common/flags.go

+	return strings.Join(*f.list, ", ")
+}
+
+func (f StringsFlag) SetDefault(v string) {


exported method StringsFlag.SetDefault should have comment or be unexported

houndci-bot · 2017-11-08T15:49:37Z

libbeat/common/flags.go

+	return StringArrVarFlag(arr, name, usage)
+}
+
+func StringArrVarFlag(arr *[]string, name, usage string) *StringsFlag {


exported function StringArrVarFlag should have comment or be unexported

houndci-bot · 2017-11-08T15:49:37Z

libbeat/common/flags.go

+	flag      *flag.Flag
+}
+
+func StringArrFlag(name, def, usage string) *StringsFlag {


exported function StringArrFlag should have comment or be unexported

houndci-bot · 2017-11-08T15:49:37Z

libbeat/common/flags.go

+	"strings"
+)
+
+type StringsFlag struct {


exported type StringsFlag should have comment or be unexported

ph

LGTM, Maybe fix the hound complains?

The failures seems to be unrelated.

--- FAIL: TestFetch (81.44s)
	Error Trace:	status_integration_test.go:20
	Error:		Received unexpected error "error making http request: Get http://kibana:5601/api/status: lookup kibana

ph · 2017-11-08T20:25:40Z

libbeat/common/flags_test.go

+			assert.Equal(t, test.expected, flag.List())
+		})
+	}
+}


wohoo tests :)

houndci-bot · 2017-11-09T14:18:45Z

libbeat/common/flags.go

+	return *f.list
+}
+
+func (f *StringsFlag) Type() string {


exported method StringsFlag.Type should have comment or be unexported

houndci-bot · 2017-11-09T14:18:45Z

libbeat/common/flags.go

+	return strings.Join(l, ", ")
+}
+
+func (f *StringsFlag) SetDefault(v string) {


exported method StringsFlag.SetDefault should have comment or be unexported

houndci-bot · 2017-11-09T14:18:45Z

libbeat/common/flags.go

+	return &StringsFlag{list: arr, isDefault: true}
+}
+
+func (f *StringsFlag) Register(fs *flag.FlagSet, name, usage string) {


exported method StringsFlag.Register should have comment or be unexported

houndci-bot · 2017-11-09T14:18:45Z

libbeat/common/flags.go

+	return f
+}
+
+func NewStringsFlag(arr *[]string) *StringsFlag {


exported function NewStringsFlag should have comment or be unexported

houndci-bot · 2017-11-09T14:18:45Z

libbeat/common/flags.go

+	return *f.list
+}
+
+func (f *StringsFlag) Type() string {


exported method StringsFlag.Type should have comment or be unexported

houndci-bot · 2017-11-09T14:18:45Z

libbeat/common/flags.go

+	return strings.Join(l, ", ")
+}
+
+func (f *StringsFlag) SetDefault(v string) {


exported method StringsFlag.SetDefault should have comment or be unexported

houndci-bot · 2017-11-09T14:18:45Z

libbeat/common/flags.go

+	return &StringsFlag{list: arr, isDefault: true}
+}
+
+func (f *StringsFlag) Register(fs *flag.FlagSet, name, usage string) {


exported method StringsFlag.Register should have comment or be unexported

houndci-bot · 2017-11-09T14:18:45Z

libbeat/common/flags.go

+	return f
+}
+
+func NewStringsFlag(arr *[]string) *StringsFlag {


exported function NewStringsFlag should have comment or be unexported

houndci-bot · 2017-11-09T22:09:34Z

libbeat/common/flags.go

+	return "setting=value"
+}
+
+func ConfigOverwriteFlag(


exported function ConfigOverwriteFlag should have comment or be unexported

houndci-bot · 2017-11-09T22:09:34Z

libbeat/common/flags.go

+	return ""
+}
+
+func (f *SettingsFlag) Type() string {


exported method SettingsFlag.Type should have comment or be unexported

houndci-bot · 2017-11-09T22:09:34Z

libbeat/common/flags.go

+	return f.access().Set(s)
+}
+
+func (f *SettingsFlag) Get() interface{} {


exported method SettingsFlag.Get should have comment or be unexported

houndci-bot · 2017-11-09T22:09:34Z

libbeat/common/flags.go

+	return fromConfig(f.access().Config())
+}
+
+func (f *SettingsFlag) Set(s string) error {


exported method SettingsFlag.Set should have comment or be unexported

houndci-bot · 2017-11-09T22:09:34Z

libbeat/common/flags.go

+	return (*cfgflag.FlagValue)(f)
+}
+
+func (f *SettingsFlag) Config() *Config {


exported method SettingsFlag.Config should have comment or be unexported

houndci-bot · 2017-11-09T22:09:34Z

libbeat/common/flags.go

+	fs.Var(f, name, usage)
+}
+
+func NewSettingsFlag(def *Config) *SettingsFlag {


exported function NewSettingsFlag should have comment or be unexported

houndci-bot · 2017-11-09T22:09:34Z

libbeat/common/flags.go

+	return cfg
+}
+
+func SettingVarFlag(fs *flag.FlagSet, def *Config, name, usage string) {


exported function SettingVarFlag should have comment or be unexported

houndci-bot · 2017-11-09T22:09:35Z

libbeat/common/flags.go

+	return "string"
+}
+
+func SettingFlag(fs *flag.FlagSet, name, usage string) *Config {


exported function SettingFlag should have comment or be unexported

houndci-bot · 2017-11-09T22:09:35Z

libbeat/common/flags.go

+	return StringArrVarFlag(fs, arr, name, usage)
+}
+
+func StringArrVarFlag(fs *flag.FlagSet, arr *[]string, name, usage string) *StringsFlag {


exported function StringArrVarFlag should have comment or be unexported

ph

LGTM, waiting on CI to merge that.
+1 on refactoring
+1 on adding test
+100 for user facing improvement :)

ph · 2017-11-10T15:32:52Z

@urso Would you mind squashing it?

- move support for flags collecting usage into a []string to libbeat/common - fix printing default value of `-c` flag - move flags manipulating config objects from config.go to flags.go - change flag namings to always end with xxxFlag - introduce unit tests for config flags - check the flags usage string in unit tests - fix potential panic when printing defaults on flag collecting into []string (nil pointer being passed) - fix printing defaults and setting types - add `Type` method improving cobra flag parameter in help message Old flags usage messages for `filebeat -h`: ``` -E, --E Flag Configuration overwrite (default null) -M, --M Flag Module configuration overwrite (default null) -N, --N Disable actual publishing for testing -c, --c argList Configuration file, relative to path.config (default beat.yml) --cpuprofile string Write cpu profile to file -d, --d string Enable certain debug selectors -e, --e Log to stderr and disable syslog/file output -h, --help help for filebeat --httpprof string Start pprof http server --memprofile string Write memory profile to this file --modules string List of enabled modules (comma separated) --once Run filebeat only once until all harvesters reach EOF --path.config flagOverwrite Configuration path --path.data flagOverwrite Data path --path.home flagOverwrite Home path --path.logs flagOverwrite Logs path --setup Load the sample Kibana dashboards --strict.perms Strict permission checking on config files (default true) -v, --v Log at INFO level ``` New flags usage messages for `filebeat -h`: ``` -E, --E setting=value Configuration overwrite -M, --M setting=value Module configuration overwrite -N, --N Disable actual publishing for testing -c, --c string Configuration file, relative to path.config (default "filebeat.yml") --cpuprofile string Write cpu profile to file -d, --d string Enable certain debug selectors -e, --e Log to stderr and disable syslog/file output -h, --help help for filebeat --httpprof string Start pprof http server --memprofile string Write memory profile to this file --modules string List of enabled modules (comma separated) --once Run filebeat only once until all harvesters reach EOF --path.config string Configuration path --path.data string Data path --path.home string Home path --path.logs string Logs path --setup Load the sample Kibana dashboards --strict.perms Strict permission checking on config files (default true) -v, --v Log at INFO level ```

urso added the review label Nov 8, 2017

houndci-bot reviewed Nov 8, 2017

View reviewed changes

urso added in progress Pull request is currently in progress. and removed review labels Nov 8, 2017

ph approved these changes Nov 8, 2017

View reviewed changes

libbeat/common/flags_test.go

assert.Equal(t, test.expected, flag.List())

})

}

}

Copy link

Contributor

ph Nov 8, 2017

Choose a reason for hiding this comment

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

wohoo tests :)

houndci-bot reviewed Nov 9, 2017

View reviewed changes

urso changed the title ~~Move []string flag support to libbeat/common~~ Improve custom config based flag handlers Nov 9, 2017

urso changed the title ~~Improve custom config based flag handlers~~ Improve custom config flag handlers Nov 9, 2017

urso added libbeat refactoring review and removed in progress Pull request is currently in progress. labels Nov 10, 2017

ph approved these changes Nov 10, 2017

View reviewed changes

urso force-pushed the feature/common-flags branch from ff58fb3 to 138b48f Compare November 10, 2017 15:52

urso force-pushed the feature/common-flags branch from 138b48f to 21da625 Compare November 10, 2017 16:04

ph merged commit 8ae5b4b into elastic:master Nov 10, 2017

urso deleted the feature/common-flags branch February 19, 2019 18:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve custom config flag handlers #5543

Improve custom config flag handlers #5543

urso commented Nov 8, 2017 •

edited

Loading

houndci-bot Nov 8, 2017

houndci-bot Nov 8, 2017

houndci-bot Nov 8, 2017

houndci-bot Nov 8, 2017

houndci-bot Nov 8, 2017

houndci-bot Nov 8, 2017

houndci-bot Nov 8, 2017

ph left a comment

ph Nov 8, 2017

houndci-bot Nov 9, 2017

houndci-bot Nov 9, 2017

houndci-bot Nov 9, 2017

houndci-bot Nov 9, 2017

houndci-bot Nov 9, 2017

houndci-bot Nov 9, 2017

houndci-bot Nov 9, 2017

houndci-bot Nov 9, 2017

houndci-bot Nov 9, 2017

houndci-bot Nov 9, 2017

houndci-bot Nov 9, 2017

houndci-bot Nov 9, 2017

houndci-bot Nov 9, 2017

houndci-bot Nov 9, 2017

houndci-bot Nov 9, 2017

houndci-bot Nov 9, 2017

houndci-bot Nov 9, 2017

ph left a comment

ph commented Nov 10, 2017

Improve custom config flag handlers #5543

Improve custom config flag handlers #5543

Conversation

urso commented Nov 8, 2017 • edited Loading

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

Choose a reason for hiding this comment

ph 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

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

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

Choose a reason for hiding this comment

ph left a comment

Choose a reason for hiding this comment

ph commented Nov 10, 2017

urso commented Nov 8, 2017 •

edited

Loading