stack upload uses HTTP digest authentication #2584
Conversation
|
I'll be pushing a new commit shortly, once http-client-tls-0.3.1 is available on the indices. |
79ca553 to
b24424e
Compare
|
stack-8.0.yaml needs an extra-dep—Travis gives |
|
Ugh, I missed one! Hold on... |
b24424e to
f2db579
Compare
|
I've just pushed another commit which bumps to http-client-0.5.3.1, due to: snoyberg/http-client#225. |
| @@ -187,7 +187,7 @@ library | |||
| , hit | |||
| , hpc | |||
| , http-client >= 0.5.0 | |||
There was a problem hiding this comment.
Strictly speaking, shouldn't d52c9bc also increase the cabal bound to http-client >= 0.5.3.1?
There was a problem hiding this comment.
Yeah, it's a good idea.
Without this, using stack upload with digest support triggers snoyberg/http-client#225. This change could arguably be made in a separate PR, but including with digest support since it would be a very dangerous idea to not bump the http-client version.
d52c9bc to
dd795ae
Compare
| putStr $ "Uploading " ++ tarName ++ "... " | ||
| hFlush stdout | ||
| withResponse req3 manager $ \res -> | ||
| withResponse (fromMaybe req2 mreq3) manager $ \res -> |
There was a problem hiding this comment.
As long as we handle applyDigestAuth failing, I'd prefer to have it logged, with sth. better than:
when (isNull mreq3)
$logWarn "Failure in applyDigestAuth"So if a submission without password fails, there's a chance to realize that's because digest authentication failed, not because we didn't try authentication.
BTW sorry for thinking about this now, I realize multiple reviews are annoying 😅
There was a problem hiding this comment.
I don't think it's necessary. If mreq3 is Nothing, then we're going to retry the same request again, and when it (almost) inevitably fails, we'll display the error message from the server to the user.
There was a problem hiding this comment.
I see that; my point is, I'd expect the error would be misleading. If anything hinted at "no password provided", I'd wonder why stack isn't providing a password, not why Hackage is rejecting digest authentication. I'm not implying some hypothetical user would be confused, I'm implying I would be confused.
There was a problem hiding this comment.
Then I'm not sure what message would be better than the proposed "Failure in applyDigestAuth."
There was a problem hiding this comment.
That one's fine then!
|
Other than this comment, LGTM. I'll leave merging to you 😉 |
No description provided.