-
Notifications
You must be signed in to change notification settings - Fork 138
Fix syncing cache behind HTTP proxy #326
Fix syncing cache behind HTTP proxy #326
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
Hi @AlexanderEkdahl. Thanks for your PR. I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
/ok-to-test |
@@ -516,7 +516,9 @@ func (c *KfConfig) SyncCache() error { | |||
return errors.WithStack(err) | |||
} | |||
} else { | |||
t := &http.Transport{} | |||
t := &http.Transport{ | |||
Proxy: http.ProxyFromEnvironment, |
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 there a way to provide a test for this?
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.
This can certainly be tested and the behavior is tested in detail here: https://github.com/golang/net/blob/master/http/httpproxy/proxy_test.go. However, I don't think it is valuable to test this in kfctl as it introduces undue maintenance burden on the kfctl developers.
Using http.ProxyFromEnvironment
is the default behavior for all HTTP request in Go(link) and this PR simply ensures the same behavior as the transport happens to be overridden in kfctl.
Hi @kkasravi , could you please check again if this PR can be merged? |
/retest |
@AlexanderEkdahl: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
@Jeffwan I can't make sense of the apparent test failure. From what I can tell all of the tests passed? |
Can you rebase and run tests? |
/test ? |
@PatrickXYS: The following commands are available to trigger jobs:
Use In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@PatrickXYS: No presubmit jobs available for kubeflow/kfctl@master In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/lgtm Thanks for the contribution |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AlexanderEkdahl, Jeffwan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This mimics the default
RoundTripper
and it useshttp.ProxyFromEnvironment
as directed by the $HTTP_PROXY and $NO_PROXY (or $http_proxy and $no_proxy) environment variables. This allows theTransport
to work behind a proxy.See https://golang.org/pkg/net/http/#Transport and https://golang.org/pkg/net/http/#RoundTripper for more details.
I assume registering custom protocol is due to internal requirements as it is not necessary when downloading files from GitHub.