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

fix: simplify project dependencies, fix tests #39

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vit-zikmund
Copy link

Hello Vojtěchu ;) @vhyza,

My colleagues and I tried to experiment with your plugin, but realized the last published version is too old for our ES instance. Despite not being a java guy at all, I decided to give a custom build a try. Following your instructions to run mvn package indeed produced a .zip package, but the test was throwing an exception that was pretty impossible to decipher. I didn't feel like ignoring that and decided to make it work. There were some hints on the internet, that the dependencies might be clashing, so I tried to untangle them. I spend some non-trivial time on that and found those could be really simplified quite a bit. The ultimate fix was to bump the maven-surefire-plugin, though 😮‍💨

Going through all this - and succeeding - I thought I might share the result with you. I actually also found quite useful the contribution from @crabhi, which allowed me to write a very similar building Dockerfile, that dynamically sets both the (plugin.version replaced by) project.version and the targeted elasticsearch.version:

ARG elasticsearch_version=8.7.1
FROM maven:3-eclipse-temurin-17-alpine as lemmagen
ARG elasticsearch_version
# we need a non-root user for the plugin tests to pass
RUN adduser -s /sbin/nologin -D builder
USER builder
# prevent access error in case of running this (otherwise ephemeral) image as a non-root user
ENV MAVEN_CONFIG=/home/builder/.m2
WORKDIR /home/builder/elasticsearch-analysis-lemmagen

COPY --chown=builder:builder . ./
RUN set -eu ;\
    mvn --no-transfer-progress versions:set -DnewVersion=$elasticsearch_version ;\
    mvn --no-transfer-progress package -Delasticsearch.version=$elasticsearch_version

With all that I'd like to thank you for sharing your code with the open source community, so we can all take part.

Many of the explicitly referenced dependencies are actually a transitive dependency of some of the others. Especially with test dependencies, where some transitive dependencies were excluded in favor of yet another explicit ones. This was quite hard to untangle and if there ever was a trouble with those versions, it's likely not the case with 8.7.1 and above.

I was also quite confused with the 'plugin.version' property, which seemed to mimic the implicit 'project.version'. These were also likely intermixed in plugin-descriptor.properties, so this change tries to fix that by replacing 'plugin.version' with 'project.version' where appropriate, ultimately removing the 'plugin.version'. Same was done with 'lucene.version' which was there only to draw in an explicit version of 'org.apache.lucene:lucene-test-framework', despite the correct version already being a dependency of 'org.elasticsearch.test:framework'.

Despite all the simplifications above the tests still had problems executing. This was ultimately resolved by using a newer version of the maven-surefire-plugin.
@vhyza
Copy link
Owner

vhyza commented Mar 1, 2024

Hi @vit-zikmund,

thank you for the PR. I'm sorry I didn't get back to you sooner. I'm quite swamped with other projects and as I do not use the plugin for years now, my motivation is not the strongest.

Anyway, I'd like to get back one day, but it will probably take some more time. I'm sorry about that.

@vit-zikmund
Copy link
Author

Hi @vhyza, what a nice surprise you're even still there! 👍 TBH, we didn't push it much further ourselves...

While we made it build/bundle for whatever ES version we desire, on using it, we discovered once the plugin gets used it throws some banal permissions error coming from log4j that likley needs some whitelisting somewhere. Maybe some policy file or what. But the Java docs about AccessController are dense as Java world can be for an outsider like me, and ES even dropped their plugin dev docs for the later versions. It seems we're 🤏 this close to figure it out, but then we get swamped with other stuff too...

So out of sheer joy of sharing I'm pasting it here, if you'd (or anyone) know where to go:

java.security.AccessControlException: access denied ("java.lang.RuntimePermission" "getClassLoader")
    at java.base/java.security.AccessControlContext.checkPermission(AccessControlContext.java:488)
    at java.base/java.security.AccessController.checkPermission(AccessController.java:1071)
    at java.base/java.lang.SecurityManager.checkPermission(SecurityManager.java:411)
    at java.base/java.lang.ClassLoader.checkClassLoaderPermission(ClassLoader.java:2070)
    at java.base/java.lang.Thread.getContextClassLoader(Thread.java:2384)
    at org.slf4j.impl.SimpleLoggerConfiguration$1.run(SimpleLoggerConfiguration.java:102)
    at org.slf4j.impl.SimpleLoggerConfiguration$1.run(SimpleLoggerConfiguration.java:100)
    at java.base/java.security.AccessController.doPrivileged(AccessController.java:319)
    at org.slf4j.impl.SimpleLoggerConfiguration.loadProperties(SimpleLoggerConfiguration.java:100)
    ...

Thank you once again and take it easy :)

@vhyza vhyza mentioned this pull request Jul 4, 2024
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.

2 participants