adding proxy setting support from env to be more like other cli's #370
Conversation
|
NetflixOSS » denominator » denominator-pull-requests #135 SUCCESS |
|
was thinking about http://stefanbirkner.github.io/system-rules/ as a possibility.. |
| @@ -426,6 +427,44 @@ void overrideFromEnv(Map<String, String> env) { | |||
| } | |||
| } | |||
|
|
|||
| static void setProxyFromEnv() { | |||
| String envHttpProxy = System.getenv("HTTP_PROXY"); | |||
There was a problem hiding this comment.
this looks refactorable.. I think CLI has guava (implicitly), so you can use default parsing in HostAndPort. Next, I think it is worth parameterizing the 's' so that the logic isn't completely copy/paste.
ex.
setProxyHostAndPort("http", System.getenv("HTTP_PROXY"));
setProxyHostAndPort("https", System.getenv("HTTPS_PROXY"));There was a problem hiding this comment.
yes will do, forgot that guava is on the cli..
There was a problem hiding this comment.
|
Thanks for giving a go! NP if my shotgun advice is a rathole :) |
|
NetflixOSS » denominator » denominator-pull-requests #136 SUCCESS |
|
refactored, but i dont think HostAndPort would really make it any more concise in this situation.. i like the system rules (going to use that in the future), but there's no help for System.getenv or setting the env unfortunately. |
|
|
||
| static void setProtocolProxyFromEnv(String proto) { | ||
| String lowerProto = Ascii.toLowerCase(proto); | ||
| String envProxy = System.getenv(Ascii.toUpperCase(proto) + "_PROXY"); |
There was a problem hiding this comment.
if you move this to a parameter, then you don't use System.getenv inside this method. That makes it testable with system rules!
There was a problem hiding this comment.
ex.
Advice on test.
the happy path test asserts that System.getProperty returns valid host and port, and host and default port
the failure test asserts that System.exit was called and that System.err message logged.
http://stefanbirkner.github.io/system-rules/
// use this to reset the system rules.
@Rule
public final RestoreSystemProperties restoreSystemProperties = new RestoreSystemProperties();
// Use this to make sure the message logged
@Rule
public final SystemErrRule systemErrRule = new SystemErrRule().enableLog();
// use this to test exit occurred on malformed
@Rule
public final ExpectedSystemExit exit = ExpectedSystemExit.none();There was a problem hiding this comment.
Yeah good idea, will fix
|
lemme know if testing is too much a bear.. I can refactor and help out if so. |
|
NetflixOSS » denominator » denominator-pull-requests #137 SUCCESS |
|
that framework is great, thanks for suggestion |
| setProtocolProxyFromEnv("https"); | ||
| } | ||
|
|
||
| void setProtocolProxyFromEnv(String proto) { |
There was a problem hiding this comment.
make this static and life gets sooo easy. just call it directly!
There was a problem hiding this comment.
like..
private static void setProxyFromEnv() { // static cause not needed to be not-static :P
setProtocolProxy("http", System.getEnv("HTTP_PROXY"));
setProtocolProxy("https", System.getEnv("HTTPS_PROXY"));
}
// call DenominatorCmd.setProtocolProxy("https", "https://10.0.0.1:8989") directly in tests
// visible for testing
static void setProtocolProxy(String scheme, nullableUrl) {
|
I like the idea of end-to-end, but I think the subclassing thing makes it harder to understand the key change. How about just calling the static method and verifying behavior with the rules you've already got? |
|
I can revert back to how it was before but I do like the fact it is fully tested now. If I make it static I can't fake the env properties easily. |
|
Ok I think I misread one of the comments.. |
|
my fault.. just updated with a code example. sorry about the run-around.
|
|
sorry i fixed it right away, just had limited connectivity.. |
|
NetflixOSS » denominator » denominator-pull-requests #138 SUCCESS |
|
NetflixOSS » denominator » denominator-pull-requests #139 SUCCESS |
| @@ -80,12 +34,27 @@ | |||
| import denominator.ultradns.UltraDNSProvider; | |||
| import feign.Logger; | |||
| import feign.Logger.Level; | |||
| import io.airlift.airline.Cli; | |||
| import io.airlift.airline.*; | |||
|
intelij messed up imports, should be right now. |
|
NetflixOSS » denominator » denominator-pull-requests #140 SUCCESS |
|
sorry to say it, but imports are still the majority of this change :P |
along with unit tests didn't added authenticated proxy support since that would be a much more intrusive change..
|
coolio! Thanks tons, Jeff. Will merge on green. |
|
NetflixOSS » denominator » denominator-pull-requests #141 SUCCESS |
adding proxy setting support from env to be more like other cli's
didn't added authenticated proxy support since that would be a much more intrusive change..
also i started to add a unit test, but there's no good way (that i know of) to modify the env from java without some extensive hackery - i'm open to suggestions though.