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

Forcelly set QUARKUS_LOG_CONSOLE_JSON to false if the trait is not provided (GH issue #2539) #2540

Merged
merged 1 commit into from
Aug 2, 2021

Conversation

orpiske
Copy link
Contributor

@orpiske orpiske commented Aug 2, 2021

Ensures that QUARKUS_LOG_CONSOLE_JSON is set to false if the logging.json trait is not provided

Release Note

Ensures that QUARKUS_LOG_CONSOLE_JSON is set to false if the `logging.json` trait is not provided

@lburgazzoli
Copy link
Contributor

@orpiske would this env var lead to a warning if the json loggign extension is not on the classpath ?

@orpiske
Copy link
Contributor Author

orpiske commented Aug 2, 2021

@orpiske would this env var lead to a warning if the json loggign extension is not on the classpath ?

I believe the warning should go away in this case. I was looking in the code and we are already including the quarkus-logging-json in the classpath when the json logging is true.

Now that we changed the default behavior if unset, I don't think we should get into a situation where it tries to log via json and it is not on the classpath. What do you think?

@lburgazzoli
Copy link
Contributor

@orpiske would this env var lead to a warning if the json loggign extension is not on the classpath ?

I believe the warning should go away in this case. I was looking in the code and we are already including the quarkus-logging-json in the classpath when the json logging is true.

Now that we changed the default behavior if unset, I don't think we should get into a situation where it tries to log via json and it is not on the classpath. What do you think?

Looks good to me, wonder if we can add io.quarkus:quarkus-logging-json as transitive dependency o camel-k so this trait would not need to change the dependencies. This would also ensure the dependency to be present inc case you provide your own container image

@orpiske
Copy link
Contributor Author

orpiske commented Aug 2, 2021

@orpiske would this env var lead to a warning if the json loggign extension is not on the classpath ?

I believe the warning should go away in this case. I was looking in the code and we are already including the quarkus-logging-json in the classpath when the json logging is true.
Now that we changed the default behavior if unset, I don't think we should get into a situation where it tries to log via json and it is not on the classpath. What do you think?

Looks good to me, wonder if we can add io.quarkus:quarkus-logging-json as transitive dependency o camel-k so this trait would not need to change the dependencies. This would also ensure the dependency to be present inc case you provide your own container image

The only likely downside is that we would need release both the runtime and camel K if we decide to backport this fix ... but, frankly, I am fine with both approaches.

Maybe we could add it here in the Runtime?

@lburgazzoli
Copy link
Contributor

@orpiske would this env var lead to a warning if the json loggign extension is not on the classpath ?

I believe the warning should go away in this case. I was looking in the code and we are already including the quarkus-logging-json in the classpath when the json logging is true.
Now that we changed the default behavior if unset, I don't think we should get into a situation where it tries to log via json and it is not on the classpath. What do you think?

Looks good to me, wonder if we can add io.quarkus:quarkus-logging-json as transitive dependency o camel-k so this trait would not need to change the dependencies. This would also ensure the dependency to be present inc case you provide your own container image

The only likely downside is that we would need release both the runtime and camel K if we decide to backport this fix ... but, frankly, I am fine with both approaches.

Maybe we could add it here in the Runtime?

I think we can defer this additional dependency to camel-k 1.6 & camel-k-runtime 1.9

@orpiske
Copy link
Contributor Author

orpiske commented Aug 2, 2021

@orpiske would this env var lead to a warning if the json loggign extension is not on the classpath ?

I believe the warning should go away in this case. I was looking in the code and we are already including the quarkus-logging-json in the classpath when the json logging is true.
Now that we changed the default behavior if unset, I don't think we should get into a situation where it tries to log via json and it is not on the classpath. What do you think?

Looks good to me, wonder if we can add io.quarkus:quarkus-logging-json as transitive dependency o camel-k so this trait would not need to change the dependencies. This would also ensure the dependency to be present inc case you provide your own container image

The only likely downside is that we would need release both the runtime and camel K if we decide to backport this fix ... but, frankly, I am fine with both approaches.
Maybe we could add it here in the Runtime?

I think we can defer this additional dependency to camel-k 1.6 & camel-k-runtime 1.9

Deal. Let me merge this as a work-around for now, then I'll backport it to 1.5 (in case we decide for a point release). I'll add a task to myself to do this cleanup in the 1.6 time frame and will take care of it when I come back from PTO.

@orpiske orpiske added this to the 1.5.1 milestone Aug 2, 2021
pkg/trait/logging.go Outdated Show resolved Hide resolved
@astefanutti astefanutti merged commit 80bdbf5 into apache:main Aug 2, 2021
@nicolaferraro nicolaferraro mentioned this pull request Sep 3, 2021
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.

None yet

3 participants