Upgrade to elasticsearch v8.7.0 #867
Conversation
MarvinZeising
force-pushed
the
upgrade-to-elasticsearch-8.7.0
branch
2 times, most recently
from
b5a8d6f to
d6c0acb
Compare
Codecov ReportPatch coverage:
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## master #867 +/- ##
==========================================
- Coverage 85.87% 85.77% -0.10%
==========================================
Files 50 52 +2
Lines 2350 2460 +110
==========================================
+ Hits 2018 2110 +92
- Misses 173 182 +9
- Partials 159 168 +9
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thank you for working on this!
The tests indeed fail now because the container never becomes healthy. I guess something is changed in the API so we can't use the same method to test container readiness. If you still want to debug this, I suggest starting with WithLogWriter and WithDebugMode options to look into what happens inside the container, and where does the flow get stuck.
If there was a change in the API, we'll have to support multiple health checks depending on preset version.
|
Hey @MarvinZeising, It means that only one major version was supported, and versions 5 and 6 were not. As I don't use Elastic in my current projects, I'm not even sure how important is this support. I think that maybe version 6 is somewhat important to support, but 5 can be dropped... Anyway, if you decide to work on supporting 7 and 8 side by side (in code and in tests), maybe you could look into supporting version 5 and 6, or only 6, at the same time. For now, I modified the tests to only run on version 7, and removed 5 and 6 from the list of supported versions in readme. |
# Conflicts: # go.sum # preset/elastic/preset_test.go
|
Thanks for your review @orlangure. Versions 5 and 6 of elasticsearch are already at end of life. If they're not supported officially and no one is using them (because they didn't work with gnomork before), I don't think we should add this. The container wasn't getting healthy because the endpoint is secure. Disabling TLS fixed. Since in my company, we use that setting in v7 as well (because it's a sensible default setting) I didn't even add a version check. I tried to combine the tests for the different versions like you did. But that proved to be difficult since there's no common interface for the clients. So I figured the cleanest approach would just be to separate these tests and use the respective client library. |
|
I don't mind separating the tests, if this is easier. But something confuses me. We now use client v8 in the preset regardless of versions (7 or 8). Does this client know how to connect and work with both Elastic 7 and 8? Because a server test for example fails because it sends v7.9.2 as the version, and Elastic client v8 can't connect to it. I would expect that existing tests continue to work if they use v7. So something wasn't backwards compatible between 7.9 and 7.17. What do you think we should do with this issue? |
|
Thanks for your comment, @orlangure! I missed two places to update the default elasticsearch version in. I fixed that and now the tests succeed locally. My idea was that I also tried the server test with 7.17.9 (by specifying that in |
I'm proposing an upgrade to elasticsearch v8.7.0, as requested in #868.
The tests didn't finish locally (even before my changes), so I'm interested to see what the CI action says.