-
Notifications
You must be signed in to change notification settings - Fork 30
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
Quarkus logging #2171
base: master
Are you sure you want to change the base?
Quarkus logging #2171
Conversation
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.
Hi @barreiro, planning to look at this either today or tomorrow, meanwhile can you please apply the formatting as it looks like there are some uncommitted changes
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 lgtm!
I've just one doubt, the quarkus doc clearly says that:
Only use the Log API in application classes, not in external dependencies. Log method calls that are not processed by Quarkus at build time will throw an exception.
But I see we are using horreum-infra-common
in the Jenkins plugin that is not using Quarkus IIUC 👉🏻 https://github.com/jenkinsci/horreum-plugin/blob/bb0d263b081da6967bba3ca6e98f4d3dc9c194cd/pom.xml#L248-L253, therefore I think there could be some problem there, wdyt?
good catch. |
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.
lgtm
I was thinking whether we want to backport this or not, if we won't.. it means that most of the next PRs that need to be backported will likely have some conflicts.. therefore I see two options:
wdyt @johnaohara @barreiro ? |
I'm fine either way. I guess it will depend on how long it will take until a |
yeah exactly! I would like to get #2202 merged and backported and then I think we can merge this one as well (IMO I would also backport it even if we won't release it in the 0.16 stream) |
Fixes Issue
Fixes #1993
Changes proposed
org.jboss.logging.Logger
toio.quarkus.logging.Log
debugf
,infof
and other "format" methods+
when creatingException
mesages"".formatted()
instead ofString.format()
Check List (Check all the applicable boxes)