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

Upgrade Jetty to version 10.x. #34

Merged
merged 2 commits into from
Aug 9, 2022

Conversation

kdombeck
Copy link
Contributor

Other libraries were upgraded to their latest versions as well.

Implementation for #32

@@ -4,7 +4,7 @@

<groupId>io.logz</groupId>
<artifactId>guice-jersey</artifactId>
<version>1.0.16-SNAPSHOT</version>
<version>1.1.0-SNAPSHOT</version>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I incremented the minor version since this uses a newer version of Jetty.

<source>1.8</source>
<target>1.8</target>
<source>11</source>
<target>11</target>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This "could" be left at 8 but I updated it to 11 since Jetty 10+ requires Java 11+.

<jackson.version>2.10.2</jackson.version>
<hk2-bridge.version>2.6.1</hk2-bridge.version>
<jersey.version>2.36</jersey.version>
<jetty.version>10.0.11</jetty.version>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not upgrade to Jetty 11+ since that requires JakartaEE instead of javax. Guice does not support that yet.

<hk2-bridge.version>2.6.1</hk2-bridge.version>
<jersey.version>2.36</jersey.version>
<jetty.version>10.0.11</jetty.version>
<guice.version>5.1.0</guice.version>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Guice did not need to be upgraded as all tests passed without upgrading it. So it could be downgraded if older version would be preferred.

@@ -38,7 +38,7 @@
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-javadoc-plugin</artifactId>
<version>2.10.4</version>
<version>3.4.0</version>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needed to upgrade this plugin otherwise when building on new JVMs it generated the following error.

Execution attach-javadocs of goal org.apache.maven.plugins:maven-javadoc-plugin:2.10.4:jar failed: An API incompatibility was encountered while executing org.apache.maven.plugins:maven-javadoc-plugin:2.10.4:jar: java.lang.ExceptionInInitializerError: null

Caused by: java.lang.StringIndexOutOfBoundsException: begin 0, end 3, length 2
    at java.lang.String.checkBoundsBeginEnd (String.java:3319)
    at java.lang.String.substring (String.java:1874)
    at org.apache.commons.lang.SystemUtils.getJavaVersionAsFloat (SystemUtils.java:1133)
    at org.apache.commons.lang.SystemUtils.<clinit> (SystemUtils.java:818)

@ofir-popowski
Copy link
Contributor

Thank you for your submission @kdombeck :)

Since your PR changes to use JDK11, I have changed the Github action to use JDK11. Build should run now if you update your branch from master

Copy link
Contributor

@ofir-popowski ofir-popowski left a comment

Choose a reason for hiding this comment

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

I think this should be okay :)

@ofir-popowski ofir-popowski merged commit cf55305 into logzio:master Aug 9, 2022
@kdombeck kdombeck deleted the jetty-10-upgrade branch August 9, 2022 12:55
@ofir-popowski
Copy link
Contributor

@kdombeck okay so, it seems like the current setup to publish this to the maven repository isn't working. I'm going to have to spend some time setting it up correctly (probably with a github action) and let you know once it's published

@kdombeck
Copy link
Contributor Author

@kdombeck okay so, it seems like the current setup to publish this to the maven repository isn't working. I'm going to have to spend some time setting it up correctly (probably with a github action) and let you know once it's published

Thank you!!

@ofir-popowski
Copy link
Contributor

@kdombeck I would like to extend an apology. I haven't had time to do what I said in my previous comment yet. I'll try to prioritize this as much as I can

@kdombeck
Copy link
Contributor Author

@kdombeck I would like to extend an apology. I haven't had time to do what I said in my previous comment yet. I'll try to prioritize this as much as I can

No need to apologize. Thank you for looking into this.

@kdombeck
Copy link
Contributor Author

@ofir-popowski is there anything that I can help with to get this published to Maven?

@ofir-popowski
Copy link
Contributor

@kdombeck heya, i think i finally managed to get this sorted
try changing the dependency to io.logz:guice-jersey:1.1.05 and see if it works out

@kdombeck
Copy link
Contributor Author

@kdombeck heya, i think i finally managed to get this sorted try changing the dependency to io.logz:guice-jersey:1.1.05 and see if it works out

Thank you!!

I did a quick test and it appears to be working. I did have a couple of questions.

  • Why is the patch version 05? Looking at Semantic Versioning it states that versions must not have leading zeros.
  • Shouldn't the version in the pom.xml be updated to match the released version?

@ofir-popowski
Copy link
Contributor

Oh thank you for pointing these out :)
It was my first time adding a Github Action and doing a release and all that so I'm still working out some kinks :p

@asafm
Copy link

asafm commented Sep 20, 2022

@ofir-popowski Take a look at the jmx2graphite repo: https://github.com/logzio/jmx2graphite
It has GitHub Actions such as when you create a release, it updates the pom.xml version matching the release name

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.

3 participants