You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "tisonkun (via GitHub)" <gi...@apache.org> on 2023/08/14 03:37:23 UTC
[GitHub] [pulsar-client-go] tisonkun opened a new pull request, #1077: Adopt pulsar admin go
tisonkun opened a new pull request, #1077:
URL: https://github.com/apache/pulsar-client-go/pull/1077
This refers to https://github.com/apache/pulsar-client-go/issues/1075.
Roughly add source code with ASF header without any nontrivial changes.
cc @flowchartsman @Gleiphir2769 @gunli
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] tisonkun merged pull request #1077: ci: replace license header checker and formatter
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun merged PR #1077:
URL: https://github.com/apache/pulsar-client-go/pull/1077
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] tisonkun commented on pull request #1077: Adopt pulsar admin go
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on PR #1077:
URL: https://github.com/apache/pulsar-client-go/pull/1077#issuecomment-1679098161
> Can this PR include the pulsar-go-admin commit?
That's possible. Let me prepare a patch for this requirement. We should be able to use git filter-repo/filter-branch for such case.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] flowchartsman commented on pull request #1077: Adopt pulsar admin go
Posted by "flowchartsman (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] tisonkun commented on pull request #1077: Adopt pulsar admin go
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on PR #1077:
URL: https://github.com/apache/pulsar-client-go/pull/1077#issuecomment-1679113346
I'd minimize the changeset to replace the license checker in this PR and prepare a new one to do the filter-branch work which can be much more complicated.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org