Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ci: replace license header checker and formatter #1077

Merged
merged 5 commits into from
Aug 15, 2023

Conversation

tisonkun
Copy link
Member

This refers to #1075.

Roughly add source code with ASF header without any nontrivial changes.

cc @flowchartsman @Gleiphir2769 @gunli

Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
@flowchartsman
Copy link
Contributor

flowchartsman commented Aug 15, 2023

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 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)

Thus,

type Client interface {
	Functions() Functions
        // ...
}

would become:

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:

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 controlling the spawning of the integration container. Which would be awesome :)

Copy link
Member

@nodece nodece left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Can this PR include the pulsar-go-admin commit?

@tisonkun
Copy link
Member Author

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.

@tisonkun
Copy link
Member Author

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.

Signed-off-by: tison <wander4096@gmail.com>
@tisonkun tisonkun changed the title Adopt pulsar admin go ci: replace license header checker and formatter Aug 15, 2023
@tisonkun tisonkun merged commit 8828c01 into apache:master Aug 15, 2023
6 checks passed
@tisonkun tisonkun deleted the adopt-pulsar-admin-go branch August 15, 2023 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants