You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by GitBox <gi...@apache.org> on 2021/08/02 11:15:48 UTC
[GitHub] [camel-k] orpiske opened a new pull request #2540: Forcelly set QUARKUS_LOG_CONSOLE_JSON to false if the trait is not provided (GH issue #2539)
orpiske opened a new pull request #2540:
URL: https://github.com/apache/camel-k/pull/2540
<!-- Description -->
Ensures that QUARKUS_LOG_CONSOLE_JSON is set to false if the `logging.json` trait is not provided
<!--
Enter your extended release note in the below block. If the PR requires
additional action from users switching to the new release, include the string
"action required". If no release note is required, write "NONE".
You can (optionally) mark this PR with labels "kind/bug" or "kind/feature" to make sure
the text is added to the right section of the release notes.
-->
**Release Note**
```release-note
Ensures that QUARKUS_LOG_CONSOLE_JSON is set to false if the `logging.json` trait is not provided
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] orpiske commented on a change in pull request #2540: Forcelly set QUARKUS_LOG_CONSOLE_JSON to false if the trait is not provided (GH issue #2539)
Posted by GitBox <gi...@apache.org>.
orpiske commented on a change in pull request #2540:
URL: https://github.com/apache/camel-k/pull/2540#discussion_r680934535
##########
File path: pkg/trait/logging.go
##########
@@ -91,6 +91,7 @@ func (l loggingTrait) Apply(environment *Environment) error {
envvar.SetVal(&environment.EnvVars, envVarQuarkusLogConsoleJsonPrettyPrint, True)
}
} else if util.IsNilOrTrue(l.Color) {
Review comment:
Color defaults to true, which is why it would usually work ... but the logic should be independent here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] orpiske commented on pull request #2540: Forcelly set QUARKUS_LOG_CONSOLE_JSON to false if the trait is not provided (GH issue #2539)
Posted by GitBox <gi...@apache.org>.
orpiske commented on pull request #2540:
URL: https://github.com/apache/camel-k/pull/2540#issuecomment-890951222
> > > @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](https://github.com/apache/camel-k/blob/main/pkg/trait/logging.go?rgh-link-date=2021-08-02T11%3A24%3A10Z#L32) 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](https://github.com/apache/camel-k-runtime/blob/main/camel-k-runtime/runtime/pom.xml#L30)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] lburgazzoli commented on pull request #2540: Forcelly set QUARKUS_LOG_CONSOLE_JSON to false if the trait is not provided (GH issue #2539)
Posted by GitBox <gi...@apache.org>.
lburgazzoli commented on pull request #2540:
URL: https://github.com/apache/camel-k/pull/2540#issuecomment-890949567
> > @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](https://github.com/apache/camel-k/blob/main/pkg/trait/logging.go?rgh-link-date=2021-08-02T11%3A24%3A10Z#L32) 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] orpiske commented on a change in pull request #2540: Forcelly set QUARKUS_LOG_CONSOLE_JSON to false if the trait is not provided (GH issue #2539)
Posted by GitBox <gi...@apache.org>.
orpiske commented on a change in pull request #2540:
URL: https://github.com/apache/camel-k/pull/2540#discussion_r681131159
##########
File path: pkg/trait/logging.go
##########
@@ -90,8 +90,12 @@ func (l loggingTrait) Apply(environment *Environment) error {
if util.IsTrue(l.JsonPrettyPrint) {
envvar.SetVal(&environment.EnvVars, envVarQuarkusLogConsoleJsonPrettyPrint, True)
}
- } else if util.IsNilOrTrue(l.Color) {
- envvar.SetVal(&environment.EnvVars, envVarQuarkusLogConsoleColor, True)
+ } else {
+ envvar.SetVal(&environment.EnvVars, envVarQuarkusLogConsoleJson, False)
Review comment:
@astefanutti @lburgazzoli thanks guys. So, let's go with this as a work-around for now.
Antonin's raised a good point here and I think I can take a look at solving this when I cleanup/refactor for #2541. At the very least this gives us some time to think about it and consider the approach.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] orpiske commented on a change in pull request #2540: Forcelly set QUARKUS_LOG_CONSOLE_JSON to false if the trait is not provided (GH issue #2539)
Posted by GitBox <gi...@apache.org>.
orpiske commented on a change in pull request #2540:
URL: https://github.com/apache/camel-k/pull/2540#discussion_r680956142
##########
File path: pkg/trait/logging.go
##########
@@ -90,8 +90,12 @@ func (l loggingTrait) Apply(environment *Environment) error {
if util.IsTrue(l.JsonPrettyPrint) {
envvar.SetVal(&environment.EnvVars, envVarQuarkusLogConsoleJsonPrettyPrint, True)
}
- } else if util.IsNilOrTrue(l.Color) {
- envvar.SetVal(&environment.EnvVars, envVarQuarkusLogConsoleColor, True)
+ } else {
+ envvar.SetVal(&environment.EnvVars, envVarQuarkusLogConsoleJson, False)
Review comment:
@astefanutti to be clear: I am also fine with your suggestion ... Although I think it's safer to be explicit here, I am not strongly opinionated towards what I suggested. As such, so if you do feel it's the best way ... I am fine with it too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] lburgazzoli commented on pull request #2540: Forcelly set QUARKUS_LOG_CONSOLE_JSON to false if the trait is not provided (GH issue #2539)
Posted by GitBox <gi...@apache.org>.
lburgazzoli commented on pull request #2540:
URL: https://github.com/apache/camel-k/pull/2540#issuecomment-890954131
> > > > @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](https://github.com/apache/camel-k/blob/main/pkg/trait/logging.go?rgh-link-date=2021-08-02T11%3A29%3A32Z#L32) 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](https://github.com/apache/camel-k-runtime/blob/main/camel-k-runtime/runtime/pom.xml?rgh-link-date=2021-08-02T11%3A29%3A32Z#L30)?
I think we can defer this additional dependency to camel-k 1.6 & camel-k-runtime 1.9
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] orpiske commented on a change in pull request #2540: Forcelly set QUARKUS_LOG_CONSOLE_JSON to false if the trait is not provided (GH issue #2539)
Posted by GitBox <gi...@apache.org>.
orpiske commented on a change in pull request #2540:
URL: https://github.com/apache/camel-k/pull/2540#discussion_r680946583
##########
File path: pkg/trait/logging.go
##########
@@ -90,8 +90,12 @@ func (l loggingTrait) Apply(environment *Environment) error {
if util.IsTrue(l.JsonPrettyPrint) {
envvar.SetVal(&environment.EnvVars, envVarQuarkusLogConsoleJsonPrettyPrint, True)
}
- } else if util.IsNilOrTrue(l.Color) {
- envvar.SetVal(&environment.EnvVars, envVarQuarkusLogConsoleColor, True)
+ } else {
+ envvar.SetVal(&environment.EnvVars, envVarQuarkusLogConsoleJson, False)
Review comment:
I see your point and I think it makes sense. There is a risk, though.
Right now we add the required library for logging only if the [json flag is true](https://github.com/apache/camel-k/blob/main/pkg/trait/logging.go#L76).
If we change that, we risk getting into a situation where Quarkus changes the default logging behavior from plain text to json or some other way/mechanism and we are left without control.
So, although this is very unlikely, I still think it would be better to be explicit in this particular case, as we are talking about a core feature of the integration.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] lburgazzoli commented on pull request #2540: Forcelly set QUARKUS_LOG_CONSOLE_JSON to false if the trait is not provided (GH issue #2539)
Posted by GitBox <gi...@apache.org>.
lburgazzoli commented on pull request #2540:
URL: https://github.com/apache/camel-k/pull/2540#issuecomment-890944744
@orpiske would this env var lead to a warning if the json loggign extension is not on the classpath ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] orpiske edited a comment on pull request #2540: Forcelly set QUARKUS_LOG_CONSOLE_JSON to false if the trait is not provided (GH issue #2539)
Posted by GitBox <gi...@apache.org>.
orpiske edited a comment on pull request #2540:
URL: https://github.com/apache/camel-k/pull/2540#issuecomment-890956477
> > > > > @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](https://github.com/apache/camel-k/blob/main/pkg/trait/logging.go?rgh-link-date=2021-08-02T11%3A29%3A32Z#L32) 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](https://github.com/apache/camel-k-runtime/blob/main/camel-k-runtime/runtime/pom.xml?rgh-link-date=2021-08-02T11%3A29%3A32Z#L30)?
>
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] lburgazzoli commented on a change in pull request #2540: Forcelly set QUARKUS_LOG_CONSOLE_JSON to false if the trait is not provided (GH issue #2539)
Posted by GitBox <gi...@apache.org>.
lburgazzoli commented on a change in pull request #2540:
URL: https://github.com/apache/camel-k/pull/2540#discussion_r681077808
##########
File path: pkg/trait/logging.go
##########
@@ -90,8 +90,12 @@ func (l loggingTrait) Apply(environment *Environment) error {
if util.IsTrue(l.JsonPrettyPrint) {
envvar.SetVal(&environment.EnvVars, envVarQuarkusLogConsoleJsonPrettyPrint, True)
}
- } else if util.IsNilOrTrue(l.Color) {
- envvar.SetVal(&environment.EnvVars, envVarQuarkusLogConsoleColor, True)
+ } else {
+ envvar.SetVal(&environment.EnvVars, envVarQuarkusLogConsoleJson, False)
Review comment:
I don't have any strong opinion here and both solution are fine
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] lburgazzoli commented on a change in pull request #2540: Forcelly set QUARKUS_LOG_CONSOLE_JSON to false if the trait is not provided (GH issue #2539)
Posted by GitBox <gi...@apache.org>.
lburgazzoli commented on a change in pull request #2540:
URL: https://github.com/apache/camel-k/pull/2540#discussion_r681077808
##########
File path: pkg/trait/logging.go
##########
@@ -90,8 +90,12 @@ func (l loggingTrait) Apply(environment *Environment) error {
if util.IsTrue(l.JsonPrettyPrint) {
envvar.SetVal(&environment.EnvVars, envVarQuarkusLogConsoleJsonPrettyPrint, True)
}
- } else if util.IsNilOrTrue(l.Color) {
- envvar.SetVal(&environment.EnvVars, envVarQuarkusLogConsoleColor, True)
+ } else {
+ envvar.SetVal(&environment.EnvVars, envVarQuarkusLogConsoleJson, False)
Review comment:
I don0t have any strong opinion here and both solution are fine
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] orpiske commented on pull request #2540: Forcelly set QUARKUS_LOG_CONSOLE_JSON to false if the trait is not provided (GH issue #2539)
Posted by GitBox <gi...@apache.org>.
orpiske commented on pull request #2540:
URL: https://github.com/apache/camel-k/pull/2540#issuecomment-890956477
> > > > > @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](https://github.com/apache/camel-k/blob/main/pkg/trait/logging.go?rgh-link-date=2021-08-02T11%3A29%3A32Z#L32) 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](https://github.com/apache/camel-k-runtime/blob/main/camel-k-runtime/runtime/pom.xml?rgh-link-date=2021-08-02T11%3A29%3A32Z#L30)?
>
> 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. 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] astefanutti commented on a change in pull request #2540: Forcelly set QUARKUS_LOG_CONSOLE_JSON to false if the trait is not provided (GH issue #2539)
Posted by GitBox <gi...@apache.org>.
astefanutti commented on a change in pull request #2540:
URL: https://github.com/apache/camel-k/pull/2540#discussion_r680938708
##########
File path: pkg/trait/logging.go
##########
@@ -90,8 +90,12 @@ func (l loggingTrait) Apply(environment *Environment) error {
if util.IsTrue(l.JsonPrettyPrint) {
envvar.SetVal(&environment.EnvVars, envVarQuarkusLogConsoleJsonPrettyPrint, True)
}
- } else if util.IsNilOrTrue(l.Color) {
- envvar.SetVal(&environment.EnvVars, envVarQuarkusLogConsoleColor, True)
+ } else {
+ envvar.SetVal(&environment.EnvVars, envVarQuarkusLogConsoleJson, False)
Review comment:
I wonder, should this be done only when the user has set the `json` property to false? In general I think it's desirable to inherit from the Quarkus default behaviour, and avoid forcing environment variables.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] astefanutti commented on a change in pull request #2540: Forcelly set QUARKUS_LOG_CONSOLE_JSON to false if the trait is not provided (GH issue #2539)
Posted by GitBox <gi...@apache.org>.
astefanutti commented on a change in pull request #2540:
URL: https://github.com/apache/camel-k/pull/2540#discussion_r680932158
##########
File path: pkg/trait/logging.go
##########
@@ -91,6 +91,7 @@ func (l loggingTrait) Apply(environment *Environment) error {
envvar.SetVal(&environment.EnvVars, envVarQuarkusLogConsoleJsonPrettyPrint, True)
}
} else if util.IsNilOrTrue(l.Color) {
Review comment:
Is only when `l.Color` `nil` or `true` the right place?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] orpiske commented on pull request #2540: Forcelly set QUARKUS_LOG_CONSOLE_JSON to false if the trait is not provided (GH issue #2539)
Posted by GitBox <gi...@apache.org>.
orpiske commented on pull request #2540:
URL: https://github.com/apache/camel-k/pull/2540#issuecomment-890947687
> @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](https://github.com/apache/camel-k/blob/main/pkg/trait/logging.go#L32) 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] orpiske commented on a change in pull request #2540: Forcelly set QUARKUS_LOG_CONSOLE_JSON to false if the trait is not provided (GH issue #2539)
Posted by GitBox <gi...@apache.org>.
orpiske commented on a change in pull request #2540:
URL: https://github.com/apache/camel-k/pull/2540#discussion_r680941361
##########
File path: pkg/trait/logging.go
##########
@@ -90,8 +90,12 @@ func (l loggingTrait) Apply(environment *Environment) error {
if util.IsTrue(l.JsonPrettyPrint) {
envvar.SetVal(&environment.EnvVars, envVarQuarkusLogConsoleJsonPrettyPrint, True)
}
- } else if util.IsNilOrTrue(l.Color) {
- envvar.SetVal(&environment.EnvVars, envVarQuarkusLogConsoleColor, True)
+ } else {
+ envvar.SetVal(&environment.EnvVars, envVarQuarkusLogConsoleJson, False)
Review comment:
In this case, no. That would cause the problem that @lburgazzoli raised.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] astefanutti commented on a change in pull request #2540: Forcelly set QUARKUS_LOG_CONSOLE_JSON to false if the trait is not provided (GH issue #2539)
Posted by GitBox <gi...@apache.org>.
astefanutti commented on a change in pull request #2540:
URL: https://github.com/apache/camel-k/pull/2540#discussion_r680942657
##########
File path: pkg/trait/logging.go
##########
@@ -90,8 +90,12 @@ func (l loggingTrait) Apply(environment *Environment) error {
if util.IsTrue(l.JsonPrettyPrint) {
envvar.SetVal(&environment.EnvVars, envVarQuarkusLogConsoleJsonPrettyPrint, True)
}
- } else if util.IsNilOrTrue(l.Color) {
- envvar.SetVal(&environment.EnvVars, envVarQuarkusLogConsoleColor, True)
+ } else {
+ envvar.SetVal(&environment.EnvVars, envVarQuarkusLogConsoleJson, False)
Review comment:
My understanding is that in @lburgazzoli case, he disables it explicitly with `trait.camel.apache.org/logging.json: "false"`, so that would work?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] orpiske commented on a change in pull request #2540: Forcelly set QUARKUS_LOG_CONSOLE_JSON to false if the trait is not provided (GH issue #2539)
Posted by GitBox <gi...@apache.org>.
orpiske commented on a change in pull request #2540:
URL: https://github.com/apache/camel-k/pull/2540#discussion_r680932807
##########
File path: pkg/trait/logging.go
##########
@@ -91,6 +91,7 @@ func (l loggingTrait) Apply(environment *Environment) error {
envvar.SetVal(&environment.EnvVars, envVarQuarkusLogConsoleJsonPrettyPrint, True)
}
} else if util.IsNilOrTrue(l.Color) {
Review comment:
Oops. Good catch!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] astefanutti merged pull request #2540: Forcelly set QUARKUS_LOG_CONSOLE_JSON to false if the trait is not provided (GH issue #2539)
Posted by GitBox <gi...@apache.org>.
astefanutti merged pull request #2540:
URL: https://github.com/apache/camel-k/pull/2540
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] orpiske commented on a change in pull request #2540: Forcelly set QUARKUS_LOG_CONSOLE_JSON to false if the trait is not provided (GH issue #2539)
Posted by GitBox <gi...@apache.org>.
orpiske commented on a change in pull request #2540:
URL: https://github.com/apache/camel-k/pull/2540#discussion_r680946583
##########
File path: pkg/trait/logging.go
##########
@@ -90,8 +90,12 @@ func (l loggingTrait) Apply(environment *Environment) error {
if util.IsTrue(l.JsonPrettyPrint) {
envvar.SetVal(&environment.EnvVars, envVarQuarkusLogConsoleJsonPrettyPrint, True)
}
- } else if util.IsNilOrTrue(l.Color) {
- envvar.SetVal(&environment.EnvVars, envVarQuarkusLogConsoleColor, True)
+ } else {
+ envvar.SetVal(&environment.EnvVars, envVarQuarkusLogConsoleJson, False)
Review comment:
I see your point and I think it makes sense. There is a risk, though.
Right now we add the required library for logging only if the [json flat is true](https://github.com/apache/camel-k/blob/main/pkg/trait/logging.go#L76).
If we change that, we risk getting into a situation where Quarkus changes the default logging behavior from plain text to json or some other way/mechanism and we are left without control.
So, although this is very unlikely, I still think it would be better to be explicit in this particular case, as we are talking about a core feature of the integration.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] astefanutti commented on a change in pull request #2540: Forcelly set QUARKUS_LOG_CONSOLE_JSON to false if the trait is not provided (GH issue #2539)
Posted by GitBox <gi...@apache.org>.
astefanutti commented on a change in pull request #2540:
URL: https://github.com/apache/camel-k/pull/2540#discussion_r681127796
##########
File path: pkg/trait/logging.go
##########
@@ -90,8 +90,12 @@ func (l loggingTrait) Apply(environment *Environment) error {
if util.IsTrue(l.JsonPrettyPrint) {
envvar.SetVal(&environment.EnvVars, envVarQuarkusLogConsoleJsonPrettyPrint, True)
}
- } else if util.IsNilOrTrue(l.Color) {
- envvar.SetVal(&environment.EnvVars, envVarQuarkusLogConsoleColor, True)
+ } else {
+ envvar.SetVal(&environment.EnvVars, envVarQuarkusLogConsoleJson, False)
Review comment:
I don't have a strong opinion either 😀. I'd be slightly inclined toward not adding a particular case. I guess the overall question is should Camel K be responsible for defaulting Quarkus runtime configuration, or should it delegates to Quarkus. Until we have a consensus on what is the best approach, it's not a big deal to choose one or the other for a few particular cases, so either way is fine with me 👍🏼.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org