Remove custom http transport in git resolver #1231
Conversation
tomkennedy513
requested review from
matthewmcnew,
chenbh and
pviraj59
as code owners
- The default http transport will handle proxy logic so there isnt a need for us to create our own transport
tomkennedy513
force-pushed
the
remote-got-resolver-init
branch
from
493be17 to
cf11558
Compare
tomkennedy513
changed the title
| it("uses the http proxy env vars", func() { | ||
| require.NoError(t, os.Setenv("HTTPS_PROXY", "http://invalid-proxy")) | ||
| defer os.Unsetenv("HTTPS_PROXY") | ||
|
|
||
| err := fetcher.Fetch(testDir, "https://github.com/git-fixtures/basic", "master", metadataDir) | ||
| require.Error(t, err) | ||
| require.Contains(t, err.Error(), "proxyconnect tcp: dial tcp") | ||
| }) |
There was a problem hiding this comment.
Were you able to verify that HTTPS_PROXY is supported out of the box? I still think it's valuable to test it, but I can't get this test to work.
There was a problem hiding this comment.
Yeah I ran a proxy locally and saw that it was attempting to reach git through the proxy. I think the problem with the tests is that they're running in parallel and attempting to use a shared environment variable. I personally don't see a ton of value on this test because we are basically testing the go http proxy package for this particular use case and not any of the other instances where kpack may need to utilize a proxy with the same default transport (i.e. registry access)
There was a problem hiding this comment.
If you focus that test with the changes in the PR then you it will use the proxy.
There was a problem hiding this comment.
The main reason I wanted to test it is that the proxy logic is hidden under so many layers of abstraction that there's no way of knowing it's even a thing if it's not explicitly called out in the docs or code.
There was a problem hiding this comment.
There might be a conversation for a integration test instead of unit, but as long as it was at least tested manually I'm fine with this
The default http transport will handle proxy logic so there isnt a need for us to create our own transport