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.

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

Add Bucket sort aggregation #2229

Merged
merged 2 commits into from
Oct 22, 2024
Merged

Add Bucket sort aggregation #2229

merged 2 commits into from
Oct 22, 2024

Conversation

gaetan-petit
Copy link
Contributor

I tried to stick to existing code organization as much as possible.
The tests are very minimal.

Fixes #2228

@ruflin
Copy link
Owner

ruflin commented Oct 21, 2024

Thanks for the contribution. Could you add an entry to the changelog file?

Overall change LGTM. Glad to see the addition of the functional test to ensure it works.

There is a test failure in one of the builds but I will run it again as I'm not sure how this could be related:

There was 1 failure:

1) Elastica\Test\PipelineTest::testDeletePipeline
an exception should be raised!

/home/runner/work/Elastica/Elastica/tests/PipelineTest.php:142

@ruflin
Copy link
Owner

ruflin commented Oct 21, 2024

All green now. Change LGTM. Can you add a commit with a changelog entry so we can get this in?

@gaetan-petit
Copy link
Contributor Author

All green now. Change LGTM. Can you add a commit with a changelog entry so we can get this in?

Sure I'll do it tomorrow! Thank you @ruflin

@gaetan-petit
Copy link
Contributor Author

@ruflin done!
Now that I'm thinking I opened this PR against 8.x, are you going to publish any new release for the 7.x branch?

@ruflin
Copy link
Owner

ruflin commented Oct 22, 2024

Thx for opening it against 8.x, I rather have backports then forward ports. For the 7.x release: It depends if there are changes worth releasing. I will not make any promises but I would still recommend to backport it to 7.x. If you need a release there with this, please let me know ;-)

I'm waiting now in CI, as soon as all passes I'll get it merged.

@ruflin ruflin merged commit af2c684 into ruflin:8.x Oct 22, 2024
18 checks passed
@ruflin
Copy link
Owner

ruflin commented Oct 22, 2024

Thank you for the contribution! Looking forward to the 7.x backport ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing Bucket Sort Aggregation
2 participants