set trigger status appropriately if they do not have broker #3144
Conversation
knative-prow-robot
added
size/L
| // If shouldLabelNamespace is set to true this test annotates the testing namespace so that a default broker is created. | ||
| // It then binds many triggers with different filtering patterns to the broker created by brokerCreator, and sends | ||
| // different events to the broker's address. | ||
| // Finally, it verifies that only the appropriate events are routed to the subscribers. |
There was a problem hiding this comment.
It looks different than this 😄
There was a problem hiding this comment.
lol, yeah, thanks, fixed :)
| return false, err | ||
| } | ||
| if ready := trigger.Status.GetTopLevelCondition(); ready != nil { | ||
| if ready.Status == corev1.ConditionFalse && ready.Reason == "BrokerDoesNotExist" { |
There was a problem hiding this comment.
If the reason is passed as a parameter this test is a valid conformance test for the control-plane conformance.
There was a problem hiding this comment.
You mean, we could run multiple tests validations for failures? Yes, good point. I'm down to make that change, but I'd prefer to do that in a followup to make sure we get this error case handled into the release :)
There was a problem hiding this comment.
I mean, the broker conformance spec doesn't force using BrokerDoesNotExist as a reason for the trigger to not become ready if there is no broker;
if the test has the reason as param implementors can reuse this test without changing the reason they're using (maybe they're using BrokerNotFound rather than BrokerDoesNotExist).
There was a problem hiding this comment.
Yep, that makes sense. As I mentioned above I'm all for it, but would prefer to do that in a followup? Ok with you?
pkg/reconciler/trigger/trigger.go
Outdated
| // it still would not get reconciled. | ||
| tt := t.DeepCopy() | ||
| tt.Status.MarkBrokerFailed("BrokerDoesNotExist", "Broker %q does not exist or there is no matching BrokerClass for it", tt.Spec.Broker) | ||
| _, te := r.eventingClientSet.EventingV1beta1().Triggers(tt.Namespace).UpdateStatus(tt) |
There was a problem hiding this comment.
I don't think you want to call update status here, that should be done in the calling method right?
There was a problem hiding this comment.
Of course :( Thanks!!
|
The following is the coverage report on the affected files.
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: n3wscott, vaikas The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Fixes #2996
Proposed Changes
Release Note
Docs