Skip to content

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.

Sign up for GitHub

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

Merged
THardy98 merged 16 commits into from
Sep 19, 2025
Merged

feat: Expose envconfig functionality through C bridge #986

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
07bb436
feat: Expose envconfig functionality through C bridge
THardy98 Aug 12, 2025
60895cb
formatting
THardy98 Aug 25, 2025
dd2345b
more formatting fixes
THardy98 Aug 25, 2025
b1ea496
improve some test flakiness for envconfig module tests
THardy98 Aug 25, 2025
c4212d7
feat: Replace callback parameters with options structs in C bridge
THardy98 Sep 12, 2025
614c72e
Update config TLS field in C bridge to be Option
THardy98 Sep 15, 2025
91e73c5
Data in DataSource should be Vec<u8>, remove useless disable_file par…
THardy98 Sep 15, 2025
c04cd5c
Use TemporalCoreByteArrayRef instead of string pointers
THardy98 Sep 17, 2025
bceec7a
Merge branch 'master' into expose_envconfig_c_bridge
THardy98 Sep 17, 2025
b550109
Revert misc changes to existing tests
THardy98 Sep 17, 2025
024df91
Add Env prefix befor Config in envconfig c-bridge structs. Run envcon…
THardy98 Sep 19, 2025
543a067
Skip test is env vars aren't set
THardy98 Sep 19, 2025
204342a
Merge branch 'master' into expose_envconfig_c_bridge
THardy98 Sep 19, 2025
de26121
Linter complaint
THardy98 Sep 19, 2025
ac570c2
remaining lint fixes from cargo 1.88.0 update
THardy98 Sep 19, 2025
7c8efb0
cargo test-lint fix
THardy98 Sep 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions core-api/src/envconfig.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1129,7 +1129,7 @@ namespace = "my-namespace"
..Default::default()
};

let profile = load_client_config_profile(options, None).unwrap();
let profile = load_client_config_profile(options, Some(&HashMap::new())).unwrap();
assert_eq!(profile.address.as_ref().unwrap(), "my-address");
assert_eq!(profile.namespace.as_ref().unwrap(), "my-namespace");
}
Expand Down Expand Up @@ -1519,7 +1519,7 @@ address = "localhost:7233"
};

// Should not error, just returns an empty profile
let profile = load_client_config_profile(options, None).unwrap();
let profile = load_client_config_profile(options, Some(&HashMap::new())).unwrap();
assert_eq!(profile, ClientConfigProfile::default());
}

Expand Down Expand Up @@ -1595,7 +1595,7 @@ api_key = "my-api-key"
..Default::default()
};

let profile = load_client_config_profile(options, None).unwrap();
let profile = load_client_config_profile(options, Some(&HashMap::new())).unwrap();

// TLS should be enabled due to API key presence
assert!(profile.tls.is_some());
Expand All @@ -1616,14 +1616,17 @@ address = "some-address"
..Default::default()
};

let profile = load_client_config_profile(options, None).unwrap();
let profile = load_client_config_profile(options, Some(&HashMap::new())).unwrap();

// TLS should not be enabled
assert!(profile.tls.is_none());
}

#[test]
fn test_load_client_config_profile_from_system_env() {
// WARNING: This test modifies system environment variables which can cause
// test pollution if tests run in parallel.

Copy link
Copy Markdown
Member

@Sushisource Sushisource Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@THardy98 THardy98 Sep 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@THardy98 THardy98 Sep 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Set up system env vars. These tests can't be run in parallel.
unsafe {
std::env::set_var("TEMPORAL_ADDRESS", "system-address");
Expand Down
2 changes: 2 additions & 0 deletions core-c-bridge/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ prost = { workspace = true }
# cause non-determinism.
rand = "0.9.2"
rand_pcg = "0.9.0"
serde = { version = "1.0", features = ["derive"] }
serde_json = "1.0"
tokio = "1.47"
tokio-stream = "0.1"
Expand All @@ -38,6 +39,7 @@ features = ["ephemeral-server"]

[dependencies.temporal-sdk-core-api]
path = "../core-api"
features = ["envconfig"]

[dependencies.temporal-sdk-core-protos]
path = "../sdk-core-protos"
Expand Down
61 changes: 61 additions & 0 deletions core-c-bridge/include/temporal-sdk-core-c-bridge.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,53 @@ typedef void (*TemporalCoreClientRpcCallCallback)(void *user_data,
const struct TemporalCoreByteArray *failure_message,
const struct TemporalCoreByteArray *failure_details);

/**
* OrFail result for client config loading operations.
* Either success or fail will be null, but never both.
* If success is not null, it contains JSON-serialized client configuration data.
* If fail is not null, it contains UTF-8 encoded error message.
* The returned ByteArrays must be freed by the caller.
*/
typedef struct TemporalCoreClientConfigOrFail {
Copy link
Copy Markdown
Contributor

@cretz cretz Sep 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

@THardy98 THardy98 Sep 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not at all - renamed all structs to have Env before Config. Renamed the function names config -> env_config

const struct TemporalCoreByteArray *success;
const struct TemporalCoreByteArray *fail;
} TemporalCoreClientConfigOrFail;

/**
* Options for loading client configuration.
*/
typedef struct TemporalCoreClientConfigLoadOptions {
const char *path;
struct TemporalCoreByteArrayRef data;
bool config_file_strict;
struct TemporalCoreByteArrayRef env_vars;
} TemporalCoreClientConfigLoadOptions;

/**
* OrFail result for client config profile loading operations.
* Either success or fail will be null, but never both.
* If success is not null, it contains JSON-serialized client configuration profile data.
* If fail is not null, it contains UTF-8 encoded error message.
* The returned ByteArrays must be freed by the caller.
*/
typedef struct TemporalCoreClientConfigProfileOrFail {
const struct TemporalCoreByteArray *success;
const struct TemporalCoreByteArray *fail;
} TemporalCoreClientConfigProfileOrFail;

/**
* Options for loading a specific client configuration profile.
*/
typedef struct TemporalCoreClientConfigProfileLoadOptions {
const char *profile;
const char *path;
Comment thread
THardy98 marked this conversation as resolved.
Outdated
struct TemporalCoreByteArrayRef data;
bool disable_file;
bool disable_env;
bool config_file_strict;
struct TemporalCoreByteArrayRef env_vars;
} TemporalCoreClientConfigProfileLoadOptions;

typedef union TemporalCoreMetricAttributeValue {
struct TemporalCoreByteArrayRef string_value;
int64_t int_value;
Expand Down Expand Up @@ -771,6 +818,20 @@ void temporal_core_client_rpc_call(struct TemporalCoreClient *client,
void *user_data,
TemporalCoreClientRpcCallCallback callback);

/**
* Load all client profiles from given sources.
* Returns ClientConfigOrFail with either success JSON or error message.
* The returned ByteArrays must be freed by the caller.
*/
struct TemporalCoreClientConfigOrFail temporal_core_client_config_load(const struct TemporalCoreClientConfigLoadOptions *options);

/**
* Load a single client profile from given sources with env overrides.
* Returns ClientConfigProfileOrFail with either success JSON or error message.
* The returned ByteArrays must be freed by the caller.
*/
struct TemporalCoreClientConfigProfileOrFail temporal_core_client_config_profile_load(const struct TemporalCoreClientConfigProfileLoadOptions *options);

struct TemporalCoreMetricMeter *temporal_core_metric_meter_new(struct TemporalCoreRuntime *runtime);

void temporal_core_metric_meter_free(struct TemporalCoreMetricMeter *meter);
Expand Down
Loading
Loading