Skip to content
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

remove unused logback dependency #88

Merged
merged 2 commits into from
Aug 26, 2023
Merged

remove unused logback dependency #88

merged 2 commits into from
Aug 26, 2023

Conversation

elharo
Copy link
Contributor

@elharo elharo commented Jul 13, 2023

No description provided.

@elharo elharo marked this pull request as ready for review July 13, 2023 13:45
@elharo elharo requested a review from gnodet July 13, 2023 13:45
slachiewicz
slachiewicz previously approved these changes Aug 2, 2023
@cstamas
Copy link
Member

cstamas commented Aug 2, 2023

Why is there warning in tests about lack of slf4j backend? Which slf4j backend is used now that logback is removed?

@olamy
Copy link
Member

olamy commented Aug 2, 2023

@gnodet
Copy link
Contributor

gnodet commented Aug 3, 2023

we need at least one slf4j impl: https://github.com/apache/maven-build-cache-extension/actions/runs/5543308023/job/15013606896?pr=88#step:6:2248

Agreed, @elharo what's the purpose of this PR ? the dependency is actually used during tests

@slachiewicz slachiewicz self-requested a review August 15, 2023 14:00
@slachiewicz slachiewicz dismissed their stale review August 15, 2023 14:01

looks like we need it

@elharo
Copy link
Contributor Author

elharo commented Aug 26, 2023

  1. It cures an unused dependency warning.
  2. More importantly it removes yet another idiosyncratic dependency not used anywhere else in maven tossed into the classpath, likely without much thought, some years ago.

https://jlbp.dev/JLBP-1

Logging in Java is a vastly over-engineered mess, and we have only ourselves to blame for it. :-(

@elharo
Copy link
Contributor Author

elharo commented Aug 26, 2023

The warning is an annoying bug in slf4j. We don't need a logging backend in tests. For that matter, we don't need logging in tests. Logging in tests is a bug. Only failing tests should generate output, and that output should come from assertion failures. Passing tests should generate no output. Otherwise the actual issues get lost in a sea of irrelevant log junk.

@elharo elharo merged commit 47f8404 into master Aug 26, 2023
27 checks passed
@elharo elharo deleted the logback branch August 26, 2023 13:31
@olamy
Copy link
Member

olamy commented Aug 26, 2023

@elharo -1
Please revert your commit as it's used dependency.
It's used only in test and very helpful when you need to debug failing tests locally or even in CI

@michael-o
Copy link
Member

Slf4j simple is enough here...

@olamy
Copy link
Member

olamy commented Aug 26, 2023

Slf4j simple is enough here...

Agree but we need one. I had to debug some issues here in the past with docker on GHA and it was really helpful

@michael-o
Copy link
Member

Slf4j simple is enough here...

Agree but we need one. I had to debug some issues here in the past with docker on GHA and it was really helpful

That's the purpose of logging...post-mortem assistance 😁

@HannesWell
Copy link

HannesWell commented Aug 28, 2023

Slf4j simple is enough here...

Agree but we need one. I had to debug some issues here in the past with docker on GHA and it was really helpful

You can configure SLF4J-simple via system properties:
https://www.slf4j.org/api/org/slf4j/simple/SimpleLogger.html
If you want, you can turn it off by default and activate it if needed through corresponding configuration or CLI arguments.

@olamy
Copy link
Member

olamy commented Aug 28, 2023

Slf4j simple is enough here...

Agree but we need one. I had to debug some issues here in the past with docker on GHA and it was really helpful

You can configure SLF4J-simple via system properties: https://www.slf4j.org/api/org/slf4j/simple/SimpleLogger.html If you want, you can turn it off by default an activate it if needed through corresponding configuration or CLI arguments.

@HannesWell we perfectly know this but here this PR removes every possible logging solution. As it has been said slf4j-simple is perfectly fine. But WE NEED ONE that's the point.
And not simply remove everything without any replacement!

@olamy
Copy link
Member

olamy commented Aug 28, 2023

logging capability restored here 3b318bb

@elharo
Copy link
Contributor Author

elharo commented Aug 28, 2023

In the short term I'm OK with slf4j-simple but logback should never have been added. It's needless dependency bloat, and every extra dependency is a security risk.

In the long term, nothing beyond the JDK is needed, nor has been since Java 1.4.

I don't think it's a good idea to commit changes without going through PRs and code review. I would have approved 3b318bb if it had been sent through the usual process, but the process should be followed. Ideally the repo should be configured to prevent accidental and deliberate pushes to master. I know I've fumble fingered that one more than once by editing on master when I thought I was on a branch,.

@olamy
Copy link
Member

olamy commented Aug 28, 2023

@elharo did you really follow any process here? Some of us added some comments/concerns but you have still merged it.
So I'm trying to figure out your comment about the following process.

@elharo
Copy link
Contributor Author

elharo commented Aug 28, 2023

Absolutely. I sent a PR. People responded. No one disapproved the PR by sending a "Request changes" review. I responded to the comments, received an approval from another committer, and merged. That is exactly how it is supposed to go.

What is not supposed to happen is anyone committing anything directly to master and then pushing the branch with no approvals or review. 3b318bb could and should have been sent as a PR to be reviewed, and then merged after review and approval like any other change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants