-
Notifications
You must be signed in to change notification settings - Fork 13
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
Feature/java client apache connector #2302
base: dev
Are you sure you want to change the base?
Conversation
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.
I dug around some more and can't find any evidence of why we didn't go with this approach back in 2023. I guess we'll just have to rediscover it if there was some real problem. :-/
Anyway, the substance looks good here, just a couple of minor tweaks.
clientlibs/java/pom.xml
Outdated
<maven.compiler.source>17</maven.compiler.source> | ||
<maven.compiler.target>17</maven.compiler.target> |
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.
They unified source
and target
into a new release
parameter:
<maven.compiler.source>17</maven.compiler.source> | |
<maven.compiler.target>17</maven.compiler.target> | |
<maven.compiler.release>17</maven.compiler.release> |
clientlibs/java/pom.xml
Outdated
@@ -9,7 +9,7 @@ | |||
at the same time that happen to rev to the same new version will be caught | |||
by a merge conflict. --> | |||
|
|||
<version>6.1.0</version> | |||
<version>6.1.1</version> |
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.
Even though this library has seen very few updates recently, I still think the comment just above is reasonable. So:
<version>6.1.1</version> | |
<version>6.1.1</version> <!-- 6.1.1: switch to jersey-apache-connector --> |
Also there seem to be several failing NPM tests, probably we'll need another PR to land to clean those up before this one lands? |
That build failure is to be ignored, I don't know why it can't be suppressed/ignored on dev, I've asked. But long story short is that task fails on dev because it is only for beanstalk builds, we have no dev beanstalk env anymore, and we haven't felt like fixing errors for a pointless task, but it lingers around because it still needs to exist for staging and prod. At the time it seemed like the switchover to ECS was going to happen more imminently, but it hasn't, so we've just sort of ignored it. the equivalent build task for ECS (upgrade-service) passes. I am going to assume this was some dangling task that Jenn had been working on when she left and got lost in the void. |
Smoke test of this worked for me testing with the
QuickTest.java
file (formerly Main.java). I installed JDK 17 on my local, and then when I target 17 here in properties for the jersey client instead of 8 I was able to replicate the PATCH error. After installing the updated dependency for apache connector as it was in the PR from years back, the error went away.Let me know what else we should consider to test, and if there are other deps we should go ahead and update alongside of this let's go ahead.