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

Fix SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder" #4883

Merged
merged 1 commit into from
Nov 6, 2023

Conversation

claudio4j
Copy link
Contributor

@claudio4j claudio4j commented Oct 30, 2023

camel-k-maven-logging from camel-k-runtime will bring slf4j-api 2.0.
slf4j-api 1.7 originally from maven zip should be removed

Fix #4882

The camel-k-runtime fix 1100 should be merged first

Release Note

NONE

Copy link
Contributor

@squakez squakez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I'm converting to a draft as we want to retest and merge this once the new default runtime version with the fix is available.

However this shows a runtime dependency we need to remove. This was something we included in the runtime when we had a single runtime. It makes sense to move any required dependency off the runtime now that we want to make it independent. Going to open a follow up issue. Or do you think it is something you want to tackle now? we should be able to easily move back the logging module into Camel K and let any operator configuration to be inside the right repository.

@squakez squakez marked this pull request as draft October 31, 2023 08:22
@claudio4j
Copy link
Contributor Author

the runtime dependency is not good, we can deal with this exclusively on camel-k operator side, by having the maven_overlay script to check the slf4j / logback compatibility and add the missing slf4j-2.0.jar, wdyt ?

@squakez
Copy link
Contributor

squakez commented Oct 31, 2023

Yes. This is kind of how it was managed before [1]. However, we used to have a static set of dependencies. We should have instead an internal maven project in order to have dependabot and all the stuff on it.

[1] #3376

@claudio4j
Copy link
Contributor Author

A proposal would be to move the camel-k-runtime/camel-k-maven-logging to camel-k/java project.

@claudio4j claudio4j force-pushed the upstream/main_fix_slf4j_maven_dep branch from 298eb11 to cdbc335 Compare November 2, 2023 17:40
@claudio4j claudio4j marked this pull request as ready for review November 2, 2023 17:40
Copy link
Contributor

@squakez squakez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the crds and the logging have to be separate maven projects. During release process we publish the CRDs, while we're not willing to publish the maven logging dependency (which is only used internally). For the rest, it looks fine.

@claudio4j
Copy link
Contributor Author

Do you mean to remove the parent java/pom.xml ?

@squakez
Copy link
Contributor

squakez commented Nov 3, 2023

Do you mean to remove the parent java/pom.xml ?

Yes. CRDs and Logging have to be separate poms.

@claudio4j claudio4j force-pushed the upstream/main_fix_slf4j_maven_dep branch from cdbc335 to d82bcb7 Compare November 3, 2023 11:38
Copy link
Contributor

@squakez squakez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're deleting the symbolic links to the crds and the license/notice files. They must be part of the CRD project.

script/Makefile Show resolved Hide resolved
@claudio4j claudio4j force-pushed the upstream/main_fix_slf4j_maven_dep branch from d82bcb7 to c851fc7 Compare November 3, 2023 12:36
@claudio4j
Copy link
Contributor Author

Somehow the symlink got lost, I will link them.

@claudio4j claudio4j force-pushed the upstream/main_fix_slf4j_maven_dep branch 2 times, most recently from 4415e5d to c2121c9 Compare November 3, 2023 12:50
@claudio4j
Copy link
Contributor Author

I think this fix is important for 2.1.x, I will provide a PR soon.

@claudio4j claudio4j force-pushed the upstream/main_fix_slf4j_maven_dep branch from c2121c9 to 534eb13 Compare November 3, 2023 14:57
moved camel-k-maven-logging from camel-k-runtime to camel-k project
to make the maven logging independent of the camel-k-runtime
slf4j-api 1.7 originally from maven zip should be removed

Fix apache#4882
@claudio4j claudio4j force-pushed the upstream/main_fix_slf4j_maven_dep branch from 534eb13 to dd04657 Compare November 3, 2023 15:07
@claudio4j claudio4j merged commit 0c1c01d into apache:main Nov 6, 2023
12 of 13 checks passed
claudio4j added a commit to claudio4j/camel-k that referenced this pull request Nov 6, 2023
…pache#4883)

moved camel-k-maven-logging from camel-k-runtime to camel-k project
to make the maven logging independent of the camel-k-runtime
slf4j-api 1.7 originally from maven zip should be removed

Fix apache#4882

(cherry picked from commit 0c1c01d)
claudio4j added a commit that referenced this pull request Nov 6, 2023
…4883) (#4900)

moved camel-k-maven-logging from camel-k-runtime to camel-k project
to make the maven logging independent of the camel-k-runtime
slf4j-api 1.7 originally from maven zip should be removed

Fix #4882

(cherry picked from commit 0c1c01d)
@claudio4j claudio4j deleted the upstream/main_fix_slf4j_maven_dep branch November 6, 2023 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder" when running an integration
3 participants