[issue-521] Enhance error messages in SonataFlowPlatform CR status for knative integration #558
Conversation
jianrongzhang89
force-pushed
the
error-message
branch
3 times, most recently
from
293032b to
ce270cd
Compare
There was a problem hiding this comment.
@jianrongzhang89 @ricardozanini
A minor non-blocking suggestion from my side guys if we have time.
internal/controller/platform/k8s.go
Outdated
| } | ||
| if !kSinkInjected { | ||
| return fmt.Errorf("waiting for K_SINK injection for %s to complete", psh.GetServiceName()) | ||
| msg := fmt.Sprintf("waiting for K_SINK injection for %s to complete", psh.GetServiceName()) |
There was a problem hiding this comment.
This is good, we can see the service name in the event.
| event := &corev1.Event{ | ||
| Type: corev1.EventTypeWarning, | ||
| Reason: WaitingKnativeEventing, | ||
| Message: err.Error(), |
There was a problem hiding this comment.
For this error message it would be nice if we can see the service name as part of the message.
I have forced the use case to happen, and in the events I can see:
5m47s Warning WaitingKnativeEventing sonataflowplatform/sonataflow-platform broker sonataflow-broker in namespace case1-kn-eventing does not exist
13m Warning Failed sonataflowplatform/sonataflow-platform Failed to update SonataFlowPlaform: broker sonataflow-broker in namespace case1-kn-
So, it's difficult to distinguish for which service we have the missing broker.
If it's not too much work, it would suggest that we include the name of the service as part of the message.
I think we can just grab the ValidatBroker message and concatenate "service" + d.GetServiceName()
and we can see:
sonataflow-broker in namespace case1-kn-eventing does not exist for service: sonataflow-platform-data-index
Considering that the configuration alternatives admits different Brokers for DI and JS, I think it'll be very helpful in situations where users needs to query the events to find the cause of the issue
There was a problem hiding this comment.
Same for the jobs-service knative addings
There was a problem hiding this comment.
Thank you @wmedvede for the suggestion. This sounds good to me. Code is updated.
…r knative integration
jianrongzhang89
force-pushed
the
error-message
branch
from
ce270cd to
2c0a8b2
Compare
…r knative integration (apache#558)
Description of the change:
Fix #521.
Note that we have decided to use K8s events instead of status conditions to notify the user that the broker is not available.
Motivation for the change:
To facilitate troubleshooting of configuration issues.
Checklist