-
Notifications
You must be signed in to change notification settings - Fork 367
feat: add WithContext() admin client methods #1425
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
feat: add WithContext() admin client methods #1425
Conversation
14825d1
to
9c7be61
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds context support to the Pulsar admin client by introducing WithContext()
methods for all admin operations. This enables proper request timeout control and context-based cancellation following Go idioms.
Key changes:
- Adds context parameter to all HTTP client methods in the REST client
- Introduces
WithContext()
equivalent methods for all admin interfaces - Updates existing methods to call the new context-enabled methods with
context.Background()
Reviewed Changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 6 comments.
Show a summary per file
File | Description |
---|---|
pulsaradmin/pkg/rest/client.go | Modified core HTTP client to accept context parameter in all request methods |
pulsaradmin/pkg/admin/tenant.go | Added WithContext() methods for tenant operations |
pulsaradmin/pkg/admin/subscription.go | Added context support for subscription management operations |
pulsaradmin/pkg/admin/sources.go | Added context-enabled methods for source connector operations |
pulsaradmin/pkg/admin/sinks.go | Added context support for sink connector management |
pulsaradmin/pkg/admin/schema.go | Added WithContext() methods for schema operations |
pulsaradmin/pkg/admin/resource_quotas.go | Added context support for resource quota management |
pulsaradmin/pkg/admin/packages.go | Added context-enabled methods for package operations |
pulsaradmin/pkg/admin/ns_isolation_policy.go | Added context support for namespace isolation policy operations |
pulsaradmin/pkg/admin/namespace.go | Added WithContext() methods for extensive namespace operations |
pulsaradmin/pkg/admin/functions_worker.go | Added context support for function worker operations |
pulsaradmin/pkg/admin/functions.go | Added context-enabled methods for function management |
pulsaradmin/pkg/admin/cluster.go | Added WithContext() methods for cluster operations |
pulsaradmin/pkg/admin/brokers_test.go | Updated test to use context-enabled method |
pulsaradmin/pkg/admin/brokers.go | Added context support for broker operations |
pulsaradmin/pkg/admin/broker_stats.go | Added WithContext() methods for broker statistics |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, please fix the CI check.
Just fixed the last linter issues, I was not getting these locally with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
TODO: Marking the old method as deprecated is a better approach. |
Motivation
The admin client does not carry / accept the context
context.Context
. Accepting a context as a first parameter is a Go idiom, and can be useful when the caller wants to set a request timeout instead of tracking manually a timeout with separate goroutine.Modifications
In order to accept that context and make this a non-breaking change, a
WithContext()
method equivalent is added to each of the methods in the admin client. Existing methods are now calling internally theWithContext()
method, with acontext.Background()
for the required context.Verifying this change
This change can be verified with existing E2E tests on the admin client for regression.
Does this pull request potentially affect one of the following parts:
Documentation