-
Notifications
You must be signed in to change notification settings - Fork 4
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: custom headers can be specified per request (query/write) #108
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #108 +/- ##
==========================================
+ Coverage 88.39% 88.53% +0.14%
==========================================
Files 15 15
Lines 836 855 +19
Branches 120 129 +9
==========================================
+ Hits 739 757 +18
Misses 42 42
- Partials 55 56 +1 ☔ View full report in Codecov by Sentry. |
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.
Found a couple of nit-picks.
- Comment in
ClientConfig.java
is wordedif... than
notif... then
- Minor lint issue in
RestClient.java
with two arguments on the same line - The internal
Builder
class ofWriteOptions
does not support the new propertyheaders
@@ -122,8 +122,7 @@ public void checkServerTrusted( | |||
void request(@Nonnull final String path, | |||
@Nonnull final HttpMethod method, | |||
@Nullable final byte[] data, | |||
@Nullable final Map<String, String> headers, | |||
@Nullable final Map<String, String> queryParams) { | |||
@Nullable final Map<String, String> queryParams, @Nullable final Map<String, String> headers) { |
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.
Two arguments here should probably follow style of one arg per line for readability.
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.
Fixed
@@ -140,6 +140,7 @@ public String getDatabase() { | |||
|
|||
/** | |||
* Gets the default precision to use for the timestamp of points. | |||
* If no precision is specified than 'ns' is used. |
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.
"than" should be "then"
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.
Fixed
@@ -63,42 +70,78 @@ public final class WriteOptions { | |||
private final WritePrecision precision; | |||
private final Integer gzipThreshold; | |||
private final Map<String, String> defaultTags; | |||
private final Map<String, String> headers; |
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.
The Builder class (ln 230) does not support this property.
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.
Fixed
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.
Tests pass locally. Looks good to go.
@karel-rehor thx |
Closes #109
https://influxdata.slack.com/archives/C5BSZ026L/p1709551752382859
Proposed Changes
This add support for use the custom headers per request:
Checklist