-
Notifications
You must be signed in to change notification settings - Fork 95
feat: Expose envconfig functionality through C bridge #986
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: master
Are you sure you want to change the base?
Conversation
240b196
to
9a9cbe7
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.
Minor suggestions. Also, can we submit use of this (which I am guessing is at temporalio/sdk-dotnet#509) for PR review before merging this just to make sure we're good there from a review POV? We can basically merge both at once when both approved (obviously this first and submodule update over there).
|
||
/// Load a single client profile from given sources with env overrides | ||
#[unsafe(no_mangle)] | ||
pub extern "C" fn temporal_core_client_config_profile_load( |
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.
Can you be a bit more specific in docs on what format the callback receives on success and fail? They are just byte arrays, so we can be a bit clearer. Can do this in the callback docs instead of here if easier.
|
||
/// Load a single client profile from given sources with env overrides | ||
#[unsafe(no_mangle)] | ||
pub extern "C" fn temporal_core_client_config_profile_load( |
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.
There are several parameters here. I think it'd be ok if we accepted a ClientConfigProfileLoadOptions
struct with all of this in it instead. Similarly, would have a ClientConfigLoadOptions
for the other exposed function.
|
||
/// Load all client profiles from given sources | ||
#[unsafe(no_mangle)] | ||
pub extern "C" fn temporal_core_client_config_load( |
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.
Was reviewing .NET and noticed - this doesn't need to be async w/ a callback. Can just return the result or failure. Can see calls like temporal_core_runtime_new
or temporal_core_worker_new
to see how they return success or fail.
What was changed
C bridge code for .NET env config
Part of [Feature Request] Environment Configuration sdk-dotnet#490
How was this tested:
Integration tests lang-side
Any docs updates needed?
No