-
Notifications
You must be signed in to change notification settings - Fork 5
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
Wildfly mini serie client service #4
base: main
Are you sure you want to change the base?
Wildfly mini serie client service #4
Conversation
@bstansberry can you assign a reviewer? |
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 comments as it looks like they were copy/pasted to each example. Once we agree and the changes have been propagated to the other examples, I'll have a look again.
<repository> | ||
<id>central</id> | ||
<name>Main Apache Maven Repository</name> | ||
<url>https://repo.maven.apache.org/maven2/</url> | ||
<layout>default</layout> | ||
<releases> | ||
<enabled>true</enabled> | ||
<updatePolicy>never</updatePolicy> | ||
</releases> | ||
<snapshots> | ||
<enabled>false</enabled> | ||
<updatePolicy>never</updatePolicy> | ||
</snapshots> | ||
</repository> |
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.
There is no need to add this one.
<pluginRepository> | ||
<id>central</id> | ||
<name>Main Apache Maven Repository</name> | ||
<url>https://repo.maven.apache.org/maven2/</url> | ||
<releases> | ||
<enabled>true</enabled> | ||
<updatePolicy>never</updatePolicy> | ||
</releases> | ||
<snapshots> | ||
<enabled>false</enabled> | ||
<updatePolicy>never</updatePolicy> | ||
</snapshots> | ||
</pluginRepository> |
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.
There is no need to add this one.
|
||
<packaging>war</packaging> | ||
<name>simple-microservice-client-secured</name> | ||
<description>Insert description for your project here.</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.
Seems like we should just delete the description.
|
||
<!-- JBoss dependency versions --> | ||
<version.wildfly.maven.plugin>5.0.0.Final</version.wildfly.maven.plugin> | ||
<version.wildfly.bom>33.0.2.Final</version.wildfly.bom> |
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.
We should use 34.0.0.Final
<version.wildfly.bom>33.0.2.Final</version.wildfly.bom> | ||
|
||
<!--Use JUnit 5 here - the WildFly bom still brings 4.x --> | ||
<version.junit5>5.10.1</version.junit5> |
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.
5.11.3 is the latest.
|
||
<dependency> | ||
<groupId>org.wildfly.bom</groupId> | ||
<artifactId>wildfly-microprofile</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.
This should be wildfly-expansion for version 34.0.0.Final+
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.
Thank you @jamezp ... I missed that
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 is missing the copyright.
@RegisterClientHeaders | ||
@RegisterRestClient(configKey="simple-microservice-server") | ||
@Path("/hello") | ||
public interface GettingStartedEndpointInterface { |
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.
IMO we should just name this GettingStartedEndpointClient
.
@jamezp Thank you for your review! |
@jamezp Thank you again for your review, I hope I have fixed everything you pointed out |
1999754
to
d9fb804
Compare
@jamezp Thank you again fro your review, do you think this is ready to be merged now? |
@tommaso-borgato Sorry, I didn't get a chance to review it before PTO. I might not have a chance until I get back on the 2nd of January. |
@jamezp Do you think this is ready to be merged now (together with wildfly/wildfly.org#671)? |
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.
Overall this looks okay. I've made some comments primarily around component upgrades. This is something we're going to need to figure out how to manage as it could get rather daunting to check for upgrades on each release. This is not something that needs to be resolved in this PR.
One question I have is why are we using Dockerfile
's instead of having the wildfly-mavne-plugin
create and deploy the images? It feels like an extra step that could simply be part of the build and ensure the correct Dockerfile
content is created not having to worry about updating them in the future if something changes.
In general with these guide examples, we need to be able to easily keep them up to date without a lot of overhead.
<version.wildfly.maven.plugin>5.0.1.Final</version.wildfly.maven.plugin> | ||
<version.wildfly.bom>34.0.1.Final</version.wildfly.bom> |
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.
These should probably be upgraded.
<version.wildfly.maven.plugin>5.0.1.Final</version.wildfly.maven.plugin> | ||
<version.wildfly.bom>34.0.1.Final</version.wildfly.bom> |
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.
Same note here about the upgrades.
<version.wildfly.maven.plugin>5.0.1.Final</version.wildfly.maven.plugin> | ||
<version.wildfly.bom>34.0.1.Final</version.wildfly.bom> |
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.
Again :)
<version.war.plugin>3.4.0</version.war.plugin> | ||
|
||
<!-- maven-compiler-plugin --> | ||
<maven.compiler.release>11</maven.compiler.release> |
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.
Should this be Java 17 now?
<version.wildfly.maven.plugin>5.0.1.Final</version.wildfly.maven.plugin> | ||
<version.wildfly.bom>34.0.1.Final</version.wildfly.bom> |
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.
These should probably be upgraded.
OidcSecurityContext oidcSecurityContext = getOidcSecurityContext(httpServletRequest); | ||
if (oidcSecurityContext != null) { | ||
String authzHeaderValue = "Bearer " + oidcSecurityContext.getTokenString(); | ||
System.out.println("\n\n[JWT] service Token: " + authzHeaderValue + "\n\n"); |
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.
We should use logging here instead of System.out
.
@Produces(MediaType.TEXT_PLAIN) | ||
@RolesAllowed({"admin"}) | ||
public Response sayHello(final @PathParam("name") String name) { | ||
System.out.println("mp.jwt.verify.publickey.location=" + publicKeyLocation); |
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.
We should use logging here instead of System.out
.
<version.wildfly.maven.plugin>5.0.1.Final</version.wildfly.maven.plugin> | ||
<version.wildfly.bom>34.0.1.Final</version.wildfly.bom> |
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.
Same notes about upgrades.
<dependency> | ||
<groupId>org.junit</groupId> | ||
<artifactId>junit-bom</artifactId> | ||
<version>5.10.1</version> |
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.
Why aren't we using the property version here?
<dependency> | ||
<groupId>org.junit</groupId> | ||
<artifactId>junit-bom</artifactId> | ||
<version>5.10.1</version> |
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.
Why aren't we using the property here?
d9fb804
to
78b5166
Compare
@jamezp Thank you for your review!!! I used |
@jamezp Thank you for your review, do you think we can merge this one and wildfly/wildfly.org#671? |
Source code for guides in wildfly/wildfly.org#671