Allow for http and broker sinks #574
Conversation
knative-prow-robot
added
size/L
There was a problem hiding this comment.
@sixolet: 2 warnings.
In response to this:
Refactors sink code to use a dynamic client to validate the sink, so we don't keep having to write the same parallel code. Paves the way for arbitrary user-configured sinks.
Fixes #570
Also fixes most of #563 : missing arbitrary ns for sinks, but now has the sink as being in the same ns as the object being created.
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.
There was a problem hiding this comment.
Thanks ! Looks good to me, with the usual nits :). We can merge it as it, however, I would maybe model the fake client a bit differently (see comments) to make it more the same like the mock interfaces.
As you mentioned, one remaining problem is how to specify the sink's namespace. IMO this could be either a dedicated option (--sink-namespace) or part of the sink specification, like in svc:mysvc:anotherns. not sure what I like better ;-)
pkg/dynamic/client.go
Outdated
| } | ||
|
|
||
| // CreateFakeKnDynamicClient gives you a dynamic client for testing contianing the given objects. | ||
| func CreateFakeKnDynamicClient(testNamespace string, objects ...runtime.Object) KnDynamicClient { |
There was a problem hiding this comment.
I think having the same mocking scheme that we use for the other clients. Maybe not now, but later.
So something in the lines of
client := NewMockDynamicClient(namespace)
recorder := client.Recorder()
recorder.RegisterGVK(SERVING_GVK)
recorder.RegisterGVK(EVENTING_GVK)
recorder.AddObject(rtObject)
....and then the RawClient() implementation would return a fake client based on the configuration stored in the recorder. Maybe one could also omit the schema recording steps and hardcode them in the recorder.
Would be only a small change but would make it easier to follow the tests as it then would follow the same scheme.
There was a problem hiding this comment.
Also this would decouple test and business code better if we put that method in a different file at least
There was a problem hiding this comment.
I have put the Fake helper method in a different package, even, because you're right it's nice to have test code decoupled from mainline code. On the other hand, I really like the mocking scheme here. Using it was refreshing and much nicer — I would much rather specify the objects in my "fake server" than specify every call to it.. I'll keep this mocking scheme. Might even look to use it in more places.
|
The following is the coverage report on the affected files.
|
| @@ -99,3 +99,13 @@ func TestListSourceTypes(t *testing.T) { | |||
| assert.Equal(t, uList.Items[1].GetName(), "bar") | |||
| }) | |||
| } | |||
|
|
|||
| // createFakeKnDynamicClient gives you a dynamic client for testing contianing the given objects. | |||
There was a problem hiding this comment.
| // createFakeKnDynamicClient gives you a dynamic client for testing contianing the given objects. | |
| // createFakeKnDynamicClient gives you a dynamic client for testing containing the given objects. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: navidshaikh, sixolet 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 |
Refactors sink code to use a dynamic client to validate the sink, so we don't keep having to write the same parallel code. Paves the way for arbitrary user-configured sinks.
Fixes #570
Also fixes most of #563 : missing arbitrary ns for sinks, but now has the sink as being in the same ns as the object being created.