💥 Enable TLS if api key specified #567
Conversation
THardy98
changed the title
THardy98
force-pushed
the
enable-tls-api-key
branch
2 times, most recently
from
c5c054f to
b3277e8
Compare
THardy98
force-pushed
the
enable-tls-api-key
branch
from
06277d6 to
0c8ebc7
Compare
| var interopOptions = options.ToInteropOptions(scope); | ||
|
|
||
| // TLS should be auto-enabled when API key is provided and TLS not explicitly set | ||
| Assert.True(interopOptions.tls_options != null); |
There was a problem hiding this comment.
Assert.NotNull will give better error message when condition fails.
| var interopOptions = options.ToInteropOptions(scope); | ||
|
|
||
| // TLS should remain disabled when explicitly set to null | ||
| Assert.True(interopOptions.tls_options == null); |
There was a problem hiding this comment.
Assert.Null will give better error message when condition fails.
| using Temporalio.Client; | ||
| using Xunit; | ||
|
|
||
| public unsafe class TemporalConnectionOptionsTests |
There was a problem hiding this comment.
Could you add tests for when:
ApiKeyis not set, is null, or is empty?Tlsis set to a non-null value?
| /// <summary> | ||
| /// Gets a value indicating whether TLS was explicitly set (even to null). | ||
| /// </summary> | ||
| internal bool TlsExplicitlySet { get; private set; } |
There was a problem hiding this comment.
I don't think we should take this approach. The difference between a user manually doing = null and the default of null would be very confusing. I think for .NET we should add a bool Disabled property to TlsOptions for users that want to explicitly disable TLS. null should be considered unset/default (regardless of whether it's set that way)
There was a problem hiding this comment.
Yeah, this is a bit nicer. Done
There was a problem hiding this comment.
Hrmm, these tests may be bit too detailed/low-level. You are testing internals and only relying on us making internals visible to this test project. Note we don't have any other tests confirming the bridge options conversions. Usually we test user-facing behavior.
How would a user write a test to confirm their setting of TLS disabled or API key or whatever does the right thing? Granted it's probably not very easy to do this. One approach that you may take is to alter the "Test cloud" CI test to use API key instead of mTLS to at least confirm the API key default.
Regardless, not a big deal if you keep these, just a bit strange for this repo (and why there never was anything like this in the past).
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