Skip to content
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

Log all upload responses with --verbose #859

Merged
merged 8 commits into from
Jan 24, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog/859.feature.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Log all upload responses with ``--verbose``.
36 changes: 33 additions & 3 deletions tests/test_upload.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,12 @@
def stub_response():
"""Mock successful upload of a package."""
return pretend.stub(
is_redirect=False, status_code=201, raise_for_status=lambda: None
is_redirect=False,
url="https://test.pypi.org/legacy/",
status_code=200,
reason="OK",
text=None,
raise_for_status=lambda: None,
)


Expand Down Expand Up @@ -138,6 +143,25 @@ def test_print_packages_if_verbose(upload_settings, capsys):
assert captured.out.count(f"{filename} ({size})") == 1


def test_print_response_if_verbose(upload_settings, stub_response, capsys):
"""Print details about the response from the repostiry."""
upload_settings.verbose = True

result = upload.upload(
upload_settings,
[helpers.WHEEL_FIXTURE, helpers.SDIST_FIXTURE],
)
assert result is None

captured = capsys.readouterr()
response_log = (
f"Response from {stub_response.url}:\n"
f"{stub_response.status_code} {stub_response.reason}"
)

assert captured.out.count(response_log) == 2


def test_success_with_pre_signed_distribution(upload_settings, stub_repository):
"""Add GPG signature provided by user to uploaded package."""
# Upload a pre-signed distribution
Expand Down Expand Up @@ -186,7 +210,8 @@ def test_exception_for_http_status(verbose, upload_settings, stub_response, caps

stub_response.is_redirect = False
stub_response.status_code = 403
stub_response.text = "Invalid or non-existent authentication information"
stub_response.reason = "Invalid or non-existent authentication information"
stub_response.text = stub_response.reason
stub_response.raise_for_status = pretend.raiser(requests.HTTPError)

with pytest.raises(requests.HTTPError):
Expand Down Expand Up @@ -288,8 +313,11 @@ def test_exception_for_redirect(

stub_response = pretend.stub(
is_redirect=True,
url=redirect_url,
status_code=301,
headers={"location": redirect_url},
reason="Redirect",
text="",
)

stub_repository = pretend.stub(
Expand Down Expand Up @@ -323,7 +351,9 @@ def test_prints_skip_message_for_response(
):
upload_settings.skip_existing = True

stub_response.status_code = 409
stub_response.status_code = 400
stub_response.reason = "File already exists"
stub_response.text = stub_response.reason

# Do the upload, triggering the error response
stub_repository.package_is_uploaded = lambda package: False
Expand Down
5 changes: 1 addition & 4 deletions tests/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -282,10 +282,7 @@ def test_check_status_code_for_missing_status_code(

captured = capsys.readouterr()

if verbose:
assert captured.out.count("Content received from server:\nForbidden\n") == 1
else:
assert captured.out.count("--verbose option") == 1
assert captured.out.count("--verbose option") == 0 if verbose else 1


@pytest.mark.parametrize(
Expand Down
3 changes: 3 additions & 0 deletions twine/commands/upload.py
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,9 @@ def upload(upload_settings: settings.Settings, dists: List[str]) -> None:
continue

resp = repository.upload(package)
logger.info(f"Response from {resp.url}:\n{resp.status_code} {resp.reason}")
if resp.text:
logger.info(resp.text)
Comment on lines +142 to +144
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After dithering on it for awhile, I went back to a simple log message, that honestly feels like it will be redundant in most cases, but potentially useful. Is there other information that should be included at this level of verbosity?

Failure:

Uploading example_pkg_bhrutledge-0.0.5-py3-none-any.whl
Response from https://test.pypi.org/legacy/:
400 File already exists. See https://test.pypi.org/help/#file-name-reuse for more information.
<html>
 <head>
  <title>400 File already exists. See https://test.pypi.org/help/#file-name-reuse for more information.</title>
 </head>
 <body>
  <h1>400 File already exists. See https://test.pypi.org/help/#file-name-reuse for more information.</h1>
  The server could not comply with the request since it is either malformed or otherwise incorrect.<br/><br/>
File already exists. See https://test.pypi.org/help/#file-name-reuse for more information.


 </body>
</html>
HTTPError: 400 Bad Request from https://test.pypi.org/legacy/
File already exists. See https://test.pypi.org/help/#file-name-reuse for more information.

Success:

Uploading example_pkg_bhrutledge-0.0.6.post2-py3-none-any.whl
Response from https://test.pypi.org/legacy/:
200 OK
Uploading example-pkg-bhrutledge-0.0.6.post2.tar.gz
Response from https://test.pypi.org/legacy/:
200 OK

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do still wonder if we need any information (for non-PyPI artifact hosts) regarding redirects. Even if it's a count and not the specific status codes in the history.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sigmavirus24 I wondered about that, but I thought maybe it was moot because redirects result in an error:

# Bug 92. If we get a redirect we should abort because something seems
# funky. The behaviour is not well defined and redirects being issued
# by PyPI should never happen in reality. This should catch malicious
# redirects as well.
if resp.is_redirect:
raise exceptions.RedirectDetected.from_args(
repository_url,
resp.headers["location"],
)

But, I could certainly be wrong. If so, can you recommend some specific attributes of the Response object to log?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, nevermind. That makes sense to me that we've turned off redirects.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we did allow redirects, we could log len(response.history) to get the number of redirects. Alternatively, we could do a ', '.join(str(r.status_code) for r in response.history), etc.


# Bug 92. If we get a redirect we should abort because something seems
# funky. The behaviour is not well defined and redirects being issued
Expand Down
3 changes: 0 additions & 3 deletions twine/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -204,9 +204,6 @@ def check_status_code(response: requests.Response, verbose: bool) -> None:
"Retry with the --verbose option for more details."
)

if response.text:
logger.info("Content received from server:\n{}".format(response.text))

raise err


Expand Down