feat: gradle build to support Elasticsearch 8.x #6
Conversation
There was a problem hiding this comment.
Hi, thanks for the PR! Good work.
As a general note, when submitting PRs to Raudikko (and I believe most other projects), please try to build a logical set of commits on top of each other, instead of bundling all your changes into a single commit. Sometimes it's even a good idea to open several PRs in order to get the uncontroversial foundation merged before the actual changes that you're targeting.
So in this case the Gradle version update has nothing to do with the rest of the changes and while it's probably overkill to open a separate PR for that, having it as a separate commit on bottom of other changes sounds like a good idea.
Similarly the "build with Docker"-stuff is unrelated to the rest of the changes and whether we'll want to merge or not is orthogonal to other issues here. No need to have it in this PR, just open a separate one and explain there what problem it solves and we can take a look at it separately.
Furthermore, I'm a bit concerned that now there's logical duplication. If we'd like to add a new flag, we'd have to remember to update two places. One solution would be to have both filters delegate their work to a shared class. But truthfully, that feels like overkill in this case. Then again, I would feel compelled to at least leave comments on both ends of the duplication, mentioning that you need to update the other end as well. (Not that anyone reads those, but hey, we tried our best.)
Now, as for the actual solution, a few weeks ago we discussed with @hakuzumon whether we want to support old versions and have separate source sets or if we'd rather just drop support for the old versions and then make releases for them from a maintenance branch, if that is ever needed. We decided for the second option. No need to complicate the build as we probably rarely need to make builds for old versions.
So I won't merge this as it is, but this feels like fine work, so if you submit the same thing simplified so that there's no multiple source sets, no support for old versions and no extra things, I will.
Thanks again, great to have support for 8.x!
Tried to use 7.6 and it works but Idea gives some errors on build file.
| @@ -34,7 +34,7 @@ val distributionZip = tasks.register<Zip>("distributionZip") { | |||
| from(file("src/main/plugin-metadata")) { | |||
| expand( | |||
| "pluginVersion" to project.version, | |||
| "javaVersion" to System.getProperty("java.specification.version"), | |||
There was a problem hiding this comment.
I don't know if this has any impact but now it follows the toolchain java version and not the version running gradle
|
Agree with you 100%. Makes it far simpler to just drop the 7.x support. |
|
btw. what comes to unrelated changes I would consider adding most (maybe all) of .idea to .gitignore. Seems like changes there are related to my environment and tracking those is really annoying :/ |
Yeah, I think I'll do that. It's really annoying that whenever JetBrains manages to fix some of the environment related things, others will creep in. In theory, most of the |
|
Thanks again and sorry for the delay! |
ES8 requires Java 17 and plugin SPI has minor change compared to 7.x which makes it backwards incompatible.
ES8 support in the build works by defining new source set for ES8 that contains the only incompatible class (RaudikkoTokenFilterFactory) that gets it's own ES8 classpath during development.
When running the build against 8.x target java toolchain is switched to 17 and the main source set is configured to include the 8.x compatible FilterFactory and to exclude the 7.x version.
Docker compose added for easier verification that the plugin works.
resolves: #5