feat: Expose envconfig functionality through C bridge#986
Conversation
THardy98
force-pushed
the
expose_envconfig_c_bridge
branch
from
240b196 to
9a9cbe7
Compare
There was a problem hiding this comment.
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).
- Replace multiple function parameters with ClientConfigLoadOptions and ClientConfigProfileLoadOptions structs - Remove async callbacks and change functions - Improve documentation to clarify byte array formats - Add ClientConfigOrFail and ClientConfigProfileOrFail result structs
THardy98
force-pushed
the
expose_envconfig_c_bridge
branch
from
9a9cbe7 to
614c72e
Compare
|
Let me know if/when the PR is ready for re-review (if you want, you can respond to comments to make it easier to know which are agreed with and/or handled) |
…ameter for ClientConfigLoadOptions
RFR, resolved the above comments |
Sushisource
left a comment
Sushisource
left a comment
There was a problem hiding this comment.
This looks good to me but I'd like to fix that env var test so it doesn't interfere with others
| // WARNING: This test modifies system environment variables which can cause | ||
| // test pollution if tests run in parallel. | ||
|
|
There was a problem hiding this comment.
Yeah we should not do that. I assume you ran into this and it caused a problem somehow? I missed this one when it first when in.
The move here is to spawn off this test into a new process, and you can set the env vars for just that process. The test can early-return unless some special env var is set that lets it know it's been spawned off.
There was a problem hiding this comment.
Hmm the change I made to this test is working locally, but I guess CI may have more restrictions around setting env variables, given that the thread panicked:
thread 'envconfig::tests::test_load_client_config_profile_from
_system_env_impl' panicked at
core-api/src/envconfig.rs:1627:45
the alternative I can think of is to revert the test change and just run it serially
There was a problem hiding this comment.
Didn't realize we run ignored tests in CI, this is panicking because env vars aren't set. Added conditional to skip if env vars aren't set
cretz
left a comment
cretz
left a comment
There was a problem hiding this comment.
LGTM, only the pedantic naming thing (and +1 on not having a side-effecting test)
| * If fail is not null, it contains UTF-8 encoded error message. | ||
| * The returned ByteArrays must be freed by the caller. | ||
| */ | ||
| typedef struct TemporalCoreClientConfigOrFail { |
There was a problem hiding this comment.
Sorry to be pedantic here and sorry I didn't read close enough earlier, but this is not clear it's env config, it makes it seem like it's generic config. Can we change the 4 struct names here to have Env before the word Config? Also can we change the two function names to change config to env_config?
There was a problem hiding this comment.
Not at all - renamed all structs to have Env before Config. Renamed the function names config -> env_config
…fig test using system env vars in separate process
THardy98
force-pushed
the
expose_envconfig_c_bridge
branch
from
9ff4dc4 to
024df91
Compare
THardy98
force-pushed
the
expose_envconfig_c_bridge
branch
from
84d7d3c to
543a067
Compare
THardy98
force-pushed
the
expose_envconfig_c_bridge
branch
from
ce144c6 to
de26121
Compare
3 participants
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