Detect HTTPS_PROXY in HTTPClient #419
Conversation
justice-adams-apple
requested review from
cmcgee1024,
etcwilde and
shahmishal
as code owners
Sources/SwiftlyCore/HTTPClient.swift
Outdated
| } | ||
| } | ||
|
|
||
| public func shutdown() async throws { |
There was a problem hiding this comment.
question: why is this explicit shutdown required? I think that the deinit above should shut the http client down when an executor goes out of scope.
There was a problem hiding this comment.
removed the explicit shutdown. Got my wires crossed, you are correct the deinit should take care of this
| // Download libarchive | ||
| let httpExecutor = HTTPRequestExecutorImpl() | ||
| defer { | ||
| Task { |
There was a problem hiding this comment.
suggestion: If you leverage the deinit of the httpExecutor then this defer shouldn't be needed here. If an explicit shutdown is needed then it should probably use the syncShutdown() of the http client so that this defer doesn't need to launch an unstructured task that may or may not be run when expected since it is asynchronous.
| } | ||
|
|
||
| let libarchiveRequest = HTTPClientRequest(url: "https://github.com/libarchive/libarchive/releases/download/v\(libArchiveVersion)/libarchive-\(libArchiveVersion).tar.gz") | ||
| let libarchiveResponse = try await HTTPClient.shared.execute(libarchiveRequest, timeout: .seconds(60)) |
There was a problem hiding this comment.
praise: this looks good for cases where the build script is run in a http proxy environment.
thought: Maybe we should have a workflow that verifies that this build script works in such an environment. I'm not quite sure how to configure Linux so that the network traffic will be blocked unless it goes through a proxy.
justice-adams-apple
force-pushed
the
jadams/release-1.1/fix-proxy
branch
from
27a2534 to
6a69f8d
Compare
3 participants
Our build jobs won't work unless we detect and add the https proxy into the HTTP client prior to downloading the lib archive dependency. We can reuse the
HTTPRequestExecutorImplto detect and use the proxy correctly