Fix a silent upload error#1310
Conversation
Fixes python-poetry#858 by first issuing a HEAD request and following a redirect. This does not cover further redirects though, so it's possible to make this more robust. It does cover the most common use cases (PyPI and TestPyPI will work correctly)
sztomi
force-pushed
the
fix_silent_upload_fail
branch
from
8c25b2e to
3d7ce0e
Compare
|
Unfortunately I do have a failing unit test and I don't know how to fix it. This test fails: The output gets unexpected whitespace, but I have no idea why. Any help is appreciated.
|
brycedrennan
added
kind/bug
brycedrennan
left a comment
brycedrennan
left a comment
There was a problem hiding this comment.
Thanks for contributing to poetry!
Do you think we could change it to manually follow redirects without needing to make two calls every time?
Alternatively, could we just output a meaningful error message and let the user fix the url?
| - Uploading simple-project-1.2.3.tar.gz 0% | ||
| - Uploading simple-project-1.2.3.tar.gz 100% | ||
| - Uploading simple-project-1.2.3.tar.gz 100% | ||
There was a problem hiding this comment.
Looks like you removed whitespace here - was that intentional? Might be related to the test failure.
There was a problem hiding this comment.
oh, I totally did. Don't know why I didn't notice that. But it explains my test failure. I'll correct that.
|
|
||
| # Check for a redirect first | ||
| resp = session.head(url) | ||
| if resp.status_code == status_codes.codes.moved: |
There was a problem hiding this comment.
This approach means we'd have to make two requests every time, even when there is no redirect. Wouldn't we be able to just try the endpoint and then follow the redirect if necessary?
There was a problem hiding this comment.
That's what I did originally, but that means POSTing the package data twice, so anyone who configured the wrong URL for their repo will have a big perf hit. On the other hand, a HEAD request is very cheap and isn't noticeable. Happy to do some benchmarks if you are not convinced.
There was a problem hiding this comment.
That's a good point that it would be expensive to POST twice.
|
Actually, I realized that 301 means permanently moved. So I'm going to open a different PR to follow the redirect at the time it is saved in the config. |
|
Superseded by the linked PR above ^ |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
2 participants
Fixes #858 by first issuing a HEAD request and following a redirect.
This does not cover further redirects though, so it's possible to make this more robust. It does cover the most common use cases (PyPI and TestPyPI will work correctly)
See my comment for more details: #858 (comment)
Pull Request Check List