More events!#2438
Conversation
For Config and Route creation.
These had two different implementation styles, one of which emitted events. Changed all of them to share the same style.
knative-prow-robot
requested review from
dprotaso,
josephburnett,
lichuqiang and
tcnghia
knative-prow-robot
added
the
size/L
knative-prow-robot
left a comment
knative-prow-robot
left a comment
There was a problem hiding this comment.
@jonjohnsonjr: 0 warnings.
Details
In response to this:
- Add Route/Config creation events to Service.
- Make
updateStatusimplementations consistent, which makes them emit events. (We might not want this...)Fixes #1133
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
|
/assign @mattmoor |
| return nil, err | ||
| } | ||
|
|
||
| c.Recorder.Eventf(desired, corev1.EventTypeNormal, "Updated", "Updated status for KPA %q", desired.Name) |
There was a problem hiding this comment.
I'm not convinced of the usefulness of this event... In some cases we update status a fair amount before we're done, which will mean a lot of events.
There was a problem hiding this comment.
For KPA specifically or for our CRDs in general?
There was a problem hiding this comment.
I'm also concerned that we may get flooded by unrelated events.
But on the other hand, I feel it acceptable to add first, and do subtraction later if we indeed find annoying events later.
There was a problem hiding this comment.
Generally I don't find it useful to surface an event that just says: "I updated status!" without what that update is.
I feel like this could be made useful by surfacing events reflecting changes in our conditions (vs. existing), especially with #2436 giving a hit for the level of event we should be surfacing (on False).
I'd exclude this from all resource types for now, and we can open an issue (for followup) that takes:
(r Recorder, obj runtime.Object, before duckv1alpha1.Conditions, after duckv1alpha1.Conditions) -> error
We should be able to take a generic delta of the Conditions and surface events through the Recorder for transitions.
A route test was expecting a status update event.
|
The following is the coverage report on pkg/.
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jonjohnsonjr, mattmoor The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow-robot
added
the
approved
5 participants
updateStatusimplementations consistent, which makes them emit events on failure.Fixes #1133