-
Notifications
You must be signed in to change notification settings - Fork 103
Environment configuration #895
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?
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.
Make sure you get rid of those .DS_Store macOS file nonsense. They should be globally excluded by gitignore.
I dunno if we need to do this in this PR, we can wait and see what @cretz and @dandavison opinions are too, but IMO it would make good sense to add a Client.connect()
overload that takes a ClientConfigProfile
here for easy use.
Also these guys need experimental notices on them too
temporalio/envconfig.py
Outdated
"""Environment and file-based configuration for Temporal clients. | ||
|
||
This module provides utilities to load Temporal client configuration from TOML files | ||
and environment variables, following the same patterns as the Go SDK. |
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.
and environment variables, following the same patterns as the Go SDK. | |
and environment variables. |
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.
removed
temporalio/envconfig.py
Outdated
|
||
|
||
@dataclass(frozen=True) | ||
class _DataSource: |
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 shouldn't be private, since it exists as a field in public things. I think probably we don't really need this, and it could just be a Union[str, bytes]
. Maybe a type alias for that, so it can have a comment explaining str is path and bytes is data.
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.
changed to a union type, kept the methods as private helpers
temporalio/envconfig.py
Outdated
# Create a dictionary of kwargs for ConnectConfig | ||
kwargs: dict[str, Any] = {"api_key": self.api_key} | ||
|
||
# Target host |
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.
Boring comments
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.
removed
Whoops, didn't realize I had committed them. Removed, added to gitignore
seems reasonable, will wait to chad gets back as i think he'll have more to suggestions anyway
added to each class (not sure if each method is necessary?) |
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 looking good to me, but have Dan or Chad when he's back take a look
What was changed
Environment configuration for Python SDK, using core base envconfig module
Closes [Feature Request] Environment configuration #835
How was this tested:
test_envconfig.py
Any docs updates needed?
probably