Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add back ESv6 support with e2e test #5704

Merged
merged 6 commits into from
Jul 4, 2024
Merged

Add back ESv6 support with e2e test #5704

merged 6 commits into from
Jul 4, 2024

Conversation

pavolloffay
Copy link
Member

@pavolloffay pavolloffay commented Jul 3, 2024 •
edited by yurishkuro
Loading

Reverts parts of #5439

Motivation: some vendors still provide commercial support for ES v6 to customers, even though ESv6 EOL was Nov 2020.

@codecov Codecov
Copy link

codecov bot commented Jul 3, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.87%. Comparing base (bbd863f) to head (42b0cb5).
Report is 4 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #5704      +/-   ##
==========================================
+ Coverage   96.40%   96.87%   +0.47%     
==========================================
  Files         334      334              
  Lines       16144    16146       +2     
==========================================
+ Hits        15563    15642      +79     
+ Misses        404      334      -70     
+ Partials      177      170       -7
Flag Coverage Δ
badger_v1 8.06% <0.00%> (-0.01%) ⬇️
badger_v2 1.90% <0.00%> (-0.01%) ⬇️
cassandra-3.x-v1 16.62% <0.00%> (-0.01%) ⬇️
cassandra-3.x-v2 1.82% <0.00%> (-0.01%) ⬇️
cassandra-4.x-v1 16.62% <0.00%> (-0.01%) ⬇️
cassandra-4.x-v2 1.82% <0.00%> (-0.01%) ⬇️
elasticsearch-6.x-v1 18.80% <33.33%> (?)
elasticsearch-7.x-v1 18.86% <33.33%> (-0.01%) ⬇️
elasticsearch-8.x-v1 19.05% <0.00%> (-0.01%) ⬇️
elasticsearch-8.x-v2 19.04% <0.00%> (-0.02%) ⬇️
grpc_v1 9.45% <0.00%> (-0.02%) ⬇️
grpc_v2 7.41% <0.00%> (-0.01%) ⬇️
kafka 9.75% <0.00%> (-0.01%) ⬇️
opensearch-1.x-v1 18.90% <33.33%> (-0.02%) ⬇️
opensearch-2.x-v1 18.91% <33.33%> (+<0.01%) ⬆️
opensearch-2.x-v2 18.90% <33.33%> (-0.02%) ⬇️
unittests 95.26% <100.00%> (+1.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pavolloffay pavolloffay added changelog:new-feature Change that should be called out as new feature in CHANGELOG changelog:skip Trivial change that does not require an entry in CHANGELOG and removed changelog:new-feature Change that should be called out as new feature in CHANGELOG labels Jul 3, 2024
@pavolloffay
Copy link
Member Author

@yurishkuro I have added ski-changelog to avoid advertising this. We (RH) will keep sponsoring the ES6 compatibility.

@pavolloffay
Fix
0f6bc35 
Signed-off-by: Pavol Loffay <[email protected]>
@pavolloffay pavolloffay marked this pull request as ready for review July 3, 2024 12:36
@pavolloffay pavolloffay requested a review from a team as a code owner July 3, 2024 12:36
@pavolloffay pavolloffay requested a review from albertteoh July 3, 2024 12:36
yurishkuro
yurishkuro reviewed Jul 3, 2024
View reviewed changes
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something failed in CI for v6

plugin/storage/es/mappings/fixtures/jaeger-dependencies.json Outdated Show resolved Hide resolved
plugin/storage/es/mappings/jaeger-dependencies.json Outdated Show resolved Hide resolved
@pavolloffay
Fix
f5f0d17 
Signed-off-by: Pavol Loffay <[email protected]>
@pavolloffay
Fix
c144e7a 
Signed-off-by: Pavol Loffay <[email protected]>
@pavolloffay
Fix
12c5997 
Signed-off-by: Pavol Loffay <[email protected]>
yurishkuro
yurishkuro reviewed Jul 3, 2024
View reviewed changes
plugin/storage/integration/es_index_cleaner_test.go
@@ -33,7 +33,7 @@ const (
spanIndexName = "jaeger-span-2019-01-01"
serviceIndexName = "jaeger-service-2019-01-01"
indexCleanerImage = "jaegertracing/jaeger-es-index-cleaner:latest"
rolloverImage = "jaegertracing/jaeger-es-rollover:latest"
rolloverImage = "jaegertracing/jaeger-es-rollover:1.57.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, I didn't know we do that. That's pretty bad, the tests should always be using current code, i.e. we should just run these directly (they are Go binaries, right?) or do a quick docker build . (it's lightweight, just copies the binary over).

Doesn't need to be in this PR, we can open help wanted ticket.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, this is far from great. I can take a look at it in a separate PR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#5709

@yurishkuro yurishkuro changed the title Add back ES6 support with e2e test Add back ESv6 support with e2e test Jul 4, 2024
@yurishkuro yurishkuro merged commit f202880 into jaegertracing:main Jul 4, 2024
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yurishkuro yurishkuro yurishkuro left review comments

@albertteoh albertteoh Awaiting requested review from albertteoh albertteoh is a code owner automatically assigned from jaegertracing/jaeger-maintainers

Assignees
No one assigned
Labels
area/storage changelog:skip Trivial change that does not require an entry in CHANGELOG storage/elasticsearch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pavolloffay @yurishkuro