[WFLY-12135] Error deploying web service without http-listener (only https-listener) #12788
Conversation
wildfly-ci
added
the
deps-ok
|
The org.wildfly.undertow capability is private: https://github.com/wildfly/wildfly-capabilities/blob/master/org/wildfly/undertow/capability.adoc That's because UndertowService is a really poor contract for use outside the subsystem; it exposes all sorts of stuff, including start/stop, that should not be visible external to the extension module. The WS subsystem already has code like this, and it has no choice because it uses what the undertow subsystem has historically exposed. So doing more of the same isn't a problem that should prevent merging a fix like this. But @fl4via it would be good to get a cleaner contract sometime during the next couple WF releases. |
I completely agree, but I couldn't find a different way to find an instance's configured listeners.
|
undertow/src/main/java/org/wildfly/extension/undertow/WebServerService.java
Show resolved
Hide resolved
...-integration/src/main/java/org/jboss/as/webservices/deployers/AspectDeploymentProcessor.java
Outdated
Show resolved
Hide resolved
...-integration/src/main/java/org/jboss/as/webservices/deployers/AspectDeploymentProcessor.java
Outdated
Show resolved
Hide resolved
...-integration/src/main/java/org/jboss/as/webservices/deployers/AspectDeploymentProcessor.java
Outdated
Show resolved
Hide resolved
|
@iweiss I see a forced push, does that address the comments Brian had? A problem with the GitHub UI is stale reviews are not cleared so an issue sits in the queue looking like no changes have been made. |
|
@iweiss This one has now been stalled since last year but we have a big backlog of PRs in the queue making it hard to see the ones that should be given attention - can we close this one for now and pick it up again once you are ready to address the discussion points so far? |
|
@darranl I think it's fair, but I would like to do a last push on this if you don't mind. I'll try to wrap this up in 1 week. |
| @@ -68,6 +74,8 @@ public void deploy(final DeploymentPhaseContext phaseContext) throws DeploymentU | |||
| WSLogger.ROOT_LOGGER.tracef("%s start: %s", aspect, unit.getName()); | |||
| ClassLoader origClassLoader = WildFlySecurityManager.getCurrentContextClassLoaderPrivileged(); | |||
| try { | |||
| dep.setProperty("isHttpsOnly", isHttpsOnly(phaseContext, unit)); | |||
|
|
|||
There was a problem hiding this comment.
@bstansberry @darranl As suggested by @jimma, I replaced the boolean attachment with a property. Looks cleaner and better.
|
jbossws/jbossws-common#19 has been merged |
|
retest please |
|
jbossws/jbossws-common#19 is now part of WildFly, #13552, so this PR is now ready to be merged. |
|
/retest |
|
@iweiss I have just kicked off some of the failed CI jobs again but TBH the failures do look related. |
|
FYI the "Linux - Elytron - JDK 8" job is an old job that can be ignored. |
|
@iweiss Is this PR still relevant? There haven't been any updates on it in a long time. |
|
@fjuma no idea. I would say no, since I haven't heard about it in a while, but I'm not really sure. |
|
Thanks @iweiss. Looking closer, the test failures do seem to be related to the changes. Might be best to close this for now. |
|
Agreed |
Issue: https://issues.jboss.org/browse/WFLY-12135
DON'T MERGE. This depends on jbossws/jbossws-common#19.