You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by "christophd (via GitHub)" <gi...@apache.org> on 2024/03/12 19:52:21 UTC
[PR] fix(#5242): Disable noErrorHandler setting for Camel 4.4.0 [camel-k]
christophd opened a new pull request, #5245:
URL: https://github.com/apache/camel-k/pull/5245
- Runtime version 3.8.0+ requires this setting so pipe error handler works as expected
- noErrorHandler is set for Kamelets by default since Camel 4.4.0
**Release Note**
```release-note
fix(#5242): Disable noErrorHandler setting for Camel 4.4.0 to make pipe error handler 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
Re: [PR] fix(#5242): Disable noErrorHandler setting for Camel 4.4.0 [camel-k]
Posted by "christophd (via GitHub)" <gi...@apache.org>.
christophd commented on PR #5245:
URL: https://github.com/apache/camel-k/pull/5245#issuecomment-1997728926
@lburgazzoli no worries I am fine. let's close this PR then
--
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
Re: [PR] fix(#5242): Disable noErrorHandler setting for Camel 4.4.0 [camel-k]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5245:
URL: https://github.com/apache/camel-k/pull/5245#issuecomment-1992465234
:heavy_check_mark: Unit test coverage report - coverage increased from 37.2% to 37.3% (**+0.1%**)
--
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
Re: [PR] fix(#5242): Disable noErrorHandler setting for Camel 4.4.0 [camel-k]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5245:
URL: https://github.com/apache/camel-k/pull/5245#issuecomment-1992592391
:heavy_check_mark: Unit test coverage report - coverage increased from 37.2% to 37.3% (**+0.1%**)
--
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
Re: [PR] fix(#5242): Disable noErrorHandler setting for Camel 4.4.0 [camel-k]
Posted by "christophd (via GitHub)" <gi...@apache.org>.
christophd commented on code in PR #5245:
URL: https://github.com/apache/camel-k/pull/5245#discussion_r1522849152
##########
pkg/trait/error_handler.go:
##########
@@ -67,6 +69,17 @@ func (t *errorHandlerTrait) Apply(e *Environment) error {
t.addErrorHandlerDependencies(e, defaultErrorHandlerURI)
}
+ if shouldHandleNoErrorHandler(e.Integration) {
+ // noErrorHandler is enabled by default on Kamelets since Camel 4.4.0 (runtimeVersion 3.8.0)
+ // need to disable this setting so that pipe error handler works
+ confValue, err := property.EncodePropertyFileEntry("camel.component.kamelet.noErrorHandler", "false")
+ if err != nil {
+ return err
+ }
+
+ e.Integration.Spec.AddConfigurationProperty(confValue)
Review Comment:
sorry, can you please elaborate? I do not understand
--
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
Re: [PR] fix(#5242): Disable noErrorHandler setting for Camel 4.4.0 [camel-k]
Posted by "christophd (via GitHub)" <gi...@apache.org>.
christophd commented on code in PR #5245:
URL: https://github.com/apache/camel-k/pull/5245#discussion_r1522686476
##########
pkg/controller/pipe/integration.go:
##########
@@ -210,6 +218,27 @@ func CreateIntegrationFor(ctx context.Context, c client.Client, binding *v1.Pipe
return &it, nil
}
+// shouldHandleNoErrorHandler determines the runtime version and checks on noErrorHandler that is configured for this version.
+func shouldHandleNoErrorHandler(ctx context.Context, c client.Client, it *v1.Integration) bool {
+ var runtimeVersion string
+ if it.Spec.Traits.Camel != nil && it.Spec.Traits.Camel.RuntimeVersion != "" {
+ runtimeVersion = it.Spec.Traits.Camel.RuntimeVersion
+ } else if pl, err := platform.GetForResource(ctx, c, it); err == nil && pl != nil {
Review Comment:
No, I don't think so as the integration resource is being created by the pipe controller at this stage. it has no status yet as it has not been created yet
--
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
Re: [PR] fix(#5242): Disable noErrorHandler setting for Camel 4.4.0 [camel-k]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5245:
URL: https://github.com/apache/camel-k/pull/5245#issuecomment-1993868003
:heavy_check_mark: Unit test coverage report - coverage increased from 37.2% to 37.3% (**+0.1%**)
--
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
Re: [PR] fix(#5242): Disable noErrorHandler setting for Camel 4.4.0 [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on code in PR #5245:
URL: https://github.com/apache/camel-k/pull/5245#discussion_r1523000539
##########
pkg/trait/error_handler.go:
##########
@@ -67,6 +69,17 @@ func (t *errorHandlerTrait) Apply(e *Environment) error {
t.addErrorHandlerDependencies(e, defaultErrorHandlerURI)
}
+ if shouldHandleNoErrorHandler(e.Integration) {
+ // noErrorHandler is enabled by default on Kamelets since Camel 4.4.0 (runtimeVersion 3.8.0)
+ // need to disable this setting so that pipe error handler works
+ confValue, err := property.EncodePropertyFileEntry("camel.component.kamelet.noErrorHandler", "false")
+ if err != nil {
+ return err
+ }
+
+ e.Integration.Spec.AddConfigurationProperty(confValue)
Review Comment:
Something like `e.ApplicationProperties["key"]="value"`
--
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
Re: [PR] fix(#5242): Disable noErrorHandler setting for Camel 4.4.0 [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on code in PR #5245:
URL: https://github.com/apache/camel-k/pull/5245#discussion_r1522631293
##########
pkg/controller/pipe/integration.go:
##########
@@ -120,6 +122,12 @@ func CreateIntegrationFor(ctx context.Context, c client.Client, binding *v1.Pipe
return nil, fmt.Errorf("could not determine error handler: %w", err)
}
+ if errorHandler != nil && shouldHandleNoErrorHandler(ctx, c, &it) {
Review Comment:
This piece of code, if any, has to go to the error handler trait. It does not belong to the Controlller IMO.
##########
pkg/controller/pipe/integration_test.go:
##########
@@ -54,6 +55,78 @@ func TestCreateIntegrationForPipe(t *testing.T) {
assert.Equal(t, expectedNominalRoute(), string(dsl))
}
+func TestCreateIntegrationForPipeWithSinkErrorHandler(t *testing.T) {
+ client, err := test.NewFakeClient()
+ require.NoError(t, err)
+
+ pipe := nominalPipe("my-error-handler-pipe")
+ pipe.Spec.Integration = &v1.IntegrationSpec{
+ Traits: v1.Traits{
+ Camel: &trait.CamelTrait{
+ RuntimeVersion: "3.8.0-SNAPSHOT", // forces noErrorHandler=false property
Review Comment:
Probably you want to put `defaults.RuntimeVersion` here.
##########
pkg/controller/pipe/integration.go:
##########
@@ -210,6 +218,27 @@ func CreateIntegrationFor(ctx context.Context, c client.Client, binding *v1.Pipe
return &it, nil
}
+// shouldHandleNoErrorHandler determines the runtime version and checks on noErrorHandler that is configured for this version.
+func shouldHandleNoErrorHandler(ctx context.Context, c client.Client, it *v1.Integration) bool {
+ var runtimeVersion string
+ if it.Spec.Traits.Camel != nil && it.Spec.Traits.Camel.RuntimeVersion != "" {
+ runtimeVersion = it.Spec.Traits.Camel.RuntimeVersion
+ } else if pl, err := platform.GetForResource(ctx, c, it); err == nil && pl != nil {
Review Comment:
No need to peek the platform. At trait level stage, we must have already set the Integration.Status.RuntimeVersion.
##########
pkg/controller/pipe/integration.go:
##########
@@ -210,6 +218,27 @@ func CreateIntegrationFor(ctx context.Context, c client.Client, binding *v1.Pipe
return &it, nil
}
+// shouldHandleNoErrorHandler determines the runtime version and checks on noErrorHandler that is configured for this version.
+func shouldHandleNoErrorHandler(ctx context.Context, c client.Client, it *v1.Integration) bool {
+ var runtimeVersion string
+ if it.Spec.Traits.Camel != nil && it.Spec.Traits.Camel.RuntimeVersion != "" {
+ runtimeVersion = it.Spec.Traits.Camel.RuntimeVersion
+ } else if pl, err := platform.GetForResource(ctx, c, it); err == nil && pl != nil {
+ runtimeVersion = pl.Status.Build.RuntimeVersion
+ }
+
+ if runtimeVersion != "" {
+ runtimeVersion, _ = strings.CutSuffix(runtimeVersion, "-SNAPSHOT")
+ if versionNumber, err := strconv.Atoi(strings.ReplaceAll(runtimeVersion, ".", "")); err == nil {
Review Comment:
this would fail for those version that have suffix, ie, `3.8.0-my-companytag`.
--
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
Re: [PR] fix(#5242): Disable noErrorHandler setting for Camel 4.4.0 [camel-k]
Posted by "lburgazzoli (via GitHub)" <gi...@apache.org>.
lburgazzoli commented on PR #5245:
URL: https://github.com/apache/camel-k/pull/5245#issuecomment-1997087228
> @lburgazzoli I am currently trying to avoid the property right now and set the error handler ref directly on the route as Camel JBang would do. Unfortunately we would end up having both global error handler and route based error handler both referencing the same error handler. This is because we would like to support both Camel < 4.4.0 and 4.4.0+
is there anything we can do in the camel-k runtime maybe ? so in fact the operator won't have to change.
--
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
Re: [PR] fix(#5242): Disable noErrorHandler setting for Camel 4.4.0 [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on code in PR #5245:
URL: https://github.com/apache/camel-k/pull/5245#discussion_r1522827198
##########
pkg/apis/camel/v1/integration_types_support.go:
##########
@@ -116,6 +116,11 @@ func (in *IntegrationSpec) AddDependency(dependency string) {
in.Dependencies = append(in.Dependencies, dependency)
}
+// AddConfigurationProperty adds a new configuration property.
+func (in *IntegrationSpec) AddConfigurationProperty(confValue string) {
Review Comment:
`.integration.spec.configuration` is deprecated. We should not use it any longer.
##########
pkg/trait/error_handler_test.go:
##########
@@ -85,9 +85,9 @@ func TestErrorHandlerApplyDependency(t *testing.T) {
CamelCatalog: c,
Integration: &v1.Integration{},
}
- e.Integration.Spec.AddConfiguration("property", "camel.beans.defaultErrorHandler = #class:org.apache.camel.builder.DeadLetterChannelBuilder")
- e.Integration.Spec.AddConfiguration("property", "camel.beans.defaultErrorHandler.deadLetterUri = log:info")
- e.Integration.Spec.AddConfiguration("property", fmt.Sprintf("%v = %s", v1.ErrorHandlerRefName, "defaultErrorHandler"))
+ e.Integration.Spec.AddConfigurationProperty("camel.beans.defaultErrorHandler = #class:org.apache.camel.builder.DeadLetterChannelBuilder")
Review Comment:
Maybe we can take the opportunity and move into the non deprecated logic also these conf.
##########
pkg/trait/error_handler.go:
##########
@@ -67,6 +69,17 @@ func (t *errorHandlerTrait) Apply(e *Environment) error {
t.addErrorHandlerDependencies(e, defaultErrorHandlerURI)
}
+ if shouldHandleNoErrorHandler(e.Integration) {
+ // noErrorHandler is enabled by default on Kamelets since Camel 4.4.0 (runtimeVersion 3.8.0)
+ // need to disable this setting so that pipe error handler works
+ confValue, err := property.EncodePropertyFileEntry("camel.component.kamelet.noErrorHandler", "false")
+ if err != nil {
+ return err
+ }
+
+ e.Integration.Spec.AddConfigurationProperty(confValue)
Review Comment:
You need to add the property into the Environment and make sure this is called before the configmap holding `application.properties` is generated.
##########
pkg/trait/error_handler.go:
##########
@@ -67,6 +69,17 @@ func (t *errorHandlerTrait) Apply(e *Environment) error {
t.addErrorHandlerDependencies(e, defaultErrorHandlerURI)
}
+ if shouldHandleNoErrorHandler(e.Integration) {
+ // noErrorHandler is enabled by default on Kamelets since Camel 4.4.0 (runtimeVersion 3.8.0)
+ // need to disable this setting so that pipe error handler works
+ confValue, err := property.EncodePropertyFileEntry("camel.component.kamelet.noErrorHandler", "false")
Review Comment:
I think the main problem with this is that we are adding a patch, instead of handling the default value as the new Camel is meant to be. For this reason, IMO, we should manage this at runtime level. In other rejected PRs we have already mentioned these aspect should not concern the operator and should be managed by the runtime. Let's try to be consistent then.
--
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
Re: [PR] fix(#5242): Disable noErrorHandler setting for Camel 4.4.0 [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on code in PR #5245:
URL: https://github.com/apache/camel-k/pull/5245#discussion_r1522719827
##########
pkg/controller/pipe/integration.go:
##########
@@ -210,6 +218,27 @@ func CreateIntegrationFor(ctx context.Context, c client.Client, binding *v1.Pipe
return &it, nil
}
+// shouldHandleNoErrorHandler determines the runtime version and checks on noErrorHandler that is configured for this version.
+func shouldHandleNoErrorHandler(ctx context.Context, c client.Client, it *v1.Integration) bool {
+ var runtimeVersion string
+ if it.Spec.Traits.Camel != nil && it.Spec.Traits.Camel.RuntimeVersion != "" {
+ runtimeVersion = it.Spec.Traits.Camel.RuntimeVersion
+ } else if pl, err := platform.GetForResource(ctx, c, it); err == nil && pl != nil {
Review Comment:
Not here. It will be available in the trait. Even more, this has to be executed during the Integration.Status.Phase runtime only.
--
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
Re: [PR] fix(#5242): Disable noErrorHandler setting for Camel 4.4.0 [camel-k]
Posted by "christophd (via GitHub)" <gi...@apache.org>.
christophd commented on PR #5245:
URL: https://github.com/apache/camel-k/pull/5245#issuecomment-1992441398
Fixes #5242 and the nightly runtime check
--
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
Re: [PR] fix(#5242): Disable noErrorHandler setting for Camel 4.4.0 [camel-k]
Posted by "christophd (via GitHub)" <gi...@apache.org>.
christophd commented on code in PR #5245:
URL: https://github.com/apache/camel-k/pull/5245#discussion_r1522688753
##########
e2e/common/misc/pipe_test.go:
##########
@@ -63,7 +63,7 @@ func TestPipe(t *testing.T) {
"--name", "throw-error-binding",
).Execute()).To(Succeed())
- g.Eventually(IntegrationPodPhase(t, ns, "throw-error-binding"), TestTimeoutMedium).Should(Equal(corev1.PodRunning))
+ g.Eventually(IntegrationPodPhase(t, ns, "throw-error-binding"), TestTimeoutLong).Should(Equal(corev1.PodRunning))
Review Comment:
GitHub CI job tends to be slow sometimes and this is why the test is flaky. I checked the failing tests and the integration build is simply not finished after 5 mins sometimes
--
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
Re: [PR] fix(#5242): Disable noErrorHandler setting for Camel 4.4.0 [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on code in PR #5245:
URL: https://github.com/apache/camel-k/pull/5245#discussion_r1523002685
##########
pkg/trait/error_handler.go:
##########
@@ -107,3 +120,17 @@ func (t *errorHandlerTrait) addGlobalErrorHandlerAsSource(e *Environment) error
return nil
}
+
+// shouldHandleNoErrorHandler determines the runtime version and checks on noErrorHandler that is configured for this version.
+func shouldHandleNoErrorHandler(it *v1.Integration) bool {
+ if it.Status.RuntimeVersion != "" {
+ runtimeVersion, _ := strings.CutSuffix(it.Status.RuntimeVersion, "-SNAPSHOT")
+ if versionNumber, err := strconv.Atoi(strings.ReplaceAll(runtimeVersion, ".", "")); err == nil {
Review Comment:
We use `semver` dependency somewhere in the code. You may reuse that logic as well. In any case, I think this should be handled directly by the runtime, so that the operator doesn't know the version and we do not need to do all this hardcoding.
--
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
Re: [PR] fix(#5242): Disable noErrorHandler setting for Camel 4.4.0 [camel-k]
Posted by "christophd (via GitHub)" <gi...@apache.org>.
christophd commented on code in PR #5245:
URL: https://github.com/apache/camel-k/pull/5245#discussion_r1522784412
##########
pkg/controller/pipe/integration.go:
##########
@@ -120,6 +122,12 @@ func CreateIntegrationFor(ctx context.Context, c client.Client, binding *v1.Pipe
return nil, fmt.Errorf("could not determine error handler: %w", err)
}
+ if errorHandler != nil && shouldHandleNoErrorHandler(ctx, c, &it) {
Review Comment:
I had a look and you are right. Better place for this is the trait where the runtime version is already set in the IntegrationStatus. Thanks I will change it
--
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
Re: [PR] fix(#5242): Disable noErrorHandler setting for Camel 4.4.0 [camel-k]
Posted by "christophd (via GitHub)" <gi...@apache.org>.
christophd commented on PR #5245:
URL: https://github.com/apache/camel-k/pull/5245#issuecomment-1993857402
@squakez thanks for your review. I moved the code to the error handler trait. please have another look
--
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
Re: [PR] fix(#5242): Disable noErrorHandler setting for Camel 4.4.0 [camel-k]
Posted by "christophd (via GitHub)" <gi...@apache.org>.
christophd commented on code in PR #5245:
URL: https://github.com/apache/camel-k/pull/5245#discussion_r1522788618
##########
pkg/controller/pipe/integration.go:
##########
@@ -210,6 +218,27 @@ func CreateIntegrationFor(ctx context.Context, c client.Client, binding *v1.Pipe
return &it, nil
}
+// shouldHandleNoErrorHandler determines the runtime version and checks on noErrorHandler that is configured for this version.
+func shouldHandleNoErrorHandler(ctx context.Context, c client.Client, it *v1.Integration) bool {
+ var runtimeVersion string
+ if it.Spec.Traits.Camel != nil && it.Spec.Traits.Camel.RuntimeVersion != "" {
+ runtimeVersion = it.Spec.Traits.Camel.RuntimeVersion
+ } else if pl, err := platform.GetForResource(ctx, c, it); err == nil && pl != nil {
Review Comment:
yes moved code to trait, thanks!
--
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
Re: [PR] fix(#5242): Disable noErrorHandler setting for Camel 4.4.0 [camel-k]
Posted by "christophd (via GitHub)" <gi...@apache.org>.
christophd commented on code in PR #5245:
URL: https://github.com/apache/camel-k/pull/5245#discussion_r1522786591
##########
e2e/common/misc/pipe_test.go:
##########
@@ -63,7 +63,7 @@ func TestPipe(t *testing.T) {
"--name", "throw-error-binding",
).Execute()).To(Succeed())
- g.Eventually(IntegrationPodPhase(t, ns, "throw-error-binding"), TestTimeoutMedium).Should(Equal(corev1.PodRunning))
+ g.Eventually(IntegrationPodPhase(t, ns, "throw-error-binding"), TestTimeoutLong).Should(Equal(corev1.PodRunning))
Review Comment:
yes, I think most of the tests use the long timeout when waiting for the pod running state
--
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
Re: [PR] fix(#5242): Disable noErrorHandler setting for Camel 4.4.0 [camel-k]
Posted by "lburgazzoli (via GitHub)" <gi...@apache.org>.
lburgazzoli commented on PR #5245:
URL: https://github.com/apache/camel-k/pull/5245#issuecomment-1997587158
my workaround-ish feeling would lead to register a Kamelet component in the `camel-k-runtime` with the `noErrorHandler` set to false so it would apply only to specific versions. This could probably buy some time to fix the Camel YAML DSL and reason about how this should be better solved, tough, I do not hold any strong opinion, 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
Re: [PR] fix(#5242): Disable noErrorHandler setting for Camel 4.4.0 [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on code in PR #5245:
URL: https://github.com/apache/camel-k/pull/5245#discussion_r1522730782
##########
e2e/common/misc/pipe_test.go:
##########
@@ -63,7 +63,7 @@ func TestPipe(t *testing.T) {
"--name", "throw-error-binding",
).Execute()).To(Succeed())
- g.Eventually(IntegrationPodPhase(t, ns, "throw-error-binding"), TestTimeoutMedium).Should(Equal(corev1.PodRunning))
+ g.Eventually(IntegrationPodPhase(t, ns, "throw-error-binding"), TestTimeoutLong).Should(Equal(corev1.PodRunning))
Review Comment:
Weird. We should investigate why a simple timer-to-log pipe is taking so long then, I don't think this is happening within the rest of tests, is it?
--
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
Re: [PR] fix(#5242): Disable noErrorHandler setting for Camel 4.4.0 [camel-k]
Posted by "claudio4j (via GitHub)" <gi...@apache.org>.
claudio4j commented on code in PR #5245:
URL: https://github.com/apache/camel-k/pull/5245#discussion_r1522896048
##########
pkg/trait/error_handler.go:
##########
@@ -107,3 +120,17 @@ func (t *errorHandlerTrait) addGlobalErrorHandlerAsSource(e *Environment) error
return nil
}
+
+// shouldHandleNoErrorHandler determines the runtime version and checks on noErrorHandler that is configured for this version.
+func shouldHandleNoErrorHandler(it *v1.Integration) bool {
+ if it.Status.RuntimeVersion != "" {
+ runtimeVersion, _ := strings.CutSuffix(it.Status.RuntimeVersion, "-SNAPSHOT")
+ if versionNumber, err := strconv.Atoi(strings.ReplaceAll(runtimeVersion, ".", "")); err == nil {
Review Comment:
The downstream version is something like "3.2.0.redhat-00014", I didn't check this function, but I remember having issues parsing the downstream version, so just a reminder to take this into account. We can also add this specific to the downstream fork if needed.
--
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
Re: [PR] fix(#5242): Disable noErrorHandler setting for Camel 4.4.0 [camel-k]
Posted by "lburgazzoli (via GitHub)" <gi...@apache.org>.
lburgazzoli commented on PR #5245:
URL: https://github.com/apache/camel-k/pull/5245#issuecomment-1997061272
@christophd following the discussion in https://github.com/apache/camel-k/issues/5242, do we still need the property ?
--
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
Re: [PR] fix(#5242): Disable noErrorHandler setting for Camel 4.4.0 [camel-k]
Posted by "christophd (via GitHub)" <gi...@apache.org>.
christophd commented on PR #5245:
URL: https://github.com/apache/camel-k/pull/5245#issuecomment-1997674194
@lburgazzoli if this is better received than the changes in this PR I am fine with it. It still is a Camel K specific workaround though
--
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
Re: [PR] fix(#5242): Disable noErrorHandler setting for Camel 4.4.0 [camel-k]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5245:
URL: https://github.com/apache/camel-k/pull/5245#issuecomment-1993709261
:heavy_check_mark: Unit test coverage report - coverage increased from 37.2% to 37.3% (**+0.1%**)
--
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
Re: [PR] fix(#5242): Disable noErrorHandler setting for Camel 4.4.0 [camel-k]
Posted by "christophd (via GitHub)" <gi...@apache.org>.
christophd commented on code in PR #5245:
URL: https://github.com/apache/camel-k/pull/5245#discussion_r1522782774
##########
pkg/controller/pipe/integration.go:
##########
@@ -210,6 +218,27 @@ func CreateIntegrationFor(ctx context.Context, c client.Client, binding *v1.Pipe
return &it, nil
}
+// shouldHandleNoErrorHandler determines the runtime version and checks on noErrorHandler that is configured for this version.
+func shouldHandleNoErrorHandler(ctx context.Context, c client.Client, it *v1.Integration) bool {
+ var runtimeVersion string
+ if it.Spec.Traits.Camel != nil && it.Spec.Traits.Camel.RuntimeVersion != "" {
+ runtimeVersion = it.Spec.Traits.Camel.RuntimeVersion
+ } else if pl, err := platform.GetForResource(ctx, c, it); err == nil && pl != nil {
+ runtimeVersion = pl.Status.Build.RuntimeVersion
+ }
+
+ if runtimeVersion != "" {
+ runtimeVersion, _ = strings.CutSuffix(runtimeVersion, "-SNAPSHOT")
+ if versionNumber, err := strconv.Atoi(strings.ReplaceAll(runtimeVersion, ".", "")); err == nil {
Review Comment:
Should be handled by the string base comparison - will add a unit test ...
--
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
Re: [PR] fix(#5242): Disable noErrorHandler setting for Camel 4.4.0 [camel-k]
Posted by "lburgazzoli (via GitHub)" <gi...@apache.org>.
lburgazzoli commented on PR #5245:
URL: https://github.com/apache/camel-k/pull/5245#issuecomment-1997700464
I think at this stage it would be better but it is gut feeling
--
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
Re: [PR] fix(#5242): Disable noErrorHandler setting for Camel 4.4.0 [camel-k]
Posted by "christophd (via GitHub)" <gi...@apache.org>.
christophd closed pull request #5245: fix(#5242): Disable noErrorHandler setting for Camel 4.4.0
URL: https://github.com/apache/camel-k/pull/5245
--
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
Re: [PR] fix(#5242): Disable noErrorHandler setting for Camel 4.4.0 [camel-k]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5245:
URL: https://github.com/apache/camel-k/pull/5245#issuecomment-1995291920
:heavy_check_mark: Unit test coverage report - coverage increased from 37.2% to 37.3% (**+0.1%**)
--
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
Re: [PR] fix(#5242): Disable noErrorHandler setting for Camel 4.4.0 [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on code in PR #5245:
URL: https://github.com/apache/camel-k/pull/5245#discussion_r1522642565
##########
e2e/common/misc/pipe_test.go:
##########
@@ -63,7 +63,7 @@ func TestPipe(t *testing.T) {
"--name", "throw-error-binding",
).Execute()).To(Succeed())
- g.Eventually(IntegrationPodPhase(t, ns, "throw-error-binding"), TestTimeoutMedium).Should(Equal(corev1.PodRunning))
+ g.Eventually(IntegrationPodPhase(t, ns, "throw-error-binding"), TestTimeoutLong).Should(Equal(corev1.PodRunning))
Review Comment:
I moved them to timeout medium on purpose. There's no need to wait 15 minutes to get a failure there. With 5 minutes we give enough time to try the execution.
--
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
Re: [PR] fix(#5242): Disable noErrorHandler setting for Camel 4.4.0 [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on code in PR #5245:
URL: https://github.com/apache/camel-k/pull/5245#discussion_r1522838711
##########
e2e/common/misc/pipe_test.go:
##########
@@ -63,7 +63,7 @@ func TestPipe(t *testing.T) {
"--name", "throw-error-binding",
).Execute()).To(Succeed())
- g.Eventually(IntegrationPodPhase(t, ns, "throw-error-binding"), TestTimeoutMedium).Should(Equal(corev1.PodRunning))
+ g.Eventually(IntegrationPodPhase(t, ns, "throw-error-binding"), TestTimeoutLong).Should(Equal(corev1.PodRunning))
Review Comment:
:worried:
--
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
Re: [PR] fix(#5242): Disable noErrorHandler setting for Camel 4.4.0 [camel-k]
Posted by "christophd (via GitHub)" <gi...@apache.org>.
christophd commented on PR #5245:
URL: https://github.com/apache/camel-k/pull/5245#issuecomment-1997079362
@lburgazzoli I am currently trying to avoid the property right now and set the error handler ref directly on the route as Camel JBang would do. Unfortunately we would end up having both global error handler and route based error handler both referencing the same error handler. This is because we would like to support both Camel < 4.4.0 and 4.4.0+
--
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
Re: [PR] fix(#5242): Disable noErrorHandler setting for Camel 4.4.0 [camel-k]
Posted by "christophd (via GitHub)" <gi...@apache.org>.
christophd commented on PR #5245:
URL: https://github.com/apache/camel-k/pull/5245#issuecomment-1997209935
> is there anything we can do in the camel-k runtime maybe ? so in fact the operator won't have to change.
I try to explain my findings:
So the proper fix in my eyes would be to set the error handler on the route (just like Camel JBang does) instead of adding it as a global error handler (this is what the Camel K operator does). This is because since Camel 4.4 the raised error is handled by the NoErrorHandler on the route by default. So we need to set the error handler on the route when running with Camel 4.4 onwards.
But unfortunately the Camel YAML DSL won't let me do this as it only supports to add the error handler as a global one on the Camel context. So we would need to change this on the Camel YAML DSL first and then let the operator set the error handler on the route.
An alternative to all that would be to set this `camel.component.kamelet.noErrorHandler=false` property on the `application.properties` which makes Camel 4.4 behave like it was before and the global error handler will handle the errors.
Unfortunately Camel K operator is keen on supporting both runtimes Camel < 4.4.0 and 4.4.0+. So setting the property or adding the error handler to the route instead of global is dependent on the runtime version used to run the integration.
Now you may give advice what and where to handle this. In the operator or in camel-k runtime layer.
--
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