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

Load template by default, depending on the version #1993

Merged
merged 1 commit into from
Jul 13, 2016

Conversation

tsg
Copy link
Contributor

@tsg tsg commented Jul 8, 2016

This queries Elasticsearch for the version and choses between the
default template file (specified by the template.path option) and
the ES 2x version of the template file (specified by the new
template.versions.2x.path option).

There's also a change in the defaults. With all the template
options commented out, the code now loads the template. Previously,
the template loading was the default via the sample configs only.

I also did a small change in the output plugins API, now they receive
the beatname in a clean way.

Fixes #1740.

@tsg tsg added in progress Pull request is currently in progress. review discuss Issue needs further discussion. labels Jul 8, 2016
@tsg
Copy link
Contributor Author

tsg commented Jul 8, 2016

Not complete yet, but I'd like your feedback on the new config options @elastic/beats.

}
}

err = json.Unmarshal(body, &response)
Copy link

Choose a reason for hiding this comment

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

make sure body is fully consumed and closed correctly. Otherwise underlying connection can not be re-used (go http module uses keep-alive in order to reuse TCP connections) + need to time out underlying connection in order to close socket.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be the case already because the body is coming from a call to execHTTPRequest which does ReadAll and Close.

@tsg
Copy link
Contributor Author

tsg commented Jul 12, 2016

Comments mostly addressed, let's see if tests pass.

@tsg
Copy link
Contributor Author

tsg commented Jul 12, 2016

jenkins, retest it

@tsg tsg removed the in progress Pull request is currently in progress. label Jul 12, 2016
@tsg
Copy link
Contributor Author

tsg commented Jul 12, 2016

jenkins, retest it

# These settings can be adjusted to load your own template or overwrite existing ones.

# Set to false to disable template loading.
# template.enabled: true
Copy link
Member

Choose a reason for hiding this comment

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

Remove the space after #

@ruflin
Copy link
Member

ruflin commented Jul 12, 2016

LGTM. Only two minor comments.

This queries Elasticsearch for the version and choses between the
default template file (specified by the `template.path` option) and
the ES 2x version of the template file (specified by the new
`template.versions.2x.path` option).

There's also a change in the defaults. With all the template
options commented out, the code now loads the template. Previously,
the template loading was the default via the sample configs only.

I also did a small change in the output plugins API, now they receive
the beatname in a clean way.

Fixes elastic#1740.
@ruflin
Copy link
Member

ruflin commented Jul 13, 2016

LGTM. Waiting for green, but will ignore appveyor.

@ruflin ruflin merged commit 787f45b into elastic:master Jul 13, 2016
tsg pushed a commit to tsg/beats that referenced this pull request Jul 13, 2016
Since we removed the template stuff from the default config, we can add
the user/pass instead so it's easy to enabled https. It also updates the
sample user/pass used, to match the default in Shield, closing elastic#1735.

Depends on elastic#1993.
ruflin pushed a commit that referenced this pull request Jul 13, 2016
Since we removed the template stuff from the default config, we can add
the user/pass instead so it's easy to enabled https. It also updates the
sample user/pass used, to match the default in Shield, closing #1735.

Depends on #1993.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Issue needs further discussion. review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants