Allow K8s Services to be Subscription's Call. #489
Conversation
|
/assign @vaikas-google |
knative-prow-robot
added
the
size/L
|
Generally looks good to me, but I'd rather disable the test than enforce a known wrong result. |
| return "", err | ||
| } | ||
|
|
||
| if t.Status.Targetable != nil { | ||
| return t.Status.Targetable.DomainInternal, nil | ||
| } | ||
| return "", fmt.Errorf("status does not contain targetable") | ||
|
|
||
| // K8s Services are special cased. |
There was a problem hiding this comment.
If we are going to special case them and need to check for them like so below, I think my preference would be to just fetch them directly as Service objects above Line 171, so see if the reference is to a Service object and if so fetch it as such. Rather than pretend they are duck types.
@scothis In general I agree. However I am following the pattern already present in the file. And even this test with the known wrong result has shown bugs in my implementation (got the wrong error message), so has been useful. I'll leave it to @vaikas-google to decide if he wants the test disabled or enforce this known wrong result. |
|
/retest |
| @@ -178,7 +194,7 @@ func (r *reconciler) resolveCall(namespace string, callable v1alpha1.Callable) ( | |||
| //t := duckv1alpha1.Target{} | |||
There was a problem hiding this comment.
not your doing (mine), but I neglected to to remove this TODO even though I already did this. Could you remove L193,194.
| getK8sService(), | ||
| }, | ||
| ReconcileKey: fmt.Sprintf("%s/%s", testNS, subscriptionName), | ||
| // TODO: JSON patch is not working for some reason. Is this the array vs. non-array, or |
There was a problem hiding this comment.
I figured out what the problem is a few days ago, if you could update these comments with a pointer to this bug:
kubernetes/client-go#478
Then I'd rather leave them here as is for the reasons you already mentioned, it does do quite a few tests and covers most of the code.
|
The following is the coverage report on pkg/.
|
|
/lgtm |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adamharwayne, vaikas-google 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 |
knative-prow-robot
added
the
approved
|
Retrying the test, failed with: |
|
/retest |
Proposed Changes
callto call K8s Services.Release Note