Add data validation for subscription in reconciler#1100
Conversation
knative-prow-robot
added
the
size/S
|
|
||
| subscriberGVK = metav1.GroupVersionKind{ | ||
| Group: "testing.eventing.knative.dev", | ||
| Group: "eventing.knative.dev", |
There was a problem hiding this comment.
this is testing ducktyping for subscribers. testing. was wanted.
There was a problem hiding this comment.
I changed that because in subscriber validation there is hardcoded logic to validate API group is eventing.knative.dev
| subscription.Status.InitializeConditions() | ||
|
|
||
| if err := subscription.Validate(ctx); err != nil { | ||
| // Verify subscription is valid |
There was a problem hiding this comment.
Can you move this comment to the other side of the if statement and end it with a .
| return resolve.DomainToURL(s.Status.Address.Hostname), nil | ||
| } | ||
| return "", fmt.Errorf("status does not contain address") | ||
| return "", fmt.Errorf("reply.status does not contain address") |
| WantEvents: []string{ | ||
| Eventf(corev1.EventTypeWarning, "ResultResolveFailed", "Failed to resolve spec.reply: status does not contain address"), | ||
| Eventf(corev1.EventTypeWarning, "SubscriptionUpdateStatusFailed", "Failed to update Subscription's status: status does not contain address"), // TODO: BUGBUG THIS IS WEIRD | ||
| Eventf(corev1.EventTypeWarning, "SubscriptionUpdateStatusFailed", "Failed to update Subscription's status: invalid value: Subscriber: spec.reply.kind\nonly 'Channel' kind is allowed"), // TODO: BUGBUG THIS IS WEIRD |
There was a problem hiding this comment.
you can remove the todo at the end.
| WithSubscriptionReply(subscriberGVK, replyName), | ||
| // The first reconciliation will initialize the status conditions. | ||
| WithInitSubscriptionConditions, | ||
| WithSubscriptionPhysicalSubscriptionSubscriber(subscriberURI), |
There was a problem hiding this comment.
interesting, I suppose this is because you are validating. NICE.
Fixes knative#1081 - Update subscription error when reply strategy does not contain address
|
@n3wscott : I have addressed all your comments |
|
The following is the coverage report on pkg/.
|
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: n3wscott, shashwathi 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
Fixes #1081
cc @n3wscott