default profile fallback to localhost and 'default' namespace #968
Conversation
|
|
||
| impl ClientConfigProfile { | ||
| /// Returns a client config profile with default values for connecting to localhost. | ||
| pub fn default_localhost_profile() -> Self { |
There was a problem hiding this comment.
I think typically you would provide an impl for default instead.
There was a problem hiding this comment.
i was unsure if providing an impl for default would be unclear that it's providing a value for address instead of None
There was a problem hiding this comment.
I would say that if we are changing every default call to this, then potentially that means the existing derived default doesn't make sense and we should remove the derive and implement a different default. If that's not the case (it's just what it looked like), then I think you are right. Either way, not that important.
There was a problem hiding this comment.
I think it's reasonable for this to just be the Default impl. You can update the docstring for the struct.
| address: Some("localhost:7233".to_string()), | ||
| namespace: Some("default".to_string()), |
There was a problem hiding this comment.
I'm think this should be done lang side specifically when loading connect options only. The client config should only be what is loaded, not with defaults.
Can you show me sample Python code of loading a config from toml, making a change, and then serializing it to toml again? Will that save now put the address and namespace? Or anything else? It is a very legitimate use case to make "load client config" calls just to see what you literally have in your client config. People should be allowed to know whether they've configured address/namespace in their environment or not if they want to know that.
(also, how do I disable using environment variables when loading, is the caller expected to use empty hash map?)
There was a problem hiding this comment.
... Python code of loading a config from toml, making a change, and then serializing it to toml again? Will that save now put the address and namespace?
Yes
I'm think this should be done lang side specifically when loading connect options only. The client config should only be what is loaded, not with defaults.
Yeah I think that's valid. I'll close this PR and make the corresponding change in temporalio/sdk-python#1004
|
Making the change lang-side |
What was changed
Default profiles fallback to
localhost:7233address anddefaultnamespaceWhy?
Allows users to load configs from lang-SDKs without having to provide any configuration
How was this tested:
Couple small integration tests
Any docs updates needed?
No, this creates parity with the docs