💥 enable TLS if api key provided #366
Conversation
| # Auto-enable TLS when API key provided and tls not explicitly set | ||
| # Convert nil to appropriate boolean: true if API key provided, false otherwise | ||
| tls = !api_key.nil? if tls.nil? |
There was a problem hiding this comment.
Hrmm, not sure we should alter the user-visible options which represents what the user passes in or clearly-visible parameter defaults. Of if we did want to change the parameter default here, we technically could change the default of nil here (and in connect) to tls: !api_key.nil?.
So two options - we either change the parameter default where the param is, or we only make this change when we're building the bridge options and leave the user-facing options as the value passed in to the function. Not sure which is best honestly, though I kinda lean towards the latter.
There was a problem hiding this comment.
moved the code change to when we construct bridge options
| target_host:, | ||
| api_key: nil, | ||
| tls: false, | ||
| tls: nil, |
There was a problem hiding this comment.
A bit surprised/disappointed this didn't require that you change sig/temporalio/client/connection.rbs (same for client.rbs). I would have expected a bundle exec rake steep call to fail, strange. Anyways, can we update those two sig files to support nil?
THardy98
changed the title
|
removed the added test file, instead opting for the cloud test in ci, like in .NET |
| namespace, | ||
| api_key: nil, | ||
| tls: false, | ||
| tls: nil, |
There was a problem hiding this comment.
Should also update the YARD docs above the method for this param (and explain the runtime default)
There was a problem hiding this comment.
I'm not seeing this diff
There was a problem hiding this comment.
Above, there is:
# @param tls [Boolean, Connection::TLSOptions] If false, do not use TLS. If true, use system default TLS options. If
# TLS options are present, those TLS options will be used.
It would change to:
# @param tls [Boolean, Connection::TLSOptions, nil] If false, do not use TLS. If true, use system default TLS options. If TLS
# options are present, those TLS options will be used. If nil (the default), TLS will be auto-enabled if
# api_key is provided.
Same as you did in the connection file. But not a big deal.
| tls = @options.tls | ||
| tls = true if tls.nil? && @options.api_key | ||
|
|
||
| if tls |
There was a problem hiding this comment.
| tls = @options.tls | |
| tls = true if tls.nil? && @options.api_key | |
| if tls | |
| if @options.tls || (@options.tls.nil? && @options.api_key) |
Reads a bit easier to me, but up to you
What was changed
DWISOTT
Part of Assume TLS enabled if API key provided in SDKs features#687
How was this tested:
Unit tests
Any docs updates needed?
Maybe