-
Notifications
You must be signed in to change notification settings - Fork 12
Migrate the path hierarchy plugin to Elastic 8.18 #35
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
Migrate the path hierarchy plugin to Elastic 8.18 #35
Conversation
Thanks to @nicenatalia Co-authored-by: Natalia Escalera <[email protected]>
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.
Pull Request Overview
Migrate the path hierarchy plugin to Elastic 8.18.1 with formatting updates, version bump, and license change
- Bump Elasticsearch and plugin versions to 8.18.1 and apply spotless formatting
- Update license from MIT to AGPL and adjust README
- Refactor builder and aggregator classes to multiline argument style and remove deprecated imports
Reviewed Changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| gradle.properties | Updated es_version and plugin_version to 8.18.1 |
| README.md | Changed license statement to AGPL |
| LICENSE | Replaced MIT license text with AGPL v3 full text |
| src/yamlRestTest/java/org/opendatasoft/elasticsearch/RestApiYamlIT.java | Added missing import for parameterized tests |
| src/test/java/org/opendatasoft/elasticsearch/PathHierarchyTests.java | Inlined JSON strings in test setup |
| src/main/java/org/opendatasoft/elasticsearch/search/aggregations/bucket/PathSortedTree.java | Removed extraneous blank lines |
| src/main/java/org/opendatasoft/elasticsearch/search/aggregations/bucket/*.java | Reformatted constructors and method calls to multiline style; removed deprecated imports |
Comments suppressed due to low confidence (3)
src/test/java/org/opendatasoft/elasticsearch/PathHierarchyTests.java:19
- [nitpick] The variable name
ordersis a JSON string rather than a list; consider renaming toorderJsonfor clarity.
String orders = "[{\"_key\": \"asc\"}, {\"_count\": \"desc\"}]";
src/main/java/org/opendatasoft/elasticsearch/search/aggregations/bucket/PathHierarchyAggregationBuilder.java:35
- Consider adding a unit test to verify that
getMinimalSupportedVersion()returnsTransportVersions.V_8_0_0as expected.
public TransportVersion getMinimalSupportedVersion() {
src/main/java/org/opendatasoft/elasticsearch/search/aggregations/bucket/DateHierarchyAggregationBuilder.java:149
- [nitpick] The comment references
timezoneAwarewhich is no longer a parameter; please update or remove it to avoid confusion.
// ES 8.x introduces field validation. Setting timezoneAware to false to avoid duplication of the timezone field
|
@garaud I didn't manage to track the cluster info issue yet. I've added a test which is failing here and working on master (7.17.28). |
avoid having a weird 405 HTTP on 'GET /' when you './gradlew run'.
Migrate the plugin for Elastic 8.18.1
apply spotless formatting
update the elasticsearch version to 8.18.1
Adapt the plugin to Elastic 8
Thanks to @nicenatalia
Co-authored-by: Natalia Escalera [email protected]
update the LICENSE to AGPL
add the yaml REST test framework dependency to the build.gradle file