New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Print the response content when upload fails #337

Merged
merged 1 commit into from Apr 28, 2018

Conversation

Projects
None yet
4 participants
@jessejoe
Contributor

jessejoe commented Apr 5, 2018

Recently we hit an issue using twine to upload to artifactory where uploads would fail with a suspicious HTTPError: 400 Client Error: Bad Request for url. This happened to coincide very close to SSL certificate changes, so we were debugging the certs, debugging the docker images, pretty much everything except twine itself. After many dead-ends I thought I would debug the HTTP response that twine is getting from the artifactory server where our internal PyPi repo is. This finally gave me an error message from the response that I could look into:

ipdb> print(response.text)
{
  "errors" : [ {
    "status" : 400,
    "message" : "No enum constant org.jfrog.repomd.pypi.model.PypiMetadata.MetadataVersion.v2_1"
  } ]
}

This led me to https://www.jfrog.com/jira/browse/RTFACT-16189 which is a bug in artifactory not being able to handle packages built with newer versions of wheel/setuptools/twine that have metadata increased to v2.1.

Had the content from the server been in the output from twine, it would have saved so much time. It's very common for servers to reply with error messages in the content, so it seems this could help lots of debugging out there.

I'm not positive this is the best place to put the change. My change catches the requests exception from raise_for_status(), prints the response content, and then re-raises the exception. I'm happy to change this if another approach is better.

Here is the output from twine without my change:

Uploading distributions to https://xxx/artifactory/api/pypi/pypi
Uploading xxx-2.3.0.post117+g0206843-py2.py3-none-any.whl
100%|███████████████████████████████████████████████████████████████████████████████████████| 810k/810k [00:00<00:00, 1.92MB/s]
HTTPError: 400 Client Error: Bad Request for url: https://xxx/artifactory/api/pypi/pypi

Here is the output with my change:

Uploading distributions to https://xxx/artifactory/api/pypi/pypi
Uploading xxx-2.3.0.post117+g0206843-py2.py3-none-any.whl
100%|███████████████████████████████████████████████████████████████████████████████████████| 810k/810k [00:00<00:00, 1.84MB/s]
Content received from server:
{
  "errors" : [ {
    "status" : 400,
    "message" : "No enum constant org.jfrog.repomd.pypi.model.PypiMetadata.MetadataVersion.v2_1"
  } ]
}
HTTPError: 400 Client Error: Bad Request for url: https://xxx/artifactory/api/pypi/pypi

UPDATE

See #issuecomment-381730323 for updated changes to this PR.

@codecov

This comment has been minimized.

codecov bot commented Apr 5, 2018

Codecov Report

Merging #337 into master will decrease coverage by 0.71%.
The diff coverage is 25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #337      +/-   ##
==========================================
- Coverage   69.55%   68.84%   -0.72%     
==========================================
  Files          12       12              
  Lines         588      597       +9     
  Branches       93       95       +2     
==========================================
+ Hits          409      411       +2     
- Misses        150      157       +7     
  Partials       29       29
Impacted Files Coverage Δ
twine/utils.py 80.7% <20%> (-5.15%) ⬇️
twine/commands/upload.py 65.88% <50%> (+0.4%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dcc6375...2fcac82. Read the comment docs.

@brainwane brainwane requested a review from sigmavirus24 Apr 13, 2018

@brainwane

This comment has been minimized.

Member

brainwane commented Apr 13, 2018

This looks reasonably good to me - Ian?

@pradyunsg

This comment has been minimized.

Member

pradyunsg commented Apr 14, 2018

I'll comment on the UX of it, not the code...

While I do agree the message should come from the server, I'm not sure if the response text should just be printed as is. Basically, if we're printing a json object, it doesn't look clean.

Maybe twine should try to parse the contents of the response assuming it is formatted how it currently is and extract the error message and code from it. Failing that, just print whatever was the response.

@jessejoe

This comment has been minimized.

Contributor

jessejoe commented Apr 14, 2018

@pradyunsg I think that would only be possible if we can guarantee that every PyPI server would return errors formatted the exact same way. Webservers and APIs in general can return multiple types of errors; some may return JSON, some may return HTML, some may return nothing. Unless all PyPI servers always return JSON and always return the same object format of {"errors" : [ {"status" : XXX, "message" : "FOO"} ]}, and always the same for any type of HTTP error (4XX and 5XX), then I think it's safer to just print the response text as is, so the user has a clue of what the issue might be without filtering anything.

@pradyunsg

This comment has been minimized.

Member

pradyunsg commented Apr 14, 2018

To be clear, I'm fine with this PR going in as is. I'm just trying to improve the UX of it - happier if it's cleaner.

You are correct. Until we have a standard format, likely specified by a PEP, we can't reasonably expect that the response will take this format - this is why I'm suggesting falling back to printing the message as is if there's a different format.

I don't think we have lose anything with this - one (common?) case improves, the rest of them stay as is. We increase the code complexity a bit for it.

@sigmavirus24

This comment has been minimized.

Member

sigmavirus24 commented Apr 14, 2018

@pradyunsg I don't think Twine can parse a response body. However, I have exactly the same concerns as you. If someone sends this to a random server, that responds with an unruly and large HTML body, their terminal will be flooded with nonsense that's unhelpful. It may be wise to hide this behind a --verbose flag. What do you think @brainwane @jessejoe?

@jessejoe

This comment has been minimized.

Contributor

jessejoe commented Apr 14, 2018

@sigmavirus24 I think --verbose is a good middle ground and relatively common CLI model to use. I think it's still possible that people will be unaware there are possible errors from the server hiding behind a flag, but you're right that it's not good to flood a terminal with HTML garbage. I'll update the PR.

@jessejoe jessejoe force-pushed the jessejoe:print_upload_error_content branch 3 times, most recently from 7255911 to fd352b4 Apr 16, 2018

@jessejoe

This comment has been minimized.

Contributor

jessejoe commented Apr 16, 2018

@sigmavirus24 @pradyunsg @brainwane I've updated my commit to go the --verbose route. 2fcac82 adds a --verbose option to the upload CLI argument and function, which will include the server response content when an upload fails with an HTTP error from the server:

$ twine upload --verbose dist/*
Uploading distributions to https://xxx/artifactory/api/pypi/pypi
Uploading xxx-2.4.0.post11+ge9d8c5c-py2.py3-none-any.whl
100%|█████████████████████████████████████| 831k/831k [00:00<00:00, 2.14MB/s]
Content received from server:
{
  "errors" : [ {
    "status" : 400,
    "message" : "No enum constant org.jfrog.repomd.pypi.model.PypiMetadata.MetadataVersion.v2_1"
  } ]
}
HTTPError: 400 Client Error: Bad Request for url: https://xxx/artifactory/api/pypi/pypi

I also added a little breadcrumb where if the upload fails with an HTTP error, but --verbose hasn't been provided, AND there is content from the server in the response it will suggest using --verbose:

$ twine upload dist/*
Uploading distributions to https://xxx/artifactory/api/pypi/pypi
Uploading xxx-2.4.0.post11+ge9d8c5c-py2.py3-none-any.whl
100%|█████████████████████████████████████| 831k/831k [00:00<00:00, 2.16MB/s]
NOTE: Try --verbose to see response content.
HTTPError: 400 Client Error: Bad Request for url: https://xxx/artifactory/api/pypi/pypi

Note: I also had to update 2 tests to comply with the new signature for the upload() function. I could make verbose an optional param by setting it to a keyword argument which defaults to False, but I did not since no other parameters were written this way.

Let me know if you have any other suggestions.

@jessejoe jessejoe force-pushed the jessejoe:print_upload_error_content branch from fd352b4 to 4525c47 Apr 16, 2018

Add option to print the response content when upload fails
The content will often contain an actual error message from the server, while
the status code and reason may not. This change adds a `--verbose` option to the
`upload` positional argument which will print the response content if an upload
fails with an HTTP error.

@jessejoe jessejoe force-pushed the jessejoe:print_upload_error_content branch from 4525c47 to 2fcac82 Apr 16, 2018

@pradyunsg

Looks good to me, functionally. :)

@sigmavirus24

This comment has been minimized.

Member

sigmavirus24 commented Apr 18, 2018

The change looks good. It'd be good to have tests but I'll leave that decision in your hands @brainwane

@jessejoe

This comment has been minimized.

Contributor

jessejoe commented Apr 18, 2018

@sigmavirus24 I thought about adding a test for this, but there are a few issues I ran into:

  1. In order to test upload(), you'd have to actually attempt to upload a package and mock a PyPI server to return an HTTP error, this would be pretty difficult
  2. We could test check_status_code() (where the main change actually is) with a mock requests response object (which is simple), however since the change is just adding print() output, it's actually more difficult to verify that - without using tricks to capture stdout. This is possible, but kind of hacky IMO.

Let me know if anyone has a better idea of a good test for this, and I'd be happy to add it.

@sigmavirus24 sigmavirus24 merged commit f5ca338 into pypa:master Apr 28, 2018

1 of 3 checks passed

codecov/patch 25% of diff hit (target 69.55%)
Details
codecov/project 68.84% (-0.72%) compared to dcc6375
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sigmavirus24

This comment has been minimized.

Member

sigmavirus24 commented Apr 28, 2018

Thanks @jessejoe!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment