Adding fix for and tests for no proxy support for deployer #729
Conversation
|
|
| if err != nil { | ||
| return errorutils.CheckError(err) | ||
| } | ||
| if deployProxyURL != nil { |
There was a problem hiding this comment.
There's a chance I'm missing something here, but it looks like deployProxyURL is used only for the debug logging. My guess is that your intention was to extract the host and the post values from deployProxyURL before adding them to the properties file.
If this is the case, I suggest simplifying the implementation of this function and doing it once, instead of twice. In other words, we can use the http.ProxyFromEnvironment API regardless of whether noProxy is defined or not.
Notice that the tests are also wrong, because when they set no_proxy, the no_proxy value is identical to the http_proxy value, and that's why no host and port are added to the properties file.
The tests should use no_proxy values with wildcards, and check that the no proxy filtering works properly.
There was a problem hiding this comment.
I agree that this could be simplified.
DeployURLProxy is used as a logic operation to determine if the proxy should be set or not depending on the environment’s No_Proxy rules. The key element here is the introduction of the http.ProxyFromEnvironment API which has a robust “NoProxy” logic in that if it returns nil, nil then you should not set the proxy flag and if it returns nil, <proxy_url> then you should set the proxy flag to that URL.
As you suggest I could effectively pull most of this code out (I tried to keep the flow of the original code) and replace it with http.ProxyFromEnvironment. I think it also was this way due to the fact that I originally thought that it mattered if we set it for Deploy vs Resolve but after digging in I realized that Gradle handles that part itself.
There was a problem hiding this comment.
As for the tests there are two sets of tests. One set where the No Proxy is identical and one where they differ.
There was a problem hiding this comment.
We could add tests that test the full gambit of no_proxy rules, I figured that was best left to the http.ProxyFromEnvironment team. Instead, I simply wanted a test that proved if the proxy was set or not depending on the variables we passed
| createSimplePropertiesFile(t, propertiesFileConfig) | ||
| setProxy(proxyOrg, t) | ||
| setNoProxy(noproxyOrg, t) | ||
| } |
There was a problem hiding this comment.
Here is a test where the proxy matches the noproxy
| setProxy(proxyOrg, t) | ||
| setNoProxy(noproxyOrg, t) | ||
| } | ||
|
|
There was a problem hiding this comment.
This test has the proxy and the no proxy not matching and should return proxy/port
|
Thanks @Enquier. Let me know after you add the changes and would like me to review this PR again. |
|
Hi @Enquier, |
|
@eyalbe4 ive been tied up deploying Eplus internally I’ll take a look to see if I can resurrect this |
|
This pull-request was closed due to 7 days of inactivity after being marked as stale. Feel free to reopen it if it remains relevant. |
4 participants
I signed JFrog's CLA. This is a dead link btw
All tests passed. If this feature is not already covered by the tests, I added new tests.
This pull request is on the dev branch.
I used gofmt for formatting the code before submitting the pull request.
This is a quick fix for #728 . I used the resolveProxyUrl from http to check if the deployer repo needs the proxy or not. Tests are added. There is no setting for resolver proxy in the artifactory gradle plugin. The proxy for resolver is handled by gradle/system so we only have control over the deployer proxy opts in this state. Previously this would only look at the proxy env settings and just blindly set the proxy