Issue #11761 fix jetty maven doco and integration test #11771
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
basil
suggested changes
May 9, 2024
There was a problem hiding this comment.
The demos need to be migrated to the new syntax as well:
jetty-ee10/jetty-ee10-demos/jetty-ee10-demo-jetty-webapp/pom.xml
jetty-ee10/jetty-ee10-demos/jetty-ee10-demo-spec/jetty-ee10-demo-spec-webapp/pom.xml
jetty-ee8/jetty-ee8-demos/jetty-ee8-demo-jetty-webapp/pom.xml
jetty-ee8/jetty-ee8-demos/jetty-ee8-demo-spec/jetty-ee8-demo-spec-webapp/pom.xml
jetty-ee9/jetty-ee9-demos/jetty-ee9-demo-jetty-webapp/pom.xml
jetty-ee9/jetty-ee9-demos/jetty-ee9-demo-spec/jetty-ee9-demo-spec-webapp/pom.xml
basil
approved these changes
May 9, 2024
basil
reviewed
May 9, 2024
| @@ -111,7 +111,9 @@ | |||
| <loginServices> | |||
| <loginService implementation="org.eclipse.jetty.security.HashLoginService"> | |||
| <name>Test Realm</name> | |||
| <config>src/test/resources/test-realm.properties</config> | |||
| <config implementation="org.eclipse.jetty.maven.MavenResource"> | |||
| <resourceAsString>src/test/resources/test-realm.properties</resourceAsString> | |||
There was a problem hiding this comment.
This is indented too much and fails spotless.
basil
suggested changes
May 9, 2024
There was a problem hiding this comment.
I installed this patch with mvn clean install -DskipTests. After that, I ran mvn -pl org.eclipse.jetty.ee9.demos:jetty-ee9-demo-jetty-webapp org.eclipse.jetty.ee9:jetty-ee9-maven-plugin:run to attempt to run the demo. The demo failed to run with a trivial Maven error. Fixing that obvious typo with
diff --git a/jetty-ee9/jetty-ee9-demos/jetty-ee9-demo-jetty-webapp/pom.xml b/jetty-ee9/jetty-ee9-demos/jetty-ee9-demo-jetty-webapp/pom.xml
index 8ba0fc74c4f..238a194ddbe 100644
--- a/jetty-ee9/jetty-ee9-demos/jetty-ee9-demo-jetty-webapp/pom.xml
+++ b/jetty-ee9/jetty-ee9-demos/jetty-ee9-demo-jetty-webapp/pom.xml
@@ -124,8 +124,8 @@
<version>${project.version}</version>
</dependency>
<dependency>
- <groupId>org.eclipse.jetty</groupId>
- <artifactId>jetty-servlets</artifactId>
+ <groupId>org.eclipse.jetty.ee9</groupId>
+ <artifactId>jetty-ee9-servlets</artifactId>
<version>${project.version}</version>
</dependency>
</dependencies>the demo failed to run with
[WARNING] Unable to get name for scope oeje9mp.MavenWebAppContext@3531509c{EE9 Demo Jetty WebApp,/test,file:///home/basil/src/eclipse/jetty.project/jetty-ee9/jetty-ee9-demos/jetty-ee9-demo-jetty-webapp/src/main/webapp/,false}{file:///home/basil/src/eclipse/jetty.project/jetty-ee9/jetty-ee9-demos/jetty-ee9-demo-jetty-webapp/src/main/webapp/}
javax.naming.NoInitialContextException: Need to specify class name in environment or system property, or in an application resource file: java.naming.factory.initial
at javax.naming.spi.NamingManager.getInitialContext (NamingManager.java:704)
at javax.naming.InitialContext.getDefaultInitCtx (InitialContext.java:305)
at javax.naming.InitialContext.getURLOrDefaultInitCtx (InitialContext.java:342)
at javax.naming.InitialContext.getNameParser (InitialContext.java:497)
at org.eclipse.jetty.plus.jndi.NamingEntryUtil.getNameForScope (NamingEntryUtil.java:184)
at org.eclipse.jetty.plus.jndi.NamingEntryUtil.destroyContextForScope (NamingEntryUtil.java:215)
at org.eclipse.jetty.ee9.plus.webapp.EnvConfiguration.destroy (EnvConfiguration.java:193)
at org.eclipse.jetty.ee9.webapp.WebAppContext.destroy (WebAppContext.java:581)
at org.eclipse.jetty.util.component.ContainerLifeCycle.destroy (ContainerLifeCycle.java:228)
at org.eclipse.jetty.server.Handler$Abstract.destroy (Handler.java:507)
at org.eclipse.jetty.server.handler.ContextHandler.lambda$destroy$2 (ContextHandler.java:806)
at org.eclipse.jetty.server.handler.ContextHandler$ScopedContext.run (ContextHandler.java:1298)
at org.eclipse.jetty.server.handler.ContextHandler$ScopedContext.run (ContextHandler.java:1285)
at org.eclipse.jetty.server.handler.ContextHandler.destroy (ContextHandler.java:806)
at org.eclipse.jetty.util.component.ContainerLifeCycle.destroy (ContainerLifeCycle.java:228)
at org.eclipse.jetty.server.Handler$Abstract.destroy (Handler.java:507)
at org.eclipse.jetty.util.component.ContainerLifeCycle.destroy (ContainerLifeCycle.java:228)
at org.eclipse.jetty.server.Handler$Abstract.destroy (Handler.java:507)
at org.eclipse.jetty.util.thread.ShutdownThread.run (ShutdownThread.java:142)
I am asking that all demos be tested and fixed if needed.
|
@basil I think we have some difficulty in executing the maven plugin on the demos - IIRC something to do with the build dependency ordering or something (@olamy can you remember why the plugin might not work on the demos?). I'm tempted to remove the plugin declarations from the demos, except I'm doing a major reorg of the demos in the jetty-12.1.0 branch, which might again make it possible to run the plugin on the demos. All of which is to say, that yes, I agree with you, but the work might be done in 12.1.x rather than 12.0.x. |
nope nothing to do with build order :). @basil is simply running only this module (not a complete build) It's just a long time since we have maintained the plugin declarations in all those demos, and we are testing them via the distribution tests now. |
|
If the demos can be fixed in a future 12.1.x release, I think that would be useful for people who are trying to figure out how to use this plugin. I tested these instructions, and they resolved my immediate issue, but I still cannot get the plugin to work in Jetty 12.0.9. After following the instructions in this PR, I now get: To reproduce, you can check out the |
|
@basil your login service definition for the jetty maven plugin needs to have a realm name, and that name needs to match a <loginService implementation="org.eclipse.jetty.security.HashLoginService">
<name>Test Realm</name>
<config implementation="org.eclipse.jetty.maven.MavenResource">
<resourceAsString>${basedir}/src/config/realm.properties</resourceAsString>
</config>
</loginService>and your web.xml needs to look like: <login-config>
<auth-method>FORM</auth-method>
<realm-name>Test Realm</realm-name>
<form-login-config>
<form-login-page>/logon.html?param=test</form-login-page>
<form-error-page>/logonError.html?param=test</form-error-page>
</form-login-config>
</login-config>I'll recheck the doco. |
basil
approved these changes
May 9, 2024
There was a problem hiding this comment.
Thank you very much! Adding back the <name> fixed it for me. I was able to leave it as default and I didn't have to edit web.xml to add a realm name there. The final set of changes that worked for me:
diff --git a/war/pom.xml b/war/pom.xml
index 3db5f853d2..47b59746f1 100644
--- a/war/pom.xml
+++ b/war/pom.xml
@@ -632,9 +638,9 @@ THE SOFTWARE.
</configuration>
</plugin>
<plugin>
- <groupId>org.eclipse.jetty</groupId>
- <artifactId>jetty-maven-plugin</artifactId>
- <version>10.0.20</version>
+ <groupId>org.eclipse.jetty.ee8</groupId>
+ <artifactId>jetty-ee8-maven-plugin</artifactId>
+ <version>12.0.9</version>
<configuration>
<!--
Reload webapp when you hit ENTER. (See JETTY-282 for more)
@@ -647,7 +653,9 @@ THE SOFTWARE.
<loginServices>
<loginService implementation="org.eclipse.jetty.security.HashLoginService">
<name>default</name>
- <config>${basedir}/src/realm.properties</config>
+ <config implementation="org.eclipse.jetty.maven.MavenResource">
+ <resourceAsString>${basedir}/src/realm.properties</resourceAsString>
+ </config>
</loginService>
</loginServices>
<systemProperties>
basil
approved these changes
May 9, 2024
olamy
approved these changes
May 9, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #11761
Also fixed name of integration test file.