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
add camel-jaxb into extensions #850
Conversation
#849 came first and it might have an impact on the current one. |
retest this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mmelko, good work, I especially appreciate the test coverage! Some suggestions inline.
extensions/jaxb/runtime/pom.xml
Outdated
<name>Camel Quarkus :: JAXB extension :: Runtime</name> | ||
|
||
<properties> | ||
<firstVersion>1.2.0</firstVersion> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<firstVersion>1.2.0</firstVersion> | |
<firstVersion>1.0.0-M5</firstVersion> |
an re-run mvn process-resources -Pformat
Sorry, this used to be incorrectly generated by the template and it should be fixed now for newly stubbed extensions.
# | ||
|
||
--- | ||
name: "Camel Quarkus JAXB t" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name: "Camel Quarkus JAXB t" | |
name: "Camel Quarkus JAXB Dataformat" |
# | ||
# Quarkus | ||
# | ||
quarkus.ssl.native=true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need this?
quarkus.native.additional-build-args = -H:IncludeResources=test/.* | ||
|
||
camel.main.xml-routes = classpath:test/jaxb-routes.xml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we sufficiently test loading the routes from XML elsewhere. I'd vote testing only via Java DSL here.
integration-tests/jaxb/pom.xml
Outdated
</dependency> | ||
<dependency> | ||
<groupId>org.apache.camel.quarkus</groupId> | ||
<artifactId>camel-quarkus-core-xml</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is camel-quarkus-core-xml required for anything else except loading the XML routes (that I propose to get rid of below)? It would be nice to make sure that camel-quarkus-core-xml is not a hidden dependency of camel-quarkus-jaxb. So I vote for removing camel-quarkus-core-xml if possible.
integration-tests/jaxb/pom.xml
Outdated
</dependency> | ||
<dependency> | ||
<groupId>org.apache.camel.quarkus</groupId> | ||
<artifactId>camel-quarkus-bean</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the bean component really used? I have not found it anywhere in the routes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was required for some reason when I was using <setBody><simple>${body....}
when I will remove xml routes I can delete it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes it is required for the simple language when using OGNL like expressions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
camel-quarkus-bean is still there. Can we remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved route from xml to java dsl so I still need it for simple language in setbody.
90235fd
to
46f53e5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few nitpicks, otherwise looks good.
extensions/jaxb/runtime/pom.xml
Outdated
<dependency> | ||
<groupId>org.apache.camel</groupId> | ||
<artifactId>camel-jaxb</artifactId> | ||
</dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indent
integration-tests/jaxb/pom.xml
Outdated
|
||
<artifactId>camel-quarkus-integration-test-jaxb</artifactId> | ||
<name>Camel Quarkus :: Integration Tests :: JAXB extension</name> | ||
<description>Integration tests for Camel Quarkus JAXB extension extension</description> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<description>Integration tests for Camel Quarkus JAXB extension extension</description> | |
<description>Integration tests for Camel Quarkus JAXB extension</description> |
integration-tests/jaxb/pom.xml
Outdated
</dependency> | ||
<dependency> | ||
<groupId>org.apache.camel.quarkus</groupId> | ||
<artifactId>camel-quarkus-bean</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
camel-quarkus-bean is still there. Can we remove it?
46f53e5
to
a30777e
Compare
a30777e
to
b0d72b0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @mmelko !
fix #786