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.

Sign up for GitHub

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

Jump to bottom

WIP: Jetty/Jersey upgrade #122

Closed
wants to merge 5 commits into from
Closed

WIP: Jetty/Jersey upgrade #122

wants to merge 5 commits into from

Conversation

fbruton
Copy link
Collaborator

@fbruton fbruton commented Mar 6, 2021

No description provided.

@drewfarris
Copy link
Collaborator

I don't know if it's helpful, but I also took a crack at the Java 11 upgrade in #83. Just wanted to point it out in case you have't seen it already.

@fbruton
Copy link
Collaborator Author

fbruton commented Mar 6, 2021

I don't know if it's helpful, but I also took a crack at the Java 11 upgrade in #83. Just wanted to point it out in case you have't seen it already.

Yes, I looked briefly at it and plan to incorporate a good portion of what's in your ticket as well. I pushed a WIP PR to get some assistance on a few failed tests from @dev-mlb. Thanks for the heads up!

@fbruton fbruton force-pushed the java-11 branch 2 times, most recently from 4624145 to ad07731 Compare April 15, 2021 15:45
@fbruton fbruton changed the title WIP: Java 11 upgrade Java 11 upgrade Apr 15, 2021
drewfarris
drewfarris reviewed Apr 16, 2021
View reviewed changes
src/test/java/emissary/config/ConfigUtilTest.java
@@ -529,6 +529,7 @@ public void testMultipleMasterClassNamesMultipleDirs() throws IOException, Emiss
}

@Test
@Ignore
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we really want to ignore this test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, until we address the LogbackCapture Java 11 compatibility issue.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fixed this separately in #201 - see https://github.com/NationalSecurityAgency/emissary/pull/201/files#diff-49ba8d133940994a3480f8831e57e54e2dc7d1f43bd3258d98cd1ff5722fccfa and file beneath for current method.

drewfarris
drewfarris reviewed Apr 16, 2021
View reviewed changes
src/main/java/emissary/server/EmissaryServer.java
@@ -595,14 +591,6 @@ private ContextHandler buildEmissaryHandler() throws EmissaryException {
return emissaryHolderContext;
}

private ContextHandler buildLogbackConfigHandler() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have an alternate mechanism for changing log levels in place? Is this just removal of unused code, or do we have a replacement for this or do we net lose functionality there?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This will require further investigation to find a Java 11 alternative for ViewStatusMessagesServlet

drewfarris
drewfarris previously approved these changes Apr 16, 2021
View reviewed changes
@jpdahlke jpdahlke added this to the v7.0.0 milestone Apr 19, 2021
@jpdahlke jpdahlke added dependencies This updates a dependency version integration A breaking change where effort will be required downstream to resolve mirror Additional tracking is necessary priority This has mirrored work driving the need labels Apr 19, 2021
@jpdahlke jpdahlke mentioned this pull request Apr 19, 2021
Closed
@fbruton fbruton changed the base branch from master to release-7.x April 19, 2021 17:10
jpdahlke
jpdahlke previously approved these changes Apr 19, 2021
View reviewed changes
Copy link
Collaborator

@jpdahlke jpdahlke left a comment

Choose a reason for hiding this comment

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

LGTM, you too @dev-mlb? Since we are just targeting the release-7.x branch we should probably just take this and start banging on it?

@fbruton
Copy link
Collaborator Author

fbruton commented Apr 20, 2021

Currently having issues with the integration tests:
mvn clean install will invoke the IT tests.

@jpdahlke jpdahlke modified the milestones: v7.0.0, v8.0.0 Apr 29, 2021
@jpdahlke jpdahlke closed this May 7, 2021
@jpdahlke jpdahlke deleted the branch NationalSecurityAgency:master May 7, 2021 15:18
@jpdahlke jpdahlke reopened this May 7, 2021
@fbruton fbruton changed the base branch from release-7.x to master July 25, 2021 01:41
@fbruton fbruton changed the title Java 11 upgrade Jetty/Jersey upgrade Jul 27, 2021
@fbruton fbruton changed the title Jetty/Jersey upgrade WIP: Jetty/Jersey upgrade Jul 27, 2021
@drivenflywheel drivenflywheel mentioned this pull request Sep 21, 2021
Closed
@jpdahlke
Copy link
Collaborator

To get past an issue when bouncing between maven-surefire-plugin 3.0.0-M3 and 3.0.0-M5, I had added this line to the pom:
<forkNode implementation="org.apache.maven.plugin.surefire.extensions.SurefireForkNodeFactory" />
IIRC, moving down to M3 was sufficient but commenting on this to make sure I don't forget.

@dev-mlb
Copy link
Collaborator

dev-mlb commented Oct 19, 2021 •
edited
Loading

Can we rebase this? It is marked as a priority, so we should keep it up to date. Should this be its own branch in Emissary to make it easier to collaborate on?

@jpdahlke jpdahlke removed the priority This has mirrored work driving the need label Dec 7, 2021
@cfkoehler cfkoehler added the major Change will go out in a major version increase label Dec 15, 2022
@jpdahlke jpdahlke dismissed stale reviews from drewfarris and themself via ff12bae March 28, 2023 13:50
@jpdahlke jpdahlke modified the milestones: v8.0.0, v8.0.0-M4, v8.0.0-M5 Aug 25, 2023
@drivenflywheel
Copy link
Collaborator

What's the current plan with this PR? Are we trying to rebase & save it, migrate selected parts to a replacement PR, or something else?

@jpdahlke jpdahlke removed this from the v8.0.0-M5 milestone Sep 14, 2023
@jpdahlke
Copy link
Collaborator

closing in favor of #552 and #550

@jpdahlke jpdahlke closed this Sep 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ukengineer925 ukengineer925 ukengineer925 left review comments

@jpdahlke jpdahlke jpdahlke left review comments

@drewfarris drewfarris drewfarris left review comments

@dev-mlb dev-mlb Awaiting requested review from dev-mlb

Assignees
No one assigned
Labels
dependencies This updates a dependency version integration A breaking change where effort will be required downstream to resolve major Change will go out in a major version increase mirror Additional tracking is necessary
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@fbruton @drewfarris @jpdahlke @dev-mlb @drivenflywheel @ukengineer925 @cfkoehler