-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Rename shipper config to beat #1165
Conversation
@@ -218,16 +219,30 @@ func (b *Beat) LoadConfig() error { | |||
// Disable stderr logging if requested by cmdline flag | |||
logp.SetStderr() | |||
|
|||
logp.Debug("beat", "Initializing output plugins") | |||
if b.Config.Beat != nil && b.Config.Shipper != nil { | |||
return fmt.Errorf("beat and shipper are set in config. Only one can be enabled. shipper is deprecated, used beat.") |
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.
s/used/use/
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.
fixed
I think we should take some time and discuss about how to structure the configuration file. It looks a bit chaotic at the moment. Until then, I would say to postpone merging this PR. |
@ruflin Could please you address/include the reason for this change in the PR description for those that aren't privy to our Slack discussions. I think it's good to be transparent. |
Code LGTM, but a winlogbeat test seems to be failing. I kind of agree with @monicasarbu that we should consider if "beat" is the best replacement here or we should look for something more specific. |
@@ -42,7 +42,7 @@ func TestConfigValidate(t *testing.T) { | |||
map[string]interface{}{"other": "value"}, | |||
}, | |||
"1 error: Invalid top-level key 'other' found. Valid keys are " + | |||
"logging, output, shipper, winlogbeat", | |||
"logging, output, beat, winlogbeat", |
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 keys here are sorted alphabetically which is why the test case is failing. Make "beat"
the first element.
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.
Yep, I moved it to the beginning.
@monicasarbu @tsg @andrewkroh Lets have again a discussion on this to make sure we are all on the same page. Will update the commit and PR with more details afterwards. |
This change is backward compatible, as shipper is still accepted but a warning will be printed out that shipper is deprecated. In case beat and shipper are set, an error is returned. * Docs were updated with new name * CHANEGLOG updated with change message
Code LGTM |
Closing as we decided to go in a different direction with the config file. |
This change is backward compatible, as shipper is still accepted but a warning will be printed out that shipper is deprecated. In case beat and shipper are set, an error is returned.