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

Having a new check to avoid redirects from PyPI #809

Closed
wants to merge 7 commits into from
Closed

Having a new check to avoid redirects from PyPI #809

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
776712a
Having a new check to avoid redirects from PyPI
dukecat0 Sep 12, 2021
dcec394
Add a link to docs of .pypirc
dukecat0 Sep 12, 2021
a806125
Fix linting error
dukecat0 Sep 12, 2021
074d5f1
Rewrite logic and message
bhrutledge Sep 12, 2021
651853a
Tweak message
bhrutledge Sep 12, 2021
414625b
Update tests
bhrutledge Sep 14, 2021
4d27ee5
Use safer URL parsing
bhrutledge Sep 14, 2021
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
19 changes: 18 additions & 1 deletion twine/settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,10 +324,12 @@ def _handle_certificates(
self.client_cert = utils.get_clientcert(client_cert, self.repository_config)

def check_repository_url(self) -> None:
"""Verify we are not using legacy PyPI.
"""Verify we are not using legacy PyPI and using the correct url for PyPI.

:raises:
:class:`~twine.exceptions.UploadToDeprecatedPyPIDetected`
:raises:
:class:`~twine.exceptions.InvalidPyPIUploadURL`
"""
repository_url = cast(str, self.repository_config["repository"])

Expand All @@ -338,6 +340,21 @@ def check_repository_url(self) -> None:
repository_url, utils.DEFAULT_REPOSITORY, utils.TEST_REPOSITORY
)

if repository_url.startswith(
("https://pypi.org/", "https://test.pypi.org/", "https://upload.pypi.org/")
) and repository_url not in [utils.DEFAULT_REPOSITORY, utils.TEST_REPOSITORY]:
raise exceptions.InvalidPyPIUploadURL(
f"The configured repository {repository_url} is not a known "
f"PyPI repository.\n"
f"Did you mean {utils.DEFAULT_REPOSITORY} "
f"or {utils.TEST_REPOSITORY} ?\n"
f"Check your --repository-url value or "
f"modify the url in {self.config_file} "
f"(if you are using -r or --repository),\n"
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the parenthetical is necessary or helpful as written

Copy link
Member Author

@dukecat0 dukecat0 Sep 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Using --repository-url https://test.pypi.org/legacy will also raise InvalidPyPIUploadURL. Probably folks who use --repository-url won't know what is ~/.pypirc and makes confusion to them?

f"so twine can upload your distributions properly.\n"
f"See {utils.PYPIRC_DOCS} for more details."
)

Copy link
Contributor

Choose a reason for hiding this comment

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

I fixed the failing test and adding coverage for the new behavior in 414625b, but looking at the drop in coverage via Codecov emphasized my point in #809 (comment), i.e. that this is similar to utils.check_status_code.

Taking a closer look at that, plus the comments in #310, plus my initial attempt in #460 to offer a better error message, I'm now wondering if it's needlessly redundant, with the primary difference being that this executes before an upload, while check_status_code executes afterwards. Instead, maybe we could improve the post-upload RedirectDetected message by tweaking the language, possibly with a special message for a trailing slash, regardless of whether or not it's a pypi.org hostname.

In other words, this feels like a case of "Look Before You Leap" vs "It's Easter to Ask Forgiveness than Permission", and with this PR, I think we're doing both. I'm going to mull this over a bit, and maybe put up another PR that improves the EAFP approach.

@meowmeowmeowcat Apologies for the confusion here; I should have taken a closer look at the existing behavior.

Copy link
Member Author

@dukecat0 dukecat0 Sep 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@bhrutledge I think the improved message in #812 would be also helpful:

RedirectDetected: https://upload.pypi.org/legacy attempted to redirect to https://upload.pypi.org/legacy/.
Your repository URL is missing a trailing slash. Please add it and try again.

But I also think that having this check will save folks' time. They will know there is an error in the config or --repository-url before twine tries to upload their distributions, although this check is similar to utils.check_status_code.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's true that it will save a little bit of time, but I'm not sure it's worth the added complexity and technical debt of duplicated logic. It's also not as robust as the repository-independent implementation in #812. Furthermore, if PyPI ever supports a different URL, then this implementation will block people from uploading to it at all.

Copy link
Member Author

@dukecat0 dukecat0 Sep 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I see. Probably this PR can be closed? The improved message in #812 is enough.

Copy link
Contributor

@bhrutledge bhrutledge Sep 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I'd like to get @sigmavirus24's feedback on #812 first.

def create_repository(self) -> repository.Repository:
"""Create a new repository for uploading."""
repo = repository.Repository(
Expand Down
1 change: 1 addition & 0 deletions twine/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@

DEFAULT_REPOSITORY = "https://upload.pypi.org/legacy/"
TEST_REPOSITORY = "https://test.pypi.org/legacy/"
PYPIRC_DOCS = "https://packaging.python.org/specifications/pypirc/"

DEFAULT_CONFIG_FILE = "~/.pypirc"

Expand Down