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

Improve custom config flag handlers #5543

Merged
merged 1 commit into from Nov 10, 2017

Conversation

Projects
None yet
3 participants
@urso
Collaborator

urso commented Nov 8, 2017

  • 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 urso added the review label Nov 8, 2017

Show outdated Hide outdated libbeat/common/flags.go Outdated
Show outdated Hide outdated libbeat/common/flags.go Outdated
Show outdated Hide outdated libbeat/common/flags.go Outdated
Show outdated Hide outdated libbeat/common/flags.go Outdated
Show outdated Hide outdated libbeat/common/flags.go Outdated
Show outdated Hide outdated libbeat/common/flags.go Outdated
Show outdated Hide outdated libbeat/common/flags.go Outdated

@urso urso added in progress and removed review labels Nov 8, 2017

@ph

ph approved these changes Nov 8, 2017

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 
assert.Equal(t, test.expected, flag.List())
})
}
}

This comment has been minimized.

@ph

ph Nov 8, 2017

Member

wohoo tests :)

@ph

ph Nov 8, 2017

Member

wohoo tests :)

Show outdated Hide outdated libbeat/common/flags.go Outdated
Show outdated Hide outdated libbeat/common/flags.go Outdated
Show outdated Hide outdated libbeat/common/flags.go Outdated
Show outdated Hide outdated libbeat/common/flags.go Outdated
Show outdated Hide outdated libbeat/common/flags.go Outdated
Show outdated Hide outdated libbeat/common/flags.go Outdated
Show outdated Hide outdated libbeat/common/flags.go Outdated
Show outdated Hide outdated libbeat/common/flags.go Outdated
Show outdated Hide outdated libbeat/common/flags.go Outdated
Show outdated Hide outdated libbeat/common/flags.go Outdated
Show outdated Hide outdated libbeat/common/flags.go Outdated
Show outdated Hide outdated libbeat/common/flags.go Outdated
Show outdated Hide outdated libbeat/common/flags.go Outdated
Show outdated Hide outdated libbeat/common/flags.go Outdated
Show outdated Hide outdated libbeat/common/flags.go Outdated
Show outdated Hide outdated libbeat/common/flags.go Outdated
Show outdated Hide outdated libbeat/common/flags.go Outdated

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

@urso urso changed the title from Improve custom config based flag handlers to Improve custom config flag handlers Nov 9, 2017

@ph

ph approved these changes Nov 10, 2017

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

@ph

This comment has been minimized.

Show comment
Hide comment
@ph

ph Nov 10, 2017

Member

@urso Would you mind squashing it?

Member

ph commented Nov 10, 2017

@urso Would you mind squashing it?

Improve custom config flag handlers
- 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
```

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

2 of 5 checks passed

beats-ci Build started sha1 is merged.
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
CLA Commit author is a member of Elasticsearch
Details
hound No violations found. Woof!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment