Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HTTP CONNECT proxy support #318

Merged
merged 8 commits into from
Aug 6, 2024

Conversation

adeal
Copy link
Contributor

@adeal adeal commented Aug 1, 2024

What was changed

This change adds HTTP CONNECT proxy support.

  • No need to update core submodule
  • Added Temporalio.Client.HttpConnectProxyConfig
  • Updated client to accept that config

Why?

Checklist

  1. Closes [Feature Request] HTTP CONNECT proxy support #215

  2. How was this tested:
    This is challenging to test w/o a local proxy setup, but I'm open to recommendations. If I'm able, I'll publish a local release and see if I can test this against an environment where proxy resources exist.

  3. Any docs updates needed?

@CLAassistant
Copy link

CLAassistant commented Aug 1, 2024

CLA assistant check
All committers have signed the CLA.

@adeal adeal force-pushed the dev/adeal/feature_http_connect_proxy branch from 9a12e56 to 536f2ea Compare August 1, 2024 14:56
src/Temporalio/Bridge/Interop/Interop.cs Outdated Show resolved Hide resolved
@@ -106,6 +106,7 @@ typedef struct ClientOptions {
const struct ClientTlsOptions *tls_options;
const struct ClientRetryOptions *retry_options;
const struct ClientKeepAliveOptions *keep_alive_options;
const HttpConnectProxyOptions *http_connect_proxy_options;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is auto-generated as part of the Rust build and therefore should match the Rust constructs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm getting an inconsistent experience, but this is ultimately what is gen'd locally for me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No prob, I may have just assumed it was going to generate const struct. If CI passes w/ what is generated here we're all good. Can ignore.

src/Temporalio/Client/HttpConnectProxyConfig.cs Outdated Show resolved Hide resolved
src/Temporalio/Client/HttpConnectProxyConfig.cs Outdated Show resolved Hide resolved
Copy link
Member

@cretz cretz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good. I think CI is failing because of the line endings of the Interop.cs file maybe? Unsure, I will try to look shortly (and may try out via merge to your fork branch if that's ok), but everything in this PR in general LGTM.

@cretz
Copy link
Member

cretz commented Aug 6, 2024

There was an issue where the FFI struct referenced HttpConnectProxyOptions instead of ClientHttpConnectProxyOptions. Fixed and regen'd. When CI passes, I will update PR from main then merge into main.

@adeal
Copy link
Contributor Author

adeal commented Aug 6, 2024

There was an issue where the FFI struct referenced HttpConnectProxyOptions instead of ClientHttpConnectProxyOptions. Fixed and regen'd. When CI passes, I will update PR from main then merge into main.

AH, obvious in hindsight. Thanks for tracking it down and making the fix.

@cretz cretz merged commit 2ce6c99 into temporalio:main Aug 6, 2024
7 checks passed
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.

[Feature Request] HTTP CONNECT proxy support
3 participants