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.

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

Use Quarkus fast-jar package format apache/camel-k-runtime#360 #1931

Merged
merged 1 commit into from
Mar 2, 2021

Conversation

jamesnetherton
Copy link
Contributor

Depends on an as-yet uncommitted change to camel-k-runtime (hence draft PR), but this PR provides the means to support the Quarkus fast-jar format.

At the moment fast-jar expects a specific directory structure, so I've had to replicate that in the container under /deployment/dependencies .

Also I assumed it's safe to not use the generate-dependency-list mojo since the quarkus-maven-plugin already does its own dependency resolution and downloading of artifacts.

Release Note

NONE

@astefanutti
Copy link
Member

Very nice! It may impact #1816.

@doru1004
Copy link
Contributor

Will this work with local dependency resolution? It would be good to at least try it out with the set of local commands which use the current way of resolving dependencies. For example running kamel inspect and kamel inspect --all-dependencies.

@jamesnetherton
Copy link
Contributor Author

Will this work with local dependency resolution? It would be good to at least try it out with the set of local commands which use the current way of resolving dependencies. For example running kamel inspect and kamel inspect --all-dependencies.

I did test that and it looked to be working ok. Also ran the 'local' integration test suite and it passed.

@jamesnetherton
Copy link
Contributor Author

I did test that and it looked to be working ok

Actually now I think about it a bit more, maybe it isn't....

The paths listed are to the temp maven build directory, which I guess is not correct (since it is 'temporary')? I suppose the paths are meant to resolve to the artifacts in the local m2 repository?

@doru1004
Copy link
Contributor

If that's the case then we now have even more need of a means of saving the dependencies after the command is completed and the temporary folder is removed.

@doru1004
Copy link
Contributor

doru1004 commented Jan 21, 2021

So I think that today we have the same issue with the jar containing the quarkus-generated main function. If I understand correctly this will now apply to the rest of the dependencies too.

@jamesnetherton
Copy link
Contributor Author

Here's some example output from kamel inspect examples/Sample.java --all-dependencies.

/tmp/maven-485089909/target/quarkus-app/app/camel-k-integration-1.4.0-SNAPSHOT.jar
/tmp/maven-485089909/target/quarkus-app/lib/boot/io.quarkus.quarkus-bootstrap-runner-1.11.0.Final.jar
/tmp/maven-485089909/target/quarkus-app/lib/boot/io.quarkus.quarkus-development-mode-spi-1.11.0.Final.jar
/tmp/maven-485089909/target/quarkus-app/lib/boot/io.smallrye.common.smallrye-common-io-1.5.0.jar
/tmp/maven-485089909/target/quarkus-app/lib/boot/org.jboss.logging.jboss-logging-3.4.1.Final.jar
...

@doru1004
Copy link
Contributor

I see, I mean that would be ok for the narrow purposes of the inspect command but unless they can be saved in another folder then those dependencies will not be there after the command completes.

The question is, when trying to containerize the application. If you run:

kamel local create --image <name> examples/Sample.java
kamel local run --image <name>

Does the image run successfully?

@jamesnetherton
Copy link
Contributor Author

Does the image run successfully?

Yes, that part works. The integration tests verify that and I did some of my own testing on it.

@doru1004
Copy link
Contributor

Awesome!

@jamesnetherton
Copy link
Contributor Author

Seems a Spectrum tweak is also required, so opened container-tools/spectrum#4.

@jamesnetherton jamesnetherton force-pushed the fast-jar branch 5 times, most recently from 1259c4a to 512ef2e Compare March 1, 2021 09:06
@jamesnetherton jamesnetherton marked this pull request as ready for review March 1, 2021 09:06
@jamesnetherton
Copy link
Contributor Author

I don't understand why there are random failures in the YAKS knative tests. They don't occur for me locally.

I've put this PR back into review and it's good to merge IMO.

@astefanutti
Copy link
Member

Thanks for the update. Yes, there are some flaky YAKS tests, that are not caused by your changes.

@astefanutti
Copy link
Member

@jamesnetherton could you please resolve the conflicts?

@jamesnetherton
Copy link
Contributor Author

Conflicts resolved.

@jamesnetherton
Copy link
Contributor Author

@astefanutti are we good to merge this one? The usual flaky tests failed.

@astefanutti
Copy link
Member

@jamesnetherton let's retry one last time, and have it merged.

@astefanutti
Copy link
Member

@jamesnetherton one last point, as the metrics endpoint exposed by Quarkus has its path changed, I think the ServiceMonitor resource created by the prometheus trait must have the path explicitly specified. The e2e monitoring tests only run on OCP 4 downstream, which explain why CI didn't surface the problem. I'm fine handling it if you prefer right after it's merged. Let me know.

/cc @llowinge who wrote the monitoring e2e tests.

@jamesnetherton
Copy link
Contributor Author

Added the additional config to the ServiceMonitor (hopefully correctly).

@astefanutti
Copy link
Member

Added the additional config to the ServiceMonitor (hopefully correctly).

Perfect, LGTM 👍🏼.

@jamesnetherton
Copy link
Contributor Author

Woah! A successful build 😅

@jamesnetherton
Copy link
Contributor Author

I'll merge as I'd like to not have to rebase this one again...

@jamesnetherton jamesnetherton merged commit 0fd5181 into apache:master Mar 2, 2021
@jamesnetherton jamesnetherton deleted the fast-jar branch March 2, 2021 16:24
@nicolaferraro nicolaferraro mentioned this pull request Apr 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants