-
Notifications
You must be signed in to change notification settings - Fork 307
Use docker-setup.sh from upstream .ci repo (#1227) #1229
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
Use docker-setup.sh from upstream .ci repo (#1227) #1229
Conversation
The custom docker-setup.sh script here is not materially different from the shared script in the `.ci` repository. The only difference were pulling and starting the elasticsearch image. This should not affect CI materially and the consistency gain we get using the same `docker-setup.sh` file everywhere is preferrable to having a slightly different script here.
136d87a
to
7684d55
Compare
Here is an interesting case... The tests for 11.x are failing
Looking at the log messages we see this is likely due to how ES is configured WRT streams: main
11.x
The test fails because there is no 400 returned for the second event, we see:
Is this an ES configuration issue? How should we update the 11.x plugin to account for this? Do we need to change the test? The test matrix? |
Long story short, we missed backporting this change - https://github.com/logstash-plugins/logstash-output-elasticsearch/pull/1220/files#diff-4ac1f3b518168b2e30c23d2c20e6eb3c4fa19c45a560bf481bca5641d41e968a If we apply the following change, CI will be happy as I have validated on my local. diff --git a/spec/integration/outputs/metrics_spec.rb b/spec/integration/outputs/metrics_spec.rb
index 4fa31c1..08bd85e 100644
--- a/spec/integration/outputs/metrics_spec.rb
+++ b/spec/integration/outputs/metrics_spec.rb
@@ -5,7 +5,10 @@ describe "metrics", :integration => true do
require "logstash/outputs/elasticsearch"
settings = {
"manage_template" => false,
- "hosts" => "#{get_host_port()}"
+ "hosts" => "#{get_host_port()}",
+ # write data to a random non templated index to ensure the bulk partially fails
+ # don't use streams like "logs-*" as those have failure stores enabled, causing the bulk to succeed instead
+ "index" => "custom_index_#{rand(10000)}"
}
plugin = LogStash::Outputs::ElasticSearch.new(settings)
end |
This commit backports the part of https://github.com/logstash-plugins/logstash-output-elasticsearch/pull/1220/files#diff-4ac1f3b518168b2e30c23d2c20e6eb3c4fa19c45a560bf481bca5641d41e968a that accounts for ES main/9.x. See logstash-plugins#1229 (comment) for detailed breakdown.
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!
The custom docker-setup.sh script here is not materially different from the shared script in the
.ci
repository. The only difference were pulling and starting the elasticsearch image. This should not affect CI materially and the consistency gain we get using the samedocker-setup.sh
file everywhere is preferrable to having a slightly different script here.Thanks for contributing to Logstash! If you haven't already signed our CLA, here's a handy link: https://www.elastic.co/contributor-agreement/