Skip to content

default profile fallback to localhost and 'default' namespace #968

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

Closed
wants to merge 2 commits into from
Closed

default profile fallback to localhost and 'default' namespace #968

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ce37c24
default profile fallback to localhost and 'default' namespace
THardy98 Jul 28, 2025
efa0595
Merge branch 'master' into env_config_default_fallback
THardy98 Aug 1, 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
30 changes: 26 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 @@ -319,7 +319,8 @@ pub fn load_client_config_profile(
};

let mut profile = if options.disable_file {
ClientConfigProfile::default()
// Create a default profile that connects to localhost.
ClientConfigProfile::default_localhost_profile()
} else {
// Load the full config
let config = load_client_config(
Expand Down Expand Up @@ -349,7 +350,9 @@ pub fn load_client_config_profile(
} else if !profile_unset {
Err(ConfigError::ProfileNotFound(profile_name))
} else {
Ok(ClientConfigProfile::default())
// If no profile was requested and the default one doesn't exist,
// create a default profile that connects to localhost.
Ok(ClientConfigProfile::default_localhost_profile())
}?
};

Expand Down Expand Up @@ -397,6 +400,14 @@ impl ClientConfig {
}

impl ClientConfigProfile {
/// Returns a client config profile with default values for connecting to localhost.
pub fn default_localhost_profile() -> Self {
Copy link
Contributor

@tconley1428 tconley1428 Jul 29, 2025

Choose a reason for hiding this comment

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

I think typically you would provide an impl for default instead.

Copy link
Contributor Author

@THardy98 THardy98 Jul 31, 2025

Choose a reason for hiding this comment

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

i was unsure if providing an impl for default would be unclear that it's providing a value for address instead of None

Copy link
Contributor

@tconley1428 tconley1428 Jul 31, 2025

Choose a reason for hiding this comment

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

I would say that if we are changing every default call to this, then potentially that means the existing derived default doesn't make sense and we should remove the derive and implement a different default. If that's not the case (it's just what it looked like), then I think you are right. Either way, not that important.

Copy link
Member

@Sushisource Sushisource Aug 1, 2025

Choose a reason for hiding this comment

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

I think it's reasonable for this to just be the Default impl. You can update the docstring for the struct.

Self {
address: Some("localhost:7233".to_string()),
namespace: Some("default".to_string()),
Comment on lines +406 to +407
Copy link
Member

@cretz cretz Aug 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'm think this should be done lang side specifically when loading connect options only. The client config should only be what is loaded, not with defaults.

Can you show me sample Python code of loading a config from toml, making a change, and then serializing it to toml again? Will that save now put the address and namespace? Or anything else? It is a very legitimate use case to make "load client config" calls just to see what you literally have in your client config. People should be allowed to know whether they've configured address/namespace in their environment or not if they want to know that.

(also, how do I disable using environment variables when loading, is the caller expected to use empty hash map?)

Copy link
Contributor Author

@THardy98 THardy98 Aug 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

... Python code of loading a config from toml, making a change, and then serializing it to toml again? Will that save now put the address and namespace?

Yes

I'm think this should be done lang side specifically when loading connect options only. The client config should only be what is loaded, not with defaults.

Yeah I think that's valid. I'll close this PR and make the corresponding change in temporalio/sdk-python#1004

..Default::default()
}
}
/// Apply environment variable overrides to this profile.
/// If `env_vars` is `None`, the system's environment variables will be used as the source.
pub fn load_from_env(
Expand Down Expand Up @@ -1518,9 +1529,9 @@ address = "localhost:7233"
..Default::default()
};

// Should not error, just returns an empty profile
// Should not error, just returns a default localhost profile
let profile = load_client_config_profile(options, None).unwrap();
assert_eq!(profile, ClientConfigProfile::default());
assert_eq!(profile, ClientConfigProfile::default_localhost_profile());
}

#[test]
Expand Down Expand Up @@ -1582,6 +1593,17 @@ address = "localhost:7233"
assert_eq!(profile.namespace.as_ref().unwrap(), "env-namespace");
}

#[test]
fn test_load_client_config_profile_disable_file_no_env_uses_default() {
let options = LoadClientConfigProfileOptions {
disable_file: true,
..Default::default()
};

let profile = load_client_config_profile(options, None).unwrap();
assert_eq!(profile, ClientConfigProfile::default_localhost_profile());
}

#[test]
fn test_api_key_tls_auto_enable() {
// Test 1: When API key is present, TLS should be automatically enabled
Expand Down
Loading