Skip to content

Conversation

@adeal
Copy link
Contributor

@adeal adeal commented Aug 24, 2024

What was changed

Why?

Follow-up fix to #318.

The proxy settings were not being pushed down to core. Although the ClientHttpConnectProxyOptions were properly translated to core's HttpConnectProxyOptions, the TryFrom trait on the CoreClientOptions was not updated to include the new ClientHttpConnectProxyOptions.

Checklist

  1. Closes
    N/A

  2. How was this tested:

Unfortunately, there still is no viable path for automated test coverage as-is. However, I validated this change by configuring a temporal app to point to an invalid proxy location and verified that a connection was not successful. Previously, no error would occur since the connection was not being routed to the proxy target.

var client = await TemporalClient.ConnectAsync(new TemporalClientConnectOptions()
{
    TargetHost = "<valid_host>",
    HttpConnectProxy = new HttpConnectProxyOptions("localhost:9392"), // Arbitrary invalid location
});
Unhandled exception. System.InvalidOperationException: Connection failed: Server connection error: tonic::transport::Error(Transport, ConnectError(client error (Connect)

Caused by:
    Connection refused (os error 61)))
   at Temporalio.Bridge.Client.ConnectAsync(Runtime runtime, TemporalConnectionOptions options)
   at Temporalio.Client.TemporalConnection.GetBridgeClientAsync()
   at Temporalio.Client.TemporalConnection.ConnectAsync(TemporalConnectionOptions options)
   at Temporalio.Client.TemporalClient.ConnectAsync(TemporalClientConnectOptions options)
   at Program.<Main>$(String[] args) in /Users/adeal/src/samples-dotnet/src/ActivitySimple/Program.cs:line 15
   at Program.<Main>(String[] args)

  1. Any docs updates needed?

@adeal adeal force-pushed the dev/adeal/http_connect_fix branch from 8a1ed05 to 6e501e8 Compare August 25, 2024 06:40
@cretz
Copy link
Member

cretz commented Aug 26, 2024

We will prioritize working on temporalio/features#521

@cretz cretz merged commit aa3edef into temporalio:main Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants