Environment configuration #326
Conversation
| # @return [Pathname, String, nil] Client certificate source | ||
| # @!attribute [r] client_private_key | ||
| # @return [Pathname, String, nil] Client key source | ||
| class ClientConfigTLS |
There was a problem hiding this comment.
I think this should be a https://docs.ruby-lang.org/en/master/Data.html class, same for others here. Can do a Foo = Data.define(... and then after that a class Foo where you can add methods if needed. You'll at least need to define initialize that just calls super if you want defaults for the kwargs.
| # TLS configuration as specified as part of client configuration | ||
| # | ||
| # @!attribute [r] disabled | ||
| # @return [Boolean] If true, TLS is explicitly disabled |
| # Create a ClientConfigTLS from a hash | ||
| # @param hash [Hash, nil] Hash representation | ||
| # @return [ClientConfigTLS, nil] The TLS configuration or nil if hash is nil/empty | ||
| def self.from_hash(hash) |
There was a problem hiding this comment.
Hrmm, help me remember, why do we have this (and the to_hash) functionality exposed to users? We don't in Go but I saw we do in Python. If it's for TOML serialization ability, I figure that's at the profile level, but even if we do want it here, shouldn't that mean disabled can be a boolean or nil? There's a difference between setting disabled as false and not having it set at all I think.
If we are going to have a to and from hash, they should be symmetrical and exactly represent what is in TOML without defaults
| # @return [Hash, false] A TLS config hash or false if disabled | ||
| def to_connect_tls_config |
There was a problem hiding this comment.
Some languages call these "options" such as Ruby and .NET. IMO this should be
| # @return [Hash, false] A TLS config hash or false if disabled | |
| def to_connect_tls_config | |
| # @return [Connection::TLSOptions, false] TLS options or false if disabled | |
| def to_tls_options |
| # Create a ClientConfigTLS from a hash | ||
| # @param hash [Hash, nil] Hash representation | ||
| # @return [ClientConfigTLS, nil] The TLS configuration or nil if hash is nil/empty | ||
| def self.from_hash(hash) |
There was a problem hiding this comment.
Would be better named from_h (here and elsewhere)
| # If it's a string path (from TOML), read the file | ||
| # Otherwise return as content | ||
| if File.exist?(source) |
There was a problem hiding this comment.
You should know whether this is a path or not from TOML, should not have to use File.exists to switch between behaviors
| path, data = Temporalio::EnvConfig.source_to_path_and_data(config_source) | ||
|
|
||
| raw_profile = Temporalio::Internal::Bridge::EnvConfig.load_client_connect_config( |
There was a problem hiding this comment.
| path, data = Temporalio::EnvConfig.source_to_path_and_data(config_source) | |
| raw_profile = Temporalio::Internal::Bridge::EnvConfig.load_client_connect_config( | |
| path, data = :EnvConfig.source_to_path_and_data(config_source) | |
| raw_profile = Internal::Bridge::EnvConfig.load_client_connect_config( |
Usually only need to qualify relative to what you're already within
| @@ -1,5 +1,6 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| require 'temporalio/envconfig' | |||
There was a problem hiding this comment.
Doesn't matter much, but I think this should only be required where it is used, though I could also see it being required from client.rb as a convenience, but I don't think it needs to be required from top-level for convenience. I should have said the same about versioning_override when it was added here.
| let hash = RHash::new(); | ||
| match ds { | ||
| DataSource::Path(p) => { | ||
| hash.aset("path", ruby.str_new(p))?; |
There was a problem hiding this comment.
All of the hashes in this Rust library should use symbols for keys IMO. But now that I think about it, maybe we should be returning a Rust struct that is accessible in Ruby. Maybe something like https://docs.rs/magnus/latest/magnus/derive.TypedData.html. Or if we did want struct/data, https://docs.rs/magnus/latest/magnus/struct.Ruby.html#method.define_struct or https://docs.rs/magnus/latest/magnus/struct.Ruby.html#method.define_data.
Not sure, can discuss, but I think hashes of string keys is probably the least safe/efficient for complex objects like these. While efficiency doesn't strictly matter for non-user-facing code and for how rare this is called, I think we should set a reasonable example for complex objects returned to Ruby from calls in Rust.
EDIT: Revisiting the Core envconfig layer, I wonder if it'd be worth having the Rust side serialize to a TOML-like-but-still-typesafe structure, and then we can convert that to Ruby hash. It probably would not be hard to have a serde-to-Ruby-hash. Sure it would still be string hash keys, but it would be cleaner I think. There's not that many data types to convert from TOML/Rust to Ruby.
| ) | ||
| } | ||
|
|
||
| // load_client_connect_config(profile: String|nil, path: String|nil, data: String|nil, disable_file: bool, disable_env: bool, config_file_strict: bool, env_vars: Hash|nil) |
There was a problem hiding this comment.
Feel free to just accept a defined-in-Ruby struct options set as we do in other Rust-from-Ruby calls if that makes it cleaner once it gets to this many parameters
What was changed
Added environment configuration
Closes [Feature Request] Environment Configuration #287
How was this tested:
Added test suite
Any docs updates needed?
Yes