-
Notifications
You must be signed in to change notification settings - Fork 107
Plugin Support #952
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
base: main
Are you sure you want to change the base?
Plugin Support #952
Conversation
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.
This is great.
I spent some time seeing if we could improve any of the names in the interface.
What I'd like to suggest there is: instead of on_create_client
and on_create_worker
, how about
def client_config(self, config: ClientConfig) -> ClientConfig
def worker_config(self, config: WorkerConfig) -> WorkerConfig
An alternative that was mentioned on slack was configure_client()
/ configure_worker()
. That could work too, but it kind of suggests that the method gets to instantiate or mutate a client/worker. I like the simplicity of client_config(self, config: ClientConfig) -> ClientConfig
: it makes it clear that your job in that method is to return the config you want, nothing less and nothing more.
on_create_client
and on_create_worker
don't work as well because their names suggest an event that occurs after or during instantiation of the client/worker, which is awkward seeing as the method is a pure transformation of a config struct and so is clearly happening before instantiation.
Other than that, it took me a bit of time to understand why the multiple inheritance made sense, and why it made sense to expose the "service client" rather than just "connect client", but I think this all makes sense and is very natural given the SDK's existing public interceptor API.
Overall agree w/ @dandavison
I think it suggests the method gets to configure the client/worker, not instantiate or mutate it. I think it's clearer than the getter-looking ones that are missing the verb. |
Yes I think I've changed my mind, I prefer these also. |
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, nothing blocking
What was changed
Added a new
Plugin
concept for configuring clients and workersWhy?
Many users need to do a common set of configurations across multiple locations, or provide such a configuration set to others.
Checklist
Closes [Feature Request] Plugins to support controlling multiple configuration points at once #950
How was this tested:
New set of tests in
test_plugins.py
Any docs updates needed?
Readme is updated, docs will be needed.