quarkiverse / quarkus-github-app

Develop your GitHub Apps in Java with Quarkus.
https://docs.quarkiverse.io/quarkus-github-app/dev/index.html
Apache License 2.0
60 stars 27 forks source link

Fix #627: Dev UI Card #633

Closed melloware closed 6 days ago

melloware commented 1 week ago

Fix #627: Dev UI Card

image

rsvoboda commented 1 week ago

curl http://localhost:8080/replay/ gives 404 - Resource Not Found

I tried with main of quarkus-github-app and replay ui is available

So this must be side effect of this PR

http://localhost:8080/replay/events is available

(running quarkus-github-bot with quarkus dev -Dquarkus-github-app.version=999-SNAPSHOT command)

melloware commented 1 week ago

@rsvoboda you can see this PR doesn't add anything so how can replay be broken by it I get a 404 when I hit it as well.

melloware commented 1 week ago

I think i have an idea...

melloware commented 1 week ago

@rsvoboda can you try again i added the missing Vert.X DEV UI SPI so I am hoping that is what caused it?

melloware commented 1 week ago

when i do a fresh checkout of main i get these errors on mvn clean install

[ERROR] io.quarkiverse.githubapp.deployment.ValidRawEventTest -- Time elapsed: 0.415 s <<< ERROR!
java.lang.RuntimeException: java.lang.RuntimeException: Failed to load steps from class io.quarkiverse.githubapp.deployment.GitHubAppProcessor
        at io.quarkus.test.QuarkusUnitTest.beforeAll(QuarkusUnitTest.java:708)
        at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
Caused by: java.lang.RuntimeException: Failed to load steps from class io.quarkiverse.githubapp.deployment.GitHubAppProcessor
        at io.quarkus.deployment.ExtensionLoader.loadStepsFrom(ExtensionLoader.java:164)
        at io.quarkus.deployment.QuarkusAugmentor.run(QuarkusAugmentor.java:107)
        at io.quarkus.runner.bootstrap.AugmentActionImpl.runAugment(AugmentActionImpl.java:327)
        at io.quarkus.runner.bootstrap.AugmentActionImpl.createInitialRuntimeApplication(AugmentActionImpl.java:252)
        at io.quarkus.test.QuarkusUnitTest.beforeAll(QuarkusUnitTest.java:654)
        ... 1 more
Caused by: java.lang.NoClassDefFoundError: DispatchingConfiguration
        at java.base/java.lang.Class.getDeclaredMethods0(Native Method)
        at java.base/java.lang.Class.privateGetDeclaredMethods(Class.java:3402)
        at java.base/java.lang.Class.getDeclaredMethods(Class.java:2504)
        at io.quarkus.deployment.ExtensionLoader.getMethods(ExtensionLoader.java:922)
        at io.quarkus.deployment.ExtensionLoader.loadStepsFromClass(ExtensionLoader.java:432)
        at io.quarkus.deployment.ExtensionLoader.loadStepsFrom(ExtensionLoader.java:162)
        ... 5 more
Caused by: java.lang.ClassNotFoundException: DispatchingConfiguration
        at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:641)
        at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188)
        at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:525)
        ... 11 more
gsmet commented 1 week ago

I don't reproduce it on my side and CI looks fine. What's weird is that it looks as if it was looking for a class in the default package.

Maybe skip the tests?

rsvoboda commented 1 week ago

http://localhost:8080/replay/ still not accessible

rsvoboda commented 1 week ago

I have no problems running ValidRawEventTest

Apache Maven 3.9.8 (36645f6c9b5079805ea5009217e36f2cffd34256)
Maven home: /Users/rsvoboda/.sdkman/candidates/maven/current
Java version: 21.0.2, vendor: Eclipse Adoptium, runtime: /Users/rsvoboda/.sdkman/candidates/java/21.0.2-tem
Default locale: en_US, platform encoding: UTF-8
OS name: "mac os x", version: "14.5", arch: "aarch64", family: "mac"
melloware commented 1 week ago

Windows

C:\dev\quarkus\quarkus-github-app>mvn --version
Apache Maven 3.9.7 (8b094c9513efc1b9ce2d952b3b9c8eaedaf8cbf0)
Maven home: C:\tools\apache-maven-3.9.7
Java version: 17.0.11, vendor: Eclipse Adoptium, runtime: C:\tools\jdk-17.0.6+10
Default locale: en_US, platform encoding: Cp1252
OS name: "windows 11", version: "10.0", arch: "amd64", family: "windows"

Linux

Apache Maven 3.9.1 (2e178502fcdbffc201671fb2537d0cb4b4cc58f8)
Maven home: /opt/maven
Java version: 17.0.9, vendor: Private Build, runtime: /usr/lib/jvm/java-17-openjdk-amd64
Default locale: en, platform encoding: UTF-8
OS name: "linux", version: "5.15.153.1-microsoft-standard-wsl2", arch: "amd64", family: "unix"

i have it failing on windows and linux

melloware commented 1 week ago

i noticed your are using JDK21 is that required i thought Quarkus was still on 17?

gsmet commented 1 week ago

I reproduce the issue with Java 17.

The route is here but somehow it's not accessible anymore.

rsvoboda commented 1 week ago

Having just cardPageBuildItemBuildProducer.produce(new CardPageBuildItem()); in GitHubAppDevUIProcessor method causes Replay UI to be unavailable.

I noticed small difference in the routes order in http://localhost:8080/q/dev-ui/endpoints-routes There is changed order for /replay/events and /

Unavailable Replay UI

unavailable-reply-ui

Available Replay UI

available-reply-ui
melloware commented 1 week ago

we might need @phillip-kruger to help us explain as that is the core Dev UI stuff.

melloware commented 1 week ago

@gsmet should we open a different ticket for the JDK 17 issue?

melloware commented 1 week ago

Just a note I have tried using order and OrderedRoute and no matter what I change the order to adding the Dev UI card breaks the routes. I think we will need the Dev UI guys help on this one as it looks like its somewhere in the Vert.X stuff.

phillip-kruger commented 1 week ago

@melloware do you have an example app / reproducer I can look at ?

melloware commented 1 week ago

@phillip-kruger yes attached: my-github-app.zip

phillip-kruger commented 1 week ago

Thanks. I'll look in the morning

gsmet commented 1 week ago

@phillip-kruger also what I'm not sure of is why we have a route on / created by DevUIRecorder. I would have expected it to be mounted on /q? Or is it for the new default index page?

phillip-kruger commented 1 week ago

Yes that would be the index page. I'll have a look at this a.s.a.p

phillip-kruger commented 6 days ago

I can recreate the issue, but I don't know why this happens.

I have some questions though:

It should be easy enough to just move this to Dev UI, can we do that ?

phillip-kruger commented 6 days ago

OK, I now know what the issue is. Both the UI for replay-ui and the dev-ui use WebJarResults with the same key (the artifactId of the deployment module). That breaks things. So (apart from the suggestions above, that I think is the correct way to do this) we can do the following:

Let me know what you think. The quickest would be the sidestep.

phillip-kruger commented 6 days ago

I have opened this PR (against @melloware 's devui-card branch) to also move the UI to it's own module. This sidesteps the issue. https://github.com/melloware/quarkus-github-app/pull/2

gsmet commented 6 days ago

@phillip-kruger OK I see. Yes, this could potentially be moved to the Dev UI at some point but for me what I do here is nothing extraordinary and I would expect it to work out of the box. So we can work around it for now but it seems like a surprising behavior to me.

phillip-kruger commented 6 days ago

Yea agree that we should do the fix in webjarBuildItem, I can look at that at some point. Maybe we should create an issue on Quarkus ?

melloware commented 6 days ago

@phillip-kruger i feel like the WebJarBuildItem should be fixed its only a matter of time before someone else gets stung by it?

phillip-kruger commented 6 days ago

Agree.

melloware commented 6 days ago

I have merged @phillip-kruger PR and I like this change as it separates the replay UI into its own jar.

gsmet commented 6 days ago

@phillip-kruger I let you create the issue? You have all the specifics and I will probably get it wrong :).

@rsvoboda you confirm you tested the latest version and everything works fine?

rsvoboda commented 6 days ago

@rsvoboda you confirm you tested the latest version and everything works fine?`

yes

gsmet commented 6 days ago

@melloware and sorry for the ambush, I thought it would be an easy win :).

melloware commented 6 days ago

HA no this was fun and worth doing because we found some bugs!

phillip-kruger commented 6 days ago

@gsmet If you can provide instructions on how to get events in the screen so that I can see how this works currently, I can migrate this to Dev UI.