From 653fd34ed7ed9c1af6908ce36a4996b7eb17bfe3 Mon Sep 17 00:00:00 2001 From: Abhijit Menon-Sen Date: Thu, 30 Jul 2015 09:02:18 +0530 Subject: [PATCH 1/5] Fix call to _expand_ppa --- packaging/os/apt_repository.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packaging/os/apt_repository.py b/packaging/os/apt_repository.py index 8f6d18d09d5..a72c0505717 100644 --- a/packaging/os/apt_repository.py +++ b/packaging/os/apt_repository.py @@ -390,7 +390,7 @@ class UbuntuSourcesList(SourcesList): continue if source_line.startswith('ppa:'): - source, ppa_owner, ppa_name = self._expand_ppa(i[3]) + source, ppa_owner, ppa_name = self._expand_ppa(source_line) _repositories.append(source) else: _repositories.append(source_line) From 6afa1da9103a4cfe70dc9c2ede87a2961b22b61b Mon Sep 17 00:00:00 2001 From: Abhijit Menon-Sen Date: Thu, 30 Jul 2015 10:45:23 +0530 Subject: [PATCH 2/5] 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. --- packaging/os/apt_repository.py | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/packaging/os/apt_repository.py b/packaging/os/apt_repository.py index a72c0505717..aa7e8d07a60 100644 --- a/packaging/os/apt_repository.py +++ b/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() From 44d16240a8134fbeb25f5598cdb6c6017bc27c37 Mon Sep 17 00:00:00 2001 From: Abhijit Menon-Sen Date: Thu, 30 Jul 2015 13:06:50 +0530 Subject: [PATCH 3/5] Make SourcesList __init__ method also set self.module This was originally required to allow other methods in SourcesList to fail, but subsequent changes rendered that unnecessary, and it's just a cleanup now, and avoids passing in module separately to save(). --- packaging/os/apt_repository.py | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/packaging/os/apt_repository.py b/packaging/os/apt_repository.py index aa7e8d07a60..0e6140651e1 100644 --- a/packaging/os/apt_repository.py +++ b/packaging/os/apt_repository.py @@ -124,7 +124,8 @@ class InvalidSource(Exception): # Simple version of aptsources.sourceslist.SourcesList. # No advanced logic and no backups inside. class SourcesList(object): - def __init__(self): + def __init__(self, module): + self.module = module self.files = {} # group sources by file # Repositories that we're adding -- used to implement mode param self.new_repos = set() @@ -234,7 +235,7 @@ class SourcesList(object): group.append((n, valid, enabled, source, comment)) self.files[file] = group - def save(self, module): + def save(self): for filename, sources in self.files.items(): if sources: d, fn = os.path.split(filename) @@ -255,13 +256,13 @@ class SourcesList(object): try: f.write(line) except IOError, err: - module.fail_json(msg="Failed to write to file %s: %s" % (tmp_path, unicode(err))) - module.atomic_move(tmp_path, filename) + self.module.fail_json(msg="Failed to write to file %s: %s" % (tmp_path, unicode(err))) + self.module.atomic_move(tmp_path, filename) # allow the user to override the default mode if filename in self.new_repos: - this_mode = module.params['mode'] - module.set_mode_if_different(filename, this_mode, False) + this_mode = self.module.params['mode'] + self.module.set_mode_if_different(filename, this_mode, False) else: del self.files[filename] if os.path.exists(filename): @@ -329,7 +330,7 @@ class UbuntuSourcesList(SourcesList): def __init__(self, module, add_ppa_signing_keys_callback=None): self.module = module self.add_ppa_signing_keys_callback = add_ppa_signing_keys_callback - super(UbuntuSourcesList, self).__init__() + super(UbuntuSourcesList, self).__init__(module) def _get_ppa_info(self, owner_name, ppa_name): lp_api = self.LP_API % (owner_name, ppa_name) @@ -438,7 +439,7 @@ def main(): 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() + sourceslist = SourcesList(module) else: module.fail_json(msg='Module apt_repository supports only Debian and Ubuntu.') @@ -462,7 +463,7 @@ def main(): if not module.check_mode and changed: try: - sourceslist.save(module) + sourceslist.save() if update_cache: cache = apt.Cache() cache.update() From 74a27ffe52368d2248bec56037f7f565d288d697 Mon Sep 17 00:00:00 2001 From: Abhijit Menon-Sen Date: Thu, 30 Jul 2015 13:16:11 +0530 Subject: [PATCH 4/5] Simplify distribution test If it's Ubuntu, use UbuntuSourcesList; if it's any other apt-friendly distribution, use SourcesList; otherwise, fail. --- packaging/os/apt_repository.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packaging/os/apt_repository.py b/packaging/os/apt_repository.py index 0e6140651e1..71f78e2970c 100644 --- a/packaging/os/apt_repository.py +++ b/packaging/os/apt_repository.py @@ -438,7 +438,7 @@ def main(): 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): + elif isinstance(distro, aptsources_distro.Distribution): sourceslist = SourcesList(module) else: module.fail_json(msg='Module apt_repository supports only Debian and Ubuntu.') From 2fdb197245ca8ae968b03595f994bb549da607f5 Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bonicoli Date: Wed, 29 Jul 2015 17:49:37 +0200 Subject: [PATCH 5/5] fix error occurring with Debian Error was: AttributeError: 'SourcesList' object has no attribute 'repos_urls' --- packaging/os/apt_repository.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/packaging/os/apt_repository.py b/packaging/os/apt_repository.py index 71f78e2970c..750169325e3 100644 --- a/packaging/os/apt_repository.py +++ b/packaging/os/apt_repository.py @@ -360,6 +360,10 @@ class UbuntuSourcesList(SourcesList): if line.startswith('ppa:'): source, ppa_owner, ppa_name = self._expand_ppa(line) + if source in self.repos_urls: + # repository already exists + return + if self.add_ppa_signing_keys_callback is not None: info = self._get_ppa_info(ppa_owner, ppa_name) if not self._key_already_exists(info['signing_key_fingerprint']): @@ -445,13 +449,8 @@ def main(): sources_before = sourceslist.dump() - if repo.startswith('ppa:'): - expanded_repo = sourceslist._expand_ppa(repo)[0] - else: - expanded_repo = repo - try: - if state == 'present' and expanded_repo not in sourceslist.repos_urls: + if state == 'present': sourceslist.add_source(repo) elif state == 'absent': sourceslist.remove_source(repo)