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

feat: allow custom http client and use ctx for http1 apiclient. Fixes #12827 #12867

Merged
merged 1 commit into from
Apr 11, 2024

Conversation

williamburgson
Copy link
Contributor

the current implementation does not suite our usecase because the http Fascade interface does not take proxy url we have to use proxy for cross network calls for argo server

so we might as well just allow a fully configured http client to be passed in - this allows more flexible configurations of the HTTP1 client via custom roundtripper

Fixes #12827

Motivation

  • Allow more flexible configuration for the http1 workflow apiclient so that caller can inject runtime headers (e.g. per-request tracing id) or use a http proxy via roundtripper
  • Include the context for the request to argo server so that timeouts can be configured by the caller

Modifications

  • Add configuration option for HTTP1Client
  • Have Facade use the custom http client if provided
  • Use NewRequestWithContext to include context from caller
  • Update do to accept context from caller
  • Update service clients to use context, _ context.Context -> ctx context.Context, and pass that do Facade

Verification

…client. Fixes argoproj#12827

the current implementation does not suite our usecase because the http Fascade interface does not take proxy url
we have to use proxy for cross network calls for argo server

so we might as well just allow a fully configured http client to be
passed in - this allows more flexible configurations of the HTTP1 client
via custom roundtripper

Signed-off-by: williamburgson <[email protected]>
@williamburgson williamburgson marked this pull request as ready for review April 2, 2024 21:27
@Joibel Joibel self-assigned this Apr 3, 2024
Copy link
Member

@Joibel Joibel left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

@isubasinghe, could you take a look please?

Copy link
Member

@isubasinghe isubasinghe left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTP1 client for server should allow more flexible client config and use context when creating new requests
4 participants