update samples to use env config #50
Conversation
|
Waiting for next Ruby release with env config feature |
THardy98
force-pushed
the
update_samples_env_config
branch
from
7aa9c6e to
624c4ea
Compare
THardy98
force-pushed
the
update_samples_env_config
branch
2 times, most recently
from
f19dbb8 to
7f0d8db
Compare
|
The big diff is from the |
|
Note, CI is failing and I believe #63 needs to be solved before CI stops failing. I will add a reminder to work on this in a day or two. |
activity_heartbeating/starter.rb
Outdated
|
|
||
| # Load config and apply defaults | ||
| positional_args, keyword_args = Temporalio::EnvConfig::ClientConfig.load_client_connect_options | ||
| positional_args = ['localhost:7233', 'default'] if positional_args.empty? |
There was a problem hiding this comment.
I do wonder, and we may have to discuss separately, what if they set the address in env config but not the namespace? Or vice versa?
There was a problem hiding this comment.
I looked into this, there's a bad behavior here:
def to_client_connect_options
positional_args = [address, namespace].compact
...
end
We shouldn't be compacting positional args, that makes things weird, particularly in the case where namespace is provided by address isn't. I'm opening a PR now to fix that. I'll make the change to these samples to instead do:
positional_args, keyword_args = load_client_connect_options
address, namespace = positional_args
address ||= 'localhost:7233'
namespace ||= 'default'
client = Client.connect(address, namespace, **keyword_args)
instead of the single check for both positional args
There was a problem hiding this comment.
Nice. I also think you can just do:
args, kwargs = Temporalio::EnvConfig::ClientConfig.load_client_connect_options
args[0] ||= 'localhost:7233'` # Default address
args[1] ||= 'default' # Default namespace
| 'default', | ||
| logger: Logger.new($stdout, level: Logger::INFO) | ||
| ) | ||
| client = Temporalio::Client.connect(*args, **kwargs, logger: Logger.new($stdout, level: Logger::INFO)) |
There was a problem hiding this comment.
Not a big deal since kwargs can never have logger in it, but usually in Ruby you would recommend the explicit args/kwargs to go before the splatting.
context_propagation/starter.rb
Outdated
| interceptors: [ContextPropagation::Interceptor.new(:my_user)] | ||
| ) | ||
| client = Temporalio::Client.connect(*args, **kwargs, logger: Logger.new($stdout, level: Logger::INFO), | ||
| interceptors: interceptors) |
There was a problem hiding this comment.
| interceptors: interceptors) | |
| interceptors:) |
Providing the name that's the same as the kwarg is redundant (this applies in other places too)
rails_app/lib/temporal_client.rb
Outdated
|
|
||
| # Load config and apply defaults | ||
| args, kwargs = Temporalio::EnvConfig::ClientConfig.load_client_connect_options | ||
| args = ["localhost:7233", "default"] if args.empty? |
There was a problem hiding this comment.
I think this is still the old form and should be the args[0] ||= style
THardy98
force-pushed
the
update_samples_env_config
branch
from
55dd4d1 to
bf002ae
Compare
What was changed
Update samples to use client environment configuration.
Why?
Client environment configuration is intended as the preferred way to configure clients going forward.
Part of [Feature Request] Environment configuration #40
How was this tested:
Existing tests
Any docs updates needed?
No