HttpGetter with Basic Auth + tests #34
Conversation
|
hi @mitchellh can you review please? |
|
btw, I can't reproduce the appveyor test errors (TestTarGzipDecompressor and TestZipDecompressor), all tests are passing in my dev env. thanks!! |
There was a problem hiding this comment.
I have some nit picks and a request for documentation. The README needs to be updated so users know this is available, and can learn how to use it.
I assume we're only supporting the format https://url.com?username=username&password=***.
I've also seen https://username:[email protected], is that a supported format here? Should it be? (I honestly don't know if that's a valid format, or maybe it's discouraged)
| @@ -115,6 +144,20 @@ func (g *HttpGetter) GetFile(dst string, u *url.URL) error { | |||
| return err | |||
| } | |||
|
|
|||
| func (g *HttpGetter) parseUrl(u *url.URL) (basicAuthUser, basicAuthPass string) { | |||
There was a problem hiding this comment.
nit pick - this method name is pretty generic/broad but the actual implementation is pretty narrow, we're specifically grabbing the username/password, so a more descriptive name would be better
| @@ -115,6 +144,20 @@ func (g *HttpGetter) GetFile(dst string, u *url.URL) error { | |||
| return err | |||
| } | |||
|
|
|||
| func (g *HttpGetter) parseUrl(u *url.URL) (basicAuthUser, basicAuthPass string) { | |||
| qUser := "http_basic_auth_user" | |||
| qPass := "http_basic_auth_pass" | |||
There was a problem hiding this comment.
We need to document the format for users so that they know how to use this authentication.
There was a problem hiding this comment.
added documentation in README.md
|
@catsby thanks for reviewing! |
|
@catsby ping! can you review it again please? I'd like to do a PR to Nomad if this gets merged. thanks! |
|
I think the changes look good to me, thanks! I defer to @mitchellh to 👍 / 👎 the merge |
|
Hey @adrianlop / @catsby / @mitchellh , happy new year! Might it be possible to push this through so this functionality can make its way into the nomad artifact downloader? |
|
@adrianlop @holtwilkins So go-getter already supports this with normal basic auth formatting: |
closes #32