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.

Sign up for GitHub

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

Jump to bottom

packetbeat protocols enabled config #1988

Merged
merged 1 commit into from
Jul 19, 2016
Merged

packetbeat protocols enabled config #1988

merged 1 commit into from
Jul 19, 2016

Conversation

urso
Copy link

@urso urso commented Jul 7, 2016

blocked by #1987

@urso urso force-pushed the enh/protocols-enable-setting branch 2 times, most recently from a8b16d4 to 669c16c Compare July 8, 2016 15:05
@urso urso removed the blocked label Jul 8, 2016
@monicasarbu
Copy link
Contributor

LGTM, waiting for green

@urso
Copy link
Author

urso commented Jul 8, 2016

some additional changes next to adding enable flag to every protocol plugin:

  • add enable flag to flows
  • renamed enabled flag to enable in ICMP protocol plugin + change default to true (as config says default is false, but sets enabled to true). The enabled flag was not used in code, but enable flag is.

urso
urso reviewed Jul 8, 2016
View reviewed changes
packetbeat/config/config.go
@@ -35,8 +35,9 @@ type InterfacesConfig struct {
}

type Flows struct {
Timeout string
Period string
Enable bool `config:"enable"`
Copy link
Author

Choose a reason for hiding this comment

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

just discovered the default value being false, but not true as documented.

Copy link
Author

Choose a reason for hiding this comment

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

fixed by introducing (*Flows).Enabled().

@urso urso force-pushed the enh/protocols-enable-setting branch 5 times, most recently from 73cfdf8 to f10cbb9 Compare July 8, 2016 16:49
@ruflin
Copy link
Member

ruflin commented Jul 8, 2016

@urso Same problem here as with the output PR. You used enable, Metricbeat uses enabled.

@urso
Copy link
Author

urso commented Jul 9, 2016

@ruflin will change metricbeat to enable ;)

@ruflin
Copy link
Member

ruflin commented Jul 11, 2016

Both works for me. I tried to find some example for enable or enabled in other projects. For example elasticearch (at least the tests) seem to use enabled: https://github.com/elastic/elasticsearch/blob/master/rest-api-spec/src/main/resources/rest-api-spec/test/search/60_query_string.yaml#L10 Most other projects I found also use enabled, but couldn't find "hard facts" for on one or the other.

@urso urso force-pushed the enh/protocols-enable-setting branch 2 times, most recently from 6dcca56 to 8eb0a52 Compare July 12, 2016 15:10
@urso
Copy link
Author

urso commented Jul 12, 2016

changed (*common.Config).Enabled method to look for enabled config setting. Did require to update docs for output modules too.

ruflin
ruflin reviewed Jul 12, 2016
View reviewed changes
packetbeat/docs/reference/configuration/packetbeat-options.asciidoc
@@ -247,6 +254,12 @@ packetbeat.protocols.thrift:

The following options are available for all protocols:

===== enabled

The enabled setting is a boolean setting to enable or disable protocols without having to comment out configuration sections.. If set to false, the protocol is disabled.
Copy link
Member

Choose a reason for hiding this comment

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

sections..

@urso urso force-pushed the enh/protocols-enable-setting branch 3 times, most recently from b9a2e8c to e8a51d1 Compare July 12, 2016 15:27
ruflin
ruflin reviewed Jul 12, 2016
View reviewed changes
packetbeat/docs/reference/configuration/packetbeat-options.asciidoc
@@ -283,9 +296,9 @@ The per protocol transaction timeout. Expired transactions will no longer be cor

You can specify the following options in the `icmp` section of the +{beatname_lc}.yml+ config file:

===== enabled
===== enable
Copy link
Member

Choose a reason for hiding this comment

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

enabled

Copy link
Author

Choose a reason for hiding this comment

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

No idea why this still shows as enabled. Locally it's enabled, only opening the github editor for file in feature branch shows enabled. Some github caching going on here?

@urso urso force-pushed the enh/protocols-enable-setting branch from e8a51d1 to 7963a3a Compare July 12, 2016 15:59
@ruflin
Copy link
Member

ruflin commented Jul 12, 2016

@urso There is also an other issue with Enabled Method and variable: https://travis-ci.org/elastic/beats/jobs/144204760#L429

@urso urso force-pushed the enh/protocols-enable-setting branch from 7963a3a to ab781c6 Compare July 19, 2016 11:20
@urso
Copy link
Author

urso commented Jul 19, 2016

@ruflin rebased and fixed build

@tsg tsg changed the title packetbeat protocols enable config packetbeat protocols enabled config Jul 19, 2016
@tsg tsg merged commit e6c8026 into elastic:master Jul 19, 2016
@urso urso deleted the enh/protocols-enable-setting branch February 19, 2019 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@urso @monicasarbu @ruflin @tsg