-
Notifications
You must be signed in to change notification settings - Fork 20
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
Set elasticsearch client version to >=7.17,<8.0.0 #230
Conversation
Codecov Report
@@ Coverage Diff @@
## master #230 +/- ##
==========================================
- Coverage 99.34% 99.29% -0.05%
==========================================
Files 54 54
Lines 2139 2142 +3
==========================================
+ Hits 2125 2127 +2
- Misses 14 15 +1
Continue to review full report at Codecov.
|
Hi @stefan-k, thanks a lot for your contribution. Do you have any idea why Best regards, |
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 a lot for your contribution. Apart from my inline comment, I noticed that the tests have not been updated to cover the new behaviour. Could you add a corresponding test case, please?
The latest versions of the elasticsearch client have a compatibility mode which can also be used for newer server versions; however, the other way around does not work. Fixing the version to >=7.17,<8.0.0 is therefore a save option for the near future. This commit also includes a fix for the case where "resource_status" is not part of `resource_attributes`, which occasionally caused crashes. If this key is not present, `resource_status` will be set to an empty string.
I had this issue in the back of my head but haven't had time yet to get back to it. Thanks for the feedback, I tried to address the suggestions in the recent force push. I guess the CI is failing due to #239.
I was not able to figure out why, but we do see this regularly. The only suspicion I have is that it may happen with drones "resurrected" after a crash of C/T. Unfortunately our C/T does crash occasionally because monitoring plugins fail due to network issues (not sure how to fix this when considering the discussion in #189, but that's a different topic). |
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 a lot for your contribution. This looks fine to me.
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.
Looks good to go.
The latest versions of the Elasticsearch client have a compatibility mode which can also be used for newer server versions; however, the other way around does not work. Fixing the version to >=7.17,<8.0.0 is therefore a save option for the near future.
This commit also includes a fix for the case where "resource_status" is not part of
resource_attributes
, which occasionally caused crashes. If this key is not present,resource_status
will be set to an empty string.Also, newer versions of the Elasticsearch client require the "scheme" parameter to be set. By setting this already, it will be easier to eventually transition to client version 8.