search

cdevents / sdk-java

Java SDK for CDEvents
Apache License 2.0
5 stars 6 forks source link

Migrate away from Reload4j #39

Closed aalmiray closed 1 year ago

aalmiray commented 1 year ago

Reload4j is the continuation of Log4j 1 for those affected by log4shell.

It would be better to use a more recent version of an Slf4j binding such as logback, log4j2, or slf4j-simple. Matter of fact, defining an slf4j binding in a library is a bit of an anti-pattern. Consumers such applications do require explicit bindings, libraries do not.

However, a binding may be needed for testing. Thus changing the scope of the logging binding to test would be better.

IF the logging binding is indeed intended for just testing I'd suggest going with slf4j-simple as it's the easiest (no more deps) to use.

elmsdata commented 1 year ago

Reading through your amazing modular dependency tracker, we would essentially need to replace the log4j.properties file with a simplelogger.properties with the configuration for the project. Then also updating the pom dependencies.

Found this issue very interesting and just wanted to learn about the requirements behind it!

aalmiray commented 1 year ago

The thing is, that log4j.properties file should not even be at src/main/resources as the consumer may use a completely different slf4j binding, rendering that file useless.

However, it does make sense providing a logging binding for tests, which is why I suggest choosing either logback or slf4j-simple, but slf4j-log4j may remain as the chosen binding. Besides updating the <scope> of the slf4j-log4j property to test we'd also need to move log4j.properties from src/main/resources to src/test/resources.

afrittoli commented 1 year ago

@aalmiray @elmsdata Thanks for this! We would like to release v0.1.2 as soon as possible, the work on Spinnaker depends on that. Is this something we can address in the v0.2 milestone, or does it need to be included in the v0.1.2? If the latter, do you have an ETA for a fix?

/cc @rjalander @adamkenihan

aalmiray commented 1 year ago

@afrittoli I'm waiting for consensus/guidance before sending a PR. As I see it given my experience with Slf4j, cdevents-java should only define slf4j-api as part of its compile scope. slf4j-simple may be used for tests.

I can send a PR pretty soon if there's agreement on how to handle logging dependencies.

rjalander commented 1 year ago

@aalmiray yes the existing logging is just used to print in the console when testing, I think you can create a PR as per your suggestion.

cdevents-bot commented 1 year ago

🎉 This issue has been resolved in v0.1.2 (Release Notes)