-
Notifications
You must be signed in to change notification settings - Fork 3k
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
fix(logging): Remove lombok as source of slf4j-api #6616
Conversation
e7165f9
to
61b99e6
Compare
9d102c9
to
2ef5aa3
Compare
Looks like tests are failing, probably need a testCompile dep on logback |
…eOnly where possible
2ef5aa3
to
5f385f1
Compare
} | ||
|
||
task run(type: JavaExec, dependsOn: build) { | ||
main = "org.eclipse.jetty.runner.Runner" | ||
args = [war.archivePath] | ||
classpath configurations.jetty8 | ||
classpath configurations.jetty9 |
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
@@ -15,7 +15,7 @@ dependencies { | |||
compile externalDependency.gson | |||
compile externalDependency.kafkaClients | |||
compile externalDependency.kafkaAvroSerde | |||
compile externalDependency.lombok | |||
compileOnly externalDependency.lombok |
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!
'log4jApi': 'org.apache.logging.log4j:log4j-api:2.19.0', | ||
'logbackClassic': "ch.qos.logback:logback-classic:$logbackClassic", | ||
'slf4jApi': "org.slf4j:slf4j-api:$slf4jVersion", | ||
'log4jCore': "org.apache.logging.log4j:log4j-core:$log4jVersion", |
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 ecosystem is still a bit convoluted to me, but why do we need all of these? Shouldn't we ideally just use logback and SLF4J?
Also, what is the purpose of log4j12Api
?
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.
Unfortunately some dependencies include the need for the older log4j api and the actual log4j impl still. It might be possible to update and/or replace those overtime however updating to latest works for now.
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. Thank you, David!
|
…eOnly where possible (datahub-project#6616)
Remove lombok as source of slf4j-api, convert to compileOnly where possible
Checklist