Env config #509
Env config #509
Conversation
|
Relies on temporalio/sdk-core#986 |
- Convert ClientConfig, ClientConfigProfile, and ClientConfigTls to record types - Remove ICloneable interface (records have built-in cloning) - Use private init setters instead of private set - Replace Dictionary parameters with IReadOnlyDictionary in public API Replace JSON serialization with record serialization - Replace ToJson()/FromJson() methods with ToRecord()/FromRecord() methods - Create ClientConfigTlsRecord and ClientConfigProfileRecord for TOML structure - Support conversion between config objects and TOML-ready record structures Rename namespace from Configuration to EnvConfig - Change namespace: Temporalio.Client.Configuration → Temporalio.Client.EnvConfig Rename ClientConfig class to ClientEnvConfig - Rename ClientConfig → ClientEnvConfig - Update all references in tests and documentation - Rename file ClientConfig.cs → ClientEnvConfig.cs - Renaming EnvProfile -> ConfigProfile Fix TLS disabled tri-state and profile not found behavior in environment config - Change Tls.Disabled from bool to bool? to support tri-state behavior (null/true/false) - Consolidate DataSource class back into ClientEnvConfig.cs - Add test for tri-state TLS behavior and profile resolution - Update profile loading logic to match Rust core behavior: - profile=null with missing "default" → return empty profile - profile="default" with missing "default" → throw exception
… Vec<u8> from DataSource instead of String
Yes, RFR. Will resolve the above comments |
| /// Convert to a record structure that can be used for TOML serialization. | ||
| /// </summary> | ||
| /// <returns>Dictionary mapping profile names to their record representations.</returns> | ||
| public IReadOnlyDictionary<string, ProfileRecord> ToRecord() |
There was a problem hiding this comment.
I wouldn't necessarily call this a "record", maybe ToDictionary? Arguably across SDKs this would be a dictionary with a single "profiles" entry if we wanted to properly match TOML, but I see we didn't really do that anywhere.
There was a problem hiding this comment.
Changed to ToDictionary.
wrt changing to a single "profiles" entry, if it's something we care about I can open some PRs once this is in
src/Temporalio/Bridge/EnvConfig.cs
Outdated
| Dictionary<string, object?> dict => dict, | ||
| JsonElement el => JsonSerializer.Deserialize<Dictionary<string, object?>>(el.GetRawText(), JsonOptions), |
There was a problem hiding this comment.
Which situations is this one vs the other? Doesn't make sense IMO to have a forgiving parser for our own internal JSON format (not completely against the use of JSON to pass across Core, just a bit abnormal).
There was a problem hiding this comment.
Yeah - I was having trouble deserializing the incoming JSON properly (nested JSON) so had this as a fallback which worked. Not very nice though.
I've removed this in favor of actual JSON DTO objects, much cleaner.
| /// </summary> | ||
| /// <param name="record">The record to convert from.</param> | ||
| /// <returns>Profile configuration instance.</returns> | ||
| public static ConfigProfile FromRecord(ProfileRecord record) |
There was a problem hiding this comment.
Why a "record" instead of a dictionary? That record won't necessarily serialize to TOML correctly which is the purpose of this IIRC
There was a problem hiding this comment.
Why is it the case that a record won't serialize to TOML correctly? I haven't seen anything on this.
There was a problem hiding this comment.
Depends on the TOML library in use on how it handles PascalCased properties and such, but even if it can, users will have to know to set the setting to convert to underscore-based names and such. If you give/accept a dictionary (of dictionaries/lists/primitives), you can control the key names. And as a bonus you don't have to have new classes.
There was a problem hiding this comment.
Have changed this to use dictionaries instead of records
… TLS configuration (disabled, enabled, not configured)
…ods on the existing Tls and Profile inner classes
What was changed
Add environment configuration feature
Closes [Feature Request] Environment Configuration #490
How was this tested:
Integration test suite
ClientConfigTests.csAny docs updates needed?
Yes