Support environment configuration #1849
Conversation
| // | ||
| // WARNING: Environment configuration is currently experimental. | ||
| type ClientConfig struct { | ||
| Profiles map[string]*ClientConfigProfile |
There was a problem hiding this comment.
Worth saying what the key is here.
| // ClientConfigCodec is codec configuration for a client. | ||
| // | ||
| // WARNING: Environment configuration is currently experimental. | ||
| type ClientConfigCodec struct { |
There was a problem hiding this comment.
Maybe call this ClientConfigRemoteCodec? What, if anything, could ever be configured here for local codec? And, even then, maybe it'd make sense to have them be different sub-configs
There was a problem hiding this comment.
It is the codec field in config and TEMPORAL_CODEC_ prefixed env vars and Codec field name in the structure in addition to this type name. So I want it to match those. If we want though, I could change the config field name, env var prefix, struct field name, and this type name.
| // See [ClientConfig.FromTOML] for details on format. | ||
| // | ||
| // WARNING: Environment configuration is currently experimental. | ||
| func LoadClientConfig(options LoadClientConfigOptions) (ClientConfig, error) { |
There was a problem hiding this comment.
This and LoadClientOptions don't have distinct enough names, IMO.
Maybe LoadClientOptionsFromFile ?
There was a problem hiding this comment.
I think it's distinct enough given its return type and such, but I can change and change the param structure to LoadClientConfigFromFileOptions. Would like to wait on other opinions if anyone has any. I am concerned there may also be too-similar naming concerns in other SDKs.
| if env == nil { | ||
| env = EnvLookupOS | ||
| } | ||
| if s, ok := env.LookupEnv("TEMPORAL_ADDRESS"); ok { |
There was a problem hiding this comment.
I think it probably makes sense to make these all public constants. It helps with both documentation and, since people can technically implement their own lookup, they might want to programmatically use them.
There was a problem hiding this comment.
I don't expect this to be where such things are documented. I am expecting docs.temporal.io to house the documentation with these environment variables. I don't expect anyone to ever reference such a constant programmatically either like one might expect with an exported constant.
| } | ||
|
|
||
| // ToClientOptionsOptions are options for [ClientConfig.ToClientOptions] and [ClientConfigProfile.ToClientOptions]. | ||
| type ToClientOptionsOptions struct { |
There was a problem hiding this comment.
OptionsOptions feels a little awkward for a name, but can't think of a great alternative..
There was a problem hiding this comment.
Agreed, I had brought this up internally but no good solution was provided. I don't expect users to use this part of the API much.
| if err != nil { | ||
| return ClientConfigProfile{}, err | ||
| } | ||
| // Find profile or fail. The one situation where we don't fail here is if the profile is not user-set |
There was a problem hiding this comment.
This comment took me a sec to understand, I think something like this could be easier to understand
| // Find profile or fail. The one situation where we don't fail here is if the profile is not user-set | |
| // Find user-set profile or use default. Fail if user uses invalid profile. |
There was a problem hiding this comment.
@cretz I updated my local copy of your Go SDK to commit #362043e and then tried this again.
Following the approach previously described, I tested this with mTLS and found that it now works as expected.
I then attempted to do the same with API Key authentication. I unset the previous four environment variables and then set the following:
TEMPORAL_API_KEY=<redacted>TEMPORAL_NAMESPACE=tom-dev.a2dd6TEMPORAL_ADDRESS=us-east-1.aws.api.temporal.io:7233
When I started the Worker, it failed:
$ go run worker/main.go
2025/02/25 13:53:31 INFO No logger configured for temporal client. Created default one.
2025/02/25 13:53:32 Unable to create Temporal client. failed reaching server: rpc error: code = Unauthenticated desc = Jwt is expired
exit status 1
I then looked at the code and realized that perhaps I needed to run this:
$ export TEMPORAL_TLS=true
I then repeated the previous command and found that the Worker now started. I did the same in the other terminal before running the command to start the Workflow, which then completed successfully.
In the end, it seems to work fine, although I think we'd be wise to emphasize in the docs that you must set TEMPORAL_TLS=true when using API Key authentication with Temporal Cloud, because I think people might overlook it otherwise.
@tomwheeler - that is the error you got originally, I am not sure how EDIT: @tomwheeler - I cannot replicate. With the set TEMPORAL_ADDRESS=us-west-2.aws.api.temporal.io:7233
set TEMPORAL_NAMESPACE=sdk-ci.a2dd6
set TEMPORAL_API_KEY=omitted
go run ./helloworld/workerand in another terminal set TEMPORAL_ADDRESS=us-west-2.aws.api.temporal.io:7233
set TEMPORAL_NAMESPACE=sdk-ci.a2dd6
set TEMPORAL_API_KEY=omitted
go run ./helloworld/starterIt works as expected. |
What was changed
go.temporal.io/sdk/contrib/envconfigmodule that supports loadingSee temporalio/features#441, this is mostly based on that proposal with some slight changes.
This SDK may support a few more shortcuts than other SDKs due to its use in CLI. Documentation about file format and which env vars do what will be made available on docs.temporal.io before this launches GA.
Checklist