-
Notifications
You must be signed in to change notification settings - Fork 81
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
Support for Logstash 8.0.x #152
Support for Logstash 8.0.x #152
Conversation
Signed-off-by: Deep Datta <deedatta@amazon.com>
Signed-off-by: Deep Datta <deedatta@amazon.com>
Signed-off-by: Deep Datta <deedatta@amazon.com>
… of 7.13.2. Also add v8.3.2 in the test matrix for the github CI workflow. Signed-off-by: Deep Datta <deedatta@amazon.com>
fa500d5
to
bef2b2f
Compare
…stro. This was causing a newly added integration test to fail for OpenDistro Signed-off-by: Deep Datta <deedatta@amazon.com>
…:disabled scenario Signed-off-by: Deep Datta <deedatta@amazon.com>
@@ -7,10 +7,10 @@ services: | |||
context: ../../ | |||
dockerfile: scripts/Dockerfile | |||
args: | |||
- LOGSTASH_VERSION=${LOGSTASH_VERSION:-7.13.2} |
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.
why are we changing this default value to 8.x?
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.
I made the change because its the latest version of Logstash and it might be good to test features/fixes against it given user interest in these versions as seen in issues like #149. Let me know if its better to keep the old version.
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.
No strong opinion against this change, just that we might end up in updating this for every new version.
Shouldn't this be updated too? |
…se instead of storing them in the repo. However the ecs-v8 schemas are stored since we needed to substitute the following data types that are not supported by OpenSearch match_only_text -> text (with norms:false) constant_keyword -> keyword wildcard -> keyword Signed-off-by: Deep Datta <deedatta@amazon.com>
Thanks Vijayan for pointing this out, this file was pulling the ecs schemas at build time from the ecs GitHub release repo. I have made changes to the script to pull the ecs_v1 schemas and name them as expected by our plugin, instead of having them in our repo. However I have left the v8 schemas in the repo as I needed to modify 3 field types in them to make it compatible with OpenSearch since https://www.elastic.co/guide/en/ecs/current/ecs-faq.html#type-interop says its ok to substitute field types from the same family. |
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.
Thanks for updating the download process. This approach makes sense for v1.
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.
LGTM! Thank you for addressing the feedback comments.
@deepdatta FAR: Can you add yourself to maintainers list? https://github.com/opensearch-project/logstash-output-opensearch/blob/main/MAINTAINERS.md#current-maintainers |
Description
Supports running the plugin with Logstash 8.x without explicitly setting ecs_compatibility=>disabled
Adds ecs compatible default templates to work with default ecs_compatibility values.
Signed-off-by: Deep Datta deedatta@amazon.com
Issues Resolved
#132
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.