From 46c015375da47327e2e0bdf740b17011e1d33f14 Mon Sep 17 00:00:00 2001 From: Abhijit Menon-Sen Date: Thu, 30 Jul 2015 10:45:23 +0530 Subject: [PATCH] Clarify HAVE_PYTHON_APT/install_python_apt handling in apt_repository MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. Don't test check_mode in both the caller and in the callee. 2. Don't test HAVE_PYTHON_APT inside an if that tests HAVE_PYTHON_APT 3. Don't be irritatingly vague about why the module fails ("You may be seeing this because…"). Note that if «apt-get -y install python-apt» succeeds with rc==0, but for some reason python_apt is not usable afterwards, this will break because the imports in install_python_apt aren't wrapped inside a try/except. In other words, we assume that install_python_apt either succeeds or fails with a traceback. This commit doesn't affect that behaviour. --- .../modules/packaging/os/apt_repository.py | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/lib/ansible/modules/packaging/os/apt_repository.py b/lib/ansible/modules/packaging/os/apt_repository.py index a72c0505717..aa7e8d07a60 100644 --- a/lib/ansible/modules/packaging/os/apt_repository.py +++ b/lib/ansible/modules/packaging/os/apt_repository.py @@ -423,24 +423,24 @@ def main(): ) params = module.params - if params['install_python_apt'] and not HAVE_PYTHON_APT and not module.check_mode: - install_python_apt(module) - repo = module.params['repo'] state = module.params['state'] update_cache = module.params['update_cache'] sourceslist = None - if HAVE_PYTHON_APT: - if isinstance(distro, aptsources_distro.UbuntuDistribution): - sourceslist = UbuntuSourcesList(module, - add_ppa_signing_keys_callback=get_add_ppa_signing_key_callback(module)) - elif HAVE_PYTHON_APT and \ - isinstance(distro, aptsources_distro.DebianDistribution) or isinstance(distro, aptsources_distro.Distribution): - sourceslist = SourcesList() + if not HAVE_PYTHON_APT: + if params['install_python_apt']: + install_python_apt(module) + else: + module.fail_json(msg='python-apt is not installed, and install_python_apt is False') + + if isinstance(distro, aptsources_distro.UbuntuDistribution): + sourceslist = UbuntuSourcesList(module, + add_ppa_signing_keys_callback=get_add_ppa_signing_key_callback(module)) + elif isinstance(distro, aptsources_distro.DebianDistribution) or isinstance(distro, aptsources_distro.Distribution): + sourceslist = SourcesList() else: - module.fail_json(msg='Module apt_repository supports only Debian and Ubuntu. ' + \ - 'You may be seeing this because python-apt is not installed, but you requested that it not be auto-installed') + module.fail_json(msg='Module apt_repository supports only Debian and Ubuntu.') sources_before = sourceslist.dump()