search

logstash-plugins / logstash-filter-elasticsearch

Elasticsearch filter for Logstash
Apache License 2.0
21 stars 82 forks source link

Add proxy support #134

Closed roaksoax closed 4 years ago

kares commented 4 years ago

@yaauie raised concerns about relying on internals when this was done on the ES input (after the output was merged), so we did it differently: https://github.com/logstash-plugins/logstash-input-elasticsearch/pull/114/files ... not sure it matters much esp. if the integration plugin is down on us sooner than later, in which case we could unify easily.

roaksoax commented 4 years ago

@kares can you clarify your comment? Either way, my changes are very similar to yours with the exception that it is using 'uri' instead of 'uri_or_empty', and validates 'uri' instead.

kares commented 4 years ago

@roaksoax see the concerns raised at the linked PR: https://github.com/logstash-plugins/logstash-input-elasticsearch/pull/114#pullrequestreview-346986372 ES output was done the same way - using config_init which is internal API compared to using validate_value

roaksoax commented 4 years ago

@yaauie thoughts on the above?

yaauie commented 4 years ago

@roaksoax I would echo the above-linked review I made of the ES Input, for the exact same reasons. Overriding config_init has side effects (e.g., changing the plugin's record of explicit as-given arguments @original_params), and overriding the validation step only more appropriately matches the desired scope.