From b4a196ee452363662adf123b4546754271b23e57 Mon Sep 17 00:00:00 2001 From: Brian Rutledge Date: Sat, 3 Jul 2021 08:55:23 -0400 Subject: [PATCH] Improve handling of missing config file (#770) * Simplify and clarify existing logic * Reindent multiline strings * Extract write_config_file fixture * Raise exception for missing config file * Add changelog entry --- changelog/770.bugfix.rst | 1 + tests/conftest.py | 27 +++++-- tests/test_register.py | 2 +- tests/test_settings.py | 32 +++----- tests/test_upload.py | 10 +-- tests/test_utils.py | 158 +++++++++++++++++---------------------- twine/settings.py | 4 +- twine/utils.py | 95 ++++++++++++----------- 8 files changed, 158 insertions(+), 171 deletions(-) create mode 100644 changelog/770.bugfix.rst diff --git a/changelog/770.bugfix.rst b/changelog/770.bugfix.rst new file mode 100644 index 00000000..f8f33f45 --- /dev/null +++ b/changelog/770.bugfix.rst @@ -0,0 +1 @@ +Improve error message for a missing config file. diff --git a/tests/conftest.py b/tests/conftest.py index 8f02aa16..479b241a 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -4,27 +4,40 @@ import pytest from twine import settings +from twine import utils @pytest.fixture() -def pypirc(tmpdir): - return tmpdir / ".pypirc" +def config_file(tmpdir, monkeypatch): + path = tmpdir / ".pypirc" + # Mimic common case of .pypirc in home directory + monkeypatch.setattr(utils, "DEFAULT_CONFIG_FILE", path) + return path + + +@pytest.fixture +def write_config_file(config_file): + def _write(config): + config_file.write(textwrap.dedent(config)) + return config_file + + return _write @pytest.fixture() -def make_settings(pypirc): +def make_settings(write_config_file): """Return a factory function for settings.Settings with defaults.""" - default_pypirc = """ + default_config = """ [pypi] username:foo password:bar """ - def _settings(pypirc_text=default_pypirc, **settings_kwargs): - pypirc.write(textwrap.dedent(pypirc_text)) + def _settings(config=default_config, **settings_kwargs): + config_file = write_config_file(config) settings_kwargs.setdefault("sign_with", None) - settings_kwargs.setdefault("config_file", str(pypirc)) + settings_kwargs.setdefault("config_file", config_file) return settings.Settings(**settings_kwargs) diff --git a/tests/test_register.py b/tests/test_register.py index d3b421ac..7ceb5b15 100644 --- a/tests/test_register.py +++ b/tests/test_register.py @@ -19,7 +19,7 @@ def register_settings(make_settings): repository: https://test.pypi.org/legacy username:foo password:bar - """ + """ ) diff --git a/tests/test_settings.py b/tests/test_settings.py index ba7102ea..d7c4cdfb 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -14,8 +14,6 @@ # limitations under the License. import argparse import logging -import os.path -import textwrap import pytest @@ -29,22 +27,18 @@ def test_settings_takes_no_positional_arguments(): settings.Settings("a", "b", "c") -def test_settings_transforms_repository_config(tmpdir): +def test_settings_transforms_repository_config(write_config_file): """Set repository config and defaults when .pypirc is provided.""" - pypirc = os.path.join(str(tmpdir), ".pypirc") - - with open(pypirc, "w") as fp: - fp.write( - textwrap.dedent( - """ - [pypi] - repository: https://upload.pypi.org/legacy/ - username:username - password:password + config_file = write_config_file( """ - ) - ) - s = settings.Settings(config_file=pypirc) + [pypi] + repository: https://upload.pypi.org/legacy/ + username:username + password:password + """ + ) + + s = settings.Settings(config_file=config_file) assert s.repository_config["repository"] == "https://upload.pypi.org/legacy/" assert s.sign is False assert s.sign_with == "gpg" @@ -72,16 +66,14 @@ def test_setup_logging(verbose, log_level): "verbose", [True, False], ) -def test_print_config_path_if_verbose(tmpdir, capsys, make_settings, verbose): +def test_print_config_path_if_verbose(config_file, capsys, make_settings, verbose): """Print the location of the .pypirc config used by the user.""" - pypirc = os.path.join(str(tmpdir), ".pypirc") - make_settings(verbose=verbose) captured = capsys.readouterr() if verbose: - assert captured.out == f"Using configuration from {pypirc}\n" + assert captured.out == f"Using configuration from {config_file}\n" else: assert captured.out == "" diff --git a/tests/test_upload.py b/tests/test_upload.py index 7e3ef1d7..465a08cf 100644 --- a/tests/test_upload.py +++ b/tests/test_upload.py @@ -203,14 +203,14 @@ def test_exception_for_http_status(verbose, upload_settings, stub_response, caps assert "--verbose" in captured.out -def test_get_config_old_format(make_settings, pypirc): +def test_get_config_old_format(make_settings, config_file): try: make_settings( """ [server-login] username:foo password:bar - """ + """ ) except KeyError as err: assert all( @@ -218,7 +218,7 @@ def test_get_config_old_format(make_settings, pypirc): for text in [ "'pypi'", "--repository-url", - pypirc, + config_file, "https://docs.python.org/", ] ) @@ -232,7 +232,7 @@ def test_deprecated_repo(make_settings): repository: https://pypi.python.org/pypi/ username:foo password:bar - """ + """ ) upload.upload(upload_settings, [helpers.WHEEL_FIXTURE]) @@ -257,7 +257,7 @@ def test_exception_for_redirect(make_settings): repository: https://test.pypi.org/legacy username:foo password:bar - """ + """ ) stub_response = pretend.stub( diff --git a/tests/test_utils.py b/tests/test_utils.py index 093649b4..903155f9 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -13,7 +13,6 @@ # limitations under the License. import os.path -import textwrap import pretend import pytest @@ -25,24 +24,19 @@ from . import helpers -def test_get_config(tmpdir): - pypirc = os.path.join(str(tmpdir), ".pypirc") - - with open(pypirc, "w") as fp: - fp.write( - textwrap.dedent( - """ - [distutils] - index-servers = pypi +def test_get_config(write_config_file): + config_file = write_config_file( + """ + [distutils] + index-servers = pypi - [pypi] - username = testuser - password = testpassword + [pypi] + username = testuser + password = testpassword """ - ) - ) + ) - assert utils.get_config(pypirc) == { + assert utils.get_config(config_file) == { "pypi": { "repository": utils.DEFAULT_REPOSITORY, "username": "testuser", @@ -51,22 +45,17 @@ def test_get_config(tmpdir): } -def test_get_config_no_distutils(tmpdir): +def test_get_config_no_distutils(write_config_file): """Upload by default to PyPI if an index server is not set in .pypirc.""" - pypirc = os.path.join(str(tmpdir), ".pypirc") - - with open(pypirc, "w") as fp: - fp.write( - textwrap.dedent( - """ - [pypi] - username = testuser - password = testpassword + config_file = write_config_file( """ - ) - ) + [pypi] + username = testuser + password = testpassword + """ + ) - assert utils.get_config(pypirc) == { + assert utils.get_config(config_file) == { "pypi": { "repository": utils.DEFAULT_REPOSITORY, "username": "testuser", @@ -80,24 +69,19 @@ def test_get_config_no_distutils(tmpdir): } -def test_get_config_no_section(tmpdir): - pypirc = os.path.join(str(tmpdir), ".pypirc") - - with open(pypirc, "w") as fp: - fp.write( - textwrap.dedent( - """ - [distutils] - index-servers = pypi foo +def test_get_config_no_section(write_config_file): + config_file = write_config_file( + """ + [distutils] + index-servers = pypi foo - [pypi] - username = testuser - password = testpassword + [pypi] + username = testuser + password = testpassword """ - ) - ) + ) - assert utils.get_config(pypirc) == { + assert utils.get_config(config_file) == { "pypi": { "repository": utils.DEFAULT_REPOSITORY, "username": "testuser", @@ -106,26 +90,19 @@ def test_get_config_no_section(tmpdir): } -def test_get_config_override_pypi_url(tmpdir): - pypirc = os.path.join(str(tmpdir), ".pypirc") - - with open(pypirc, "w") as fp: - fp.write( - textwrap.dedent( - """ - [pypi] - repository = http://pypiproxy +def test_get_config_override_pypi_url(write_config_file): + config_file = write_config_file( """ - ) - ) - - assert utils.get_config(pypirc)["pypi"]["repository"] == "http://pypiproxy" + [pypi] + repository = http://pypiproxy + """ + ) + assert utils.get_config(config_file)["pypi"]["repository"] == "http://pypiproxy" -def test_get_config_missing(tmpdir): - pypirc = os.path.join(str(tmpdir), ".pypirc") - assert utils.get_config(pypirc) == { +def test_get_config_missing(config_file): + assert utils.get_config(config_file) == { "pypi": { "repository": utils.DEFAULT_REPOSITORY, "username": None, @@ -139,44 +116,38 @@ def test_get_config_missing(tmpdir): } -def test_empty_userpass(tmpdir): +def test_empty_userpass(write_config_file): """Suppress prompts if empty username and password are provided in .pypirc.""" - pypirc = os.path.join(str(tmpdir), ".pypirc") - - with open(pypirc, "w") as fp: - fp.write( - textwrap.dedent( - """ - [pypi] - username= - password= + config_file = write_config_file( """ - ) - ) + [pypi] + username= + password= + """ + ) - config = utils.get_config(pypirc) + config = utils.get_config(config_file) pypi = config["pypi"] assert pypi["username"] == pypi["password"] == "" -def test_get_repository_config_missing(tmpdir): - pypirc = os.path.join(str(tmpdir), ".pypirc") - +def test_get_repository_config_missing(config_file): repository_url = "https://notexisting.python.org/pypi" exp = { "repository": repository_url, "username": None, "password": None, } - assert utils.get_repository_from_config(pypirc, "foo", repository_url) == exp - assert utils.get_repository_from_config(pypirc, "pypi", repository_url) == exp + assert utils.get_repository_from_config(config_file, "foo", repository_url) == exp + assert utils.get_repository_from_config(config_file, "pypi", repository_url) == exp + exp = { "repository": utils.DEFAULT_REPOSITORY, "username": None, "password": None, } - assert utils.get_repository_from_config(pypirc, "pypi") == exp + assert utils.get_repository_from_config(config_file, "pypi") == exp @pytest.mark.parametrize( @@ -191,22 +162,33 @@ def test_get_repository_config_missing(tmpdir): ("foo.bar", "host, scheme were required but missing."), ], ) -def test_get_repository_config_with_invalid_url(tmpdir, repo_url, message): +def test_get_repository_config_with_invalid_url(config_file, repo_url, message): """Raise an exception for a URL with an invalid/missing scheme and/or host.""" - pypirc = os.path.join(str(tmpdir), ".pypirc") - with pytest.raises( exceptions.UnreachableRepositoryURLDetected, match=message, ): - utils.get_repository_from_config(pypirc, "pypi", repo_url) + utils.get_repository_from_config(config_file, "pypi", repo_url) + +def test_get_repository_config_missing_repository(write_config_file): + """Raise an exception when a custom repository isn't defined in .pypirc.""" + config_file = write_config_file("") + with pytest.raises( + exceptions.InvalidConfiguration, + match="Missing 'missing-repository'", + ): + utils.get_repository_from_config(config_file, "missing-repository") -def test_get_repository_config_missing_config(tmpdir): - """Raise an exception when a repository isn't defined in .pypirc.""" - pypirc = os.path.join(str(tmpdir), ".pypirc") - with pytest.raises(exceptions.InvalidConfiguration): - utils.get_repository_from_config(pypirc, "foobar") + +@pytest.mark.parametrize("repository", ["pypi", "missing-repository"]) +def test_get_repository_config_missing_file(repository): + """Raise an exception when a custom config file doesn't exist.""" + with pytest.raises( + exceptions.InvalidConfiguration, + match=r"No such file.*missing-file", + ): + utils.get_repository_from_config("missing-file", repository) def test_get_config_deprecated_pypirc(): diff --git a/twine/settings.py b/twine/settings.py index 2fe4f897..fe2f26ce 100644 --- a/twine/settings.py +++ b/twine/settings.py @@ -52,7 +52,7 @@ def __init__( password: Optional[str] = None, non_interactive: bool = False, comment: Optional[str] = None, - config_file: str = "~/.pypirc", + config_file: str = utils.DEFAULT_CONFIG_FILE, skip_existing: bool = False, cacert: Optional[str] = None, client_cert: Optional[str] = None, @@ -244,7 +244,7 @@ def register_argparse_arguments(parser: argparse.ArgumentParser) -> None: ) parser.add_argument( "--config-file", - default="~/.pypirc", + default=utils.DEFAULT_CONFIG_FILE, help="The .pypirc config file to use.", ) parser.add_argument( diff --git a/twine/utils.py b/twine/utils.py index 7b22ae29..e5bdb4b5 100644 --- a/twine/utils.py +++ b/twine/utils.py @@ -33,6 +33,8 @@ DEFAULT_REPOSITORY = "https://upload.pypi.org/legacy/" TEST_REPOSITORY = "https://test.pypi.org/legacy/" +DEFAULT_CONFIG_FILE = "~/.pypirc" + # TODO: In general, it seems to be assumed that the values retrieved from # instances of this type aren't None, except for username and password. # Type annotations would be cleaner if this were Dict[str, str], but that @@ -43,46 +45,45 @@ logger = logging.getLogger(__name__) -def get_config(path: str = "~/.pypirc") -> Dict[str, RepositoryConfig]: - # even if the config file does not exist, set up the parser - # variable to reduce the number of if/else statements - parser = configparser.RawConfigParser() - - # this list will only be used if index-servers - # is not defined in the config file - index_servers = ["pypi", "testpypi"] - - # default configuration for each repository - defaults: RepositoryConfig = {"username": None, "password": None} - - # Expand user strings in the path - path = os.path.expanduser(path) - - logger.info(f"Using configuration from {path}") - - # Parse the rc file - if os.path.isfile(path): - parser.read(path) - - # Get a list of index_servers from the config file - # format: https://packaging.python.org/specifications/pypirc/ - if parser.has_option("distutils", "index-servers"): - index_servers = parser.get("distutils", "index-servers").split() +def get_config(path: str) -> Dict[str, RepositoryConfig]: + """Read repository configuration from a file (i.e. ~/.pypirc). - for key in ["username", "password"]: - if parser.has_option("server-login", key): - defaults[key] = parser.get("server-login", key) + Format: https://packaging.python.org/specifications/pypirc/ - config: DefaultDict[str, RepositoryConfig] = collections.defaultdict( - lambda: defaults.copy() - ) + If the default config file doesn't exist, return a default configuration for + pypyi and testpypi. + """ + realpath = os.path.realpath(os.path.expanduser(path)) + parser = configparser.RawConfigParser() - # don't require users to manually configure URLs for these repositories + try: + with open(realpath) as f: + parser.read_file(f) + logger.info(f"Using configuration from {realpath}") + except FileNotFoundError: + # User probably set --config-file, but the file can't be read + if path != DEFAULT_CONFIG_FILE: + raise + + # server-login is obsolete, but retained for backwards compatibility + defaults: RepositoryConfig = { + "username": parser.get("server-login", "username", fallback=None), + "password": parser.get("server-login", "password", fallback=None), + } + + config: DefaultDict[str, RepositoryConfig] + config = collections.defaultdict(lambda: defaults.copy()) + + index_servers = parser.get( + "distutils", "index-servers", fallback="pypi testpypi" + ).split() + + # Don't require users to manually configure URLs for these repositories config["pypi"]["repository"] = DEFAULT_REPOSITORY if "testpypi" in index_servers: config["testpypi"]["repository"] = TEST_REPOSITORY - # optional configuration values for individual repositories + # Optional configuration values for individual repositories for repository in index_servers: for key in [ "username", @@ -94,8 +95,7 @@ def get_config(path: str = "~/.pypirc") -> Dict[str, RepositoryConfig]: if parser.has_option(repository, key): config[repository][key] = parser.get(repository, key) - # convert the defaultdict to a regular dict at this point - # to prevent surprising behavior later on + # Convert the defaultdict to a regular dict to prevent surprising behavior later on return dict(config) @@ -117,30 +117,29 @@ def _validate_repository_url(repository_url: str) -> None: def get_repository_from_config( - config_file: str, repository: str, repository_url: Optional[str] = None + config_file: str, + repository: str, + repository_url: Optional[str] = None, ) -> RepositoryConfig: - # Get our config from, if provided, command-line values for the - # repository name and URL, or the .pypirc file - + """Get repository config command-line values or the .pypirc file.""" + # Prefer CLI `repository_url` over `repository` or .pypirc if repository_url: _validate_repository_url(repository_url) - # prefer CLI `repository_url` over `repository` or .pypirc return { "repository": repository_url, "username": None, "password": None, } + try: return get_config(config_file)[repository] + except OSError as exc: + raise exceptions.InvalidConfiguration(str(exc)) except KeyError: - msg = ( - "Missing '{repo}' section from the configuration file\n" - "or not a complete URL in --repository-url.\n" - "Maybe you have an out-dated '{cfg}' format?\n" - "more info: " - "https://packaging.python.org/specifications/pypirc/\n" - ).format(repo=repository, cfg=config_file) - raise exceptions.InvalidConfiguration(msg) + raise exceptions.InvalidConfiguration( + f"Missing '{repository}' section from {config_file}.\n" + f"More info: https://packaging.python.org/specifications/pypirc/ " + ) _HOSTNAMES = {