Adding a fake broker server#928
Conversation
This is a continuation of kubernetes-retired#533, and is a pre-requisite for kubernetes-retired#923
k8s-ci-robot
added
the
cncf-cla: yes
| version: 9497139cb62015905ba5b3d11836f2b0c117ff80 | ||
| subpackages: | ||
| - pkg/api | ||
| - pkg/api/install |
There was a problem hiding this comment.
this and the below were added because the new brokerapi package was added with glide get, which does a full update
| ) | ||
|
|
||
| // ConvertCatalog converts a (github.com/kubernetes-incubator/service-catalog/pkg/brokerapi).Catalog | ||
| // to an array of brokerapi.Services |
There was a problem hiding this comment.
given the overloaded "brokerapi" it might be good to be explicit in the "to" like you are in the "from". Meaning
say it's from pivotal's brokersapi
There was a problem hiding this comment.
I keep having to stop and think way too much about which brokerapi is which, up to you, but it might be nice to make ours "svccat" instead of "pkgbrokerapi" - so people don't need to think that much.
| // Handler is a fake implementation oif a brokerapi.ServiceBroker | ||
| type Handler struct { | ||
| Catalog []brokerapi.Service | ||
| CatalogRequests int |
There was a problem hiding this comment.
Can you add a comment to explain why this is just an "int" while the other ones are slices of structs?
Just something like:
// Since there is no incoming data to save (unlike the other APIs) we just keep a count
// of the requests.
| return &Handler{} | ||
| } | ||
|
|
||
| // Services is the interface implementation of brokerapi.ServiceBroker |
There was a problem hiding this comment.
// Returns the list of available Services (ie. the Catalog), and
// tracks how many times its been called.
| return h.Catalog | ||
| } | ||
|
|
||
| // Provision is the interface implementation of brokerapi.ServiceBroker |
There was a problem hiding this comment.
// Save incoming provision request and return canned response
| return h.ProvisionResp, h.ProvisionRespError | ||
| } | ||
|
|
||
| // Deprovision is the interface implementation of brokerapi.ServiceBroker |
There was a problem hiding this comment.
// Save incoming request and return canned response
| return h.DeprovisionResp, h.DeprovisonRespErr | ||
| } | ||
|
|
||
| // Bind is the interface implementation of brokerapi.ServiceBroker |
There was a problem hiding this comment.
// Save incoming request and return canned response
| return h.BindResp, h.BindRespErr | ||
| } | ||
|
|
||
| // Unbind is the interface implementation of brokerapi.ServiceBroker |
There was a problem hiding this comment.
// Save incoming request and return canned response
| return h.UnbindRespErr | ||
| } | ||
|
|
||
| // Update is the interface implementation of brokerapi.ServiceBroker |
There was a problem hiding this comment.
// Save incoming request and return canned response
| return h.UpdateResp, h.UpdateRespErr | ||
| } | ||
|
|
||
| // LastOperation is the interface implementation of brokerapi.ServiceBroker |
There was a problem hiding this comment.
// Save incoming request and return canned response
| Details: details, | ||
| AsyncAllowed: asyncAllowed, | ||
| }) | ||
| return h.ProvisionResp, h.ProvisionRespError |
There was a problem hiding this comment.
I don't see where things like ProvisionResp is defined - I'm guessing a follow-on PR will do that and I'm also guessing that it'll be up to the client/tester to do it. If so, would it make sense to have some default logic here so that the client/tester doesn't need to hard code all responses, but instead the fake broker will do what's expected in most cases and the client just needs to provide these canned responses if they want something other than the default/happy-path?
There was a problem hiding this comment.
ProvisionResp is defined in the Handler, and I can't predict what a reasonable default would be. Going forward, if multiple tests continuously define the same mock value (for ProvisionResp or otherwise), we can then put in a default
There was a problem hiding this comment.
I was thinking that the "reasonable defaults" would be based on previous data. For example, we have a canned catalog and someone asks to provision an instance - we know what we should return from the provision request, no? Even the case of the client defining the catalog for us we can still generate an instance and even a binding if a custom one isn't created during the test - I would think.
There was a problem hiding this comment.
@duglin I see what you're suggesting here. That kind of functionality wouldn't be appropriate here because this is a test mock, which exists to help unit tests exercise code-under-test in a controller manner.
It's not appropriate for this mock server to do much more than record request details and regurgitate pre-programmed responses. More advanced servers are more suitable for use in higher-level tests. One could be certainly built using github.com/pivotal-cf/brokerapi package in another PR.
|
definitely like the direction of this - the more real we can make our tests the better. Just a few comments/questions though... |
to eliminate possible ambiguity
|
@duglin changes to docs made, PTAL |
| @@ -0,0 +1,28 @@ | |||
| /* | |||
There was a problem hiding this comment.
What's the reasoning for breaking each type into its own file?
There was a problem hiding this comment.
It's mostly personal preference.
n.b. there are some best practices in Go that support this decision, but there are also conflicting ones too
5 participants
This PR uses
github.com/pivotal-cf/brokerapito implement a fake broker server for use in unit tests. The result is that unit tests can use the broker API client at./pkg/brokerapi/openservicebroker-- which makes real HTTP requests to a broker server -- instead of the fake broker client. The result is that unit tests exercise the real open service broker API client instead of a fake.I've also refactored a single unit test to show how future refactors would work. This patch is a pre-requisite for tests that I'll be writing in #923.
Note: this patch has lots of additions because I vendored in the
github.com/pivotal-cf/brokerapipackage that I mentioned above.cc/ @pmorie @vaikas-google