flowchartsman commented on PR #1077: URL: https://github.com/apache/pulsar-client-go/pull/1077#issuecomment-1678429073
As an administrative note now that we have an actual PR, I am prepped to replay the changes from streamnative/pulsar-admin-go#41 on top of these changes to refactor the API, either as a PR on [this branch](tisonkun:adopt-pulsar-admin-go) branch, against master, or as a separate PR, whichever is most appropriate. See #1075 for context. I would then begin the work of merging auth functionality and creating an admin client type. This would remove the `pulsaradmin.Client` alias, replacing it with either an interface or a concrete type which would return concrete types representing the different administrative APIs currently modeled as interfaces (see [`FunctionsWorker` here](https://github.com/apache/pulsar-client-go/pull/1077/files#diff-77a7be6f50bcdbe42702153ff4b70c7cc136540832f57ed44c6b503780a25327R24-R39)) Thus, ```go type Client interface { Functions() Functions // ... } ``` would become: ```go type Client interface { Functions() *FunctionsWorker // ... } // or type Client struct{ // internal fields Functions *FunctionsWorker //etc } ``` I lean towards the concrete type if I'm being honest, since the construction of calling `client.Functions().Somemethod()` is awkward and making fat interfaces and `_impl` types is a bit of a Go antipattern, IMHO. The concrete client type can be tested using integration testing, since it consists almost entirely of HTTP calls, and users that wish to mock functionality in their code can simply make their own, smaller interfaces using just the functionality they need, such as: ```go type MyFunctionMaintainer struct { functionAPI functionAPI } type functionAPI interface { CreateFuncWithURL(data *utils.FunctionConfig, pkgURL string) error GetFunction(tenant, namespace, name string) (utils.FunctionConfig, error) UpdateFunction(functionConfig *utils.FunctionConfig, fileName string, updateOptions *utils.UpdateOptions) error DeleteFunction(tenant, namespace, name string) error } ``` A further PR could integrate the admin client into the normal pulsar tests, providing a single place to access the integration container, removing some of the awkward constructions used in #1076 and using the test admin client as the single point of access to the integration container. Which would be awesome :) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
