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
Allow per-request base URIs in HttpClient #7922
Conversation
9d7f429
to
be10518
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
protected HttpRequest(HttpRuntimeConfig config, URI baseUri) { | ||
requireNonNull(baseUri, "Base URI cannot be null"); | ||
checkArgument( | ||
"http".equals(baseUri.getScheme()) || "https".equals(baseUri.getScheme()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe equalsIgnoreCase
? Per rfc3986 implementations should accept either case.
static Builder builder() { | ||
return new HttpClientBuilderImpl(); | ||
} | ||
|
||
URI getBaseUri(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please leave this one
@@ -31,16 +31,18 @@ runs: | |||
- name: Gradle / Init | |||
uses: gradle/gradle-build-action@v2 | |||
with: | |||
cache-read-only: ${{ inputs.cache-read-only }} | |||
# cache-read-only: ${{ inputs.cache-read-only }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated change in this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying to fix gradle build cache issues.
@@ -31,7 +36,14 @@ public abstract class HttpRequest | |||
protected String accept = "application/json; charset=utf-8"; | |||
|
|||
protected HttpRequest(HttpRuntimeConfig config) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can get rid of the single-arg constructor and only use the new one below. WDYT?
Authentication is another component that could benefit from a per-request override mechanism – I can look into that in another PR. |
No description provided.