-
Notifications
You must be signed in to change notification settings - Fork 68
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
Add support for custom HTTP headers in API calls #35
Add support for custom HTTP headers in API calls #35
Conversation
This commit adds a map of custom HTTP headers to UnleashConfig, and includes these headers in calls to the Unleash server. This may be used to, e.g., add an `Authorization` field if the server is behind a wrapper or proxy which requires authentication. See Unleash/unleash#222 on GitHub for background discussion.
@@ -88,8 +101,17 @@ public UnleashContextProvider getContextProvider() { | |||
return contextProvider; | |||
} | |||
|
|||
public static void setRequestProperties(HttpURLConnection connection, UnleashConfig config) { | |||
connection.setRequestProperty(UNLEASH_APP_NAME_HEADER, config.getAppName()); | |||
connection.setRequestProperty(UNLEASH_INSTANCE_ID_HEADER, config.getInstanceId()); |
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.
Can this set User-Agent
as well? Since this is getting a small refactor either way 🙂
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.
Sure. I didn't think to add it as there was no User-Agent
in the code to begin with, but I'll add it now. Based on the linked commit, I take it the User-Agent
should be the app name.
public static void setRequestProperties(HttpURLConnection connection, UnleashConfig config) { | ||
connection.setRequestProperty(UNLEASH_APP_NAME_HEADER, config.getAppName()); | ||
connection.setRequestProperty(UNLEASH_INSTANCE_ID_HEADER, config.getInstanceId()); | ||
for (String name : config.getCustomHttpHeaders().keySet()) { |
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.
Is this module java 8? If so, can use config.getCustomHttpHeaders().keySet().forEach
. If not java 8, ignore!
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.
maven-compiler-plugin
in the pom.xml
specifies a source and target of 1.8, so I guess that makes the module Java 8. I'll change it to forEach
, that is nicer – thanks!
|
||
import no.finn.unleash.UnleashContextProvider; | ||
|
||
public class UnleashConfig { | ||
protected static final String UNLEASH_APP_NAME_HEADER = "UNLEASH-APPNAME"; |
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.
can be package local instead of protected, right?
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.
Yes, I'll change it, thanks.
Following code review, this commit - adds a `User-Agent` field to the request properties set by UnleashConfig.setRequestProperties - changes two variables from 'protected' to package-private scope - replaces a for loop with a Java 8 forEach
Nice job, even got tests! Thanks =) |
Will release this next week (traveling this weekend, and do not have correct keys set up on my laptop). |
Released as 2.1.2! |
Nice, thanks!
😄 |
This commit adds a map of custom HTTP headers to UnleashConfig, and
includes these headers in calls to the Unleash server. This may be used
to, e.g., add an
Authorization
field if the server is behind a wrapperor proxy which requires authentication.
See Unleash/unleash#222 on GitHub for background discussion.