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.

Sign up for GitHub

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

Jump to bottom

Log all upload responses with --verbose #859

Merged
merged 8 commits into from Jan 24, 2022
Merged

Log all upload responses with --verbose #859

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2cf7138
Log all upload responses with --verbose
bhrutledge Jan 17, 2022
b458254
Use requests_toolbelt to log request/response
bhrutledge Jan 19, 2022
b00659a
Revert "Use requests_toolbelt to log request/response"
bhrutledge Jan 22, 2022
788ce71
Log request details
bhrutledge Jan 22, 2022
d348579
Fix failing tests
bhrutledge Jan 22, 2022
c800a21
Add new test
bhrutledge Jan 22, 2022
a228795
Merge branch 'main' into 856-verbose-response
bhrutledge Jan 22, 2022
fbf4b73
Add changelog entry
bhrutledge Jan 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
36 changes: 33 additions & 3 deletions tests/test_upload.py
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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:

twine/twine/commands/upload.py

Lines 146 to 154 in a228795

# 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
View file
Open in desktop
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