ansible/community.general
1
0
Fork 0
mirror of https://github.com/ansible-collections/community.general.git synced 2024-09-14 20:13:21 +02:00
Code Releases Activity

[PR #6783/867704dd backport][stable-7] rhsm_repository: refactor handling of subscription-manager (#6830)

Browse source 
rhsm_repository: refactor handling of subscription-manager (#6783)

Create a small helper class Rhsm, so all the logic related to the
interaction with subscription-manager is grouped there:
- create the Rhsm object in main(), once the initial checks are done
- search subscription-manager as required (so there is no need to
  manually check it), and store its path for reuse
- store the common arguments for running subscription-manager
- move run_subscription_manager() to Rhsm as run_repos()
- get rid of the different list parameters: we list only all the
  repositories, so the other cases are not needed (and can be added
  easily, if needed)
- move get_repository_list() to Rhsm as list_repositories()

The execution of subscription-manager is improved as well:
- pass the arguments to run_command() directly as list, rather than
  joining the arguments to string, which run_command() will need to
  split again
- move the "repos" parameter directly in run_repos()
- explicitly disable the shell, already off by default
- disable the expansions of variables, as there are none

Adapt the unit test to the different way run_command() is called.

There should be no behaviour changes.

(cherry picked from commit 867704dd75)

Co-authored-by: Pino Toscano <ptoscano@redhat.com>
This commit is contained in:
patchback[bot] 2023-07-02 22:01:53 +02:00 committed by GitHub
parent 6c718a4f55
commit 72d1af86f3
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
3 changed files with 305 additions and 171 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

6
changelogs/fragments/6783-rhsm_repository-internal-refactor.yml Normal file
View file

@ -0,0 +1,6 @@
minor_changes:
- |
rhsm_repository - the interaction with ``subscription-manager`` was
refactored by grouping things together, removing unused bits, and hardening
the way it is run; no behaviour change is expected
(https://github.com/ansible-collections/community.general/pull/6783).

55
plugins/modules/rhsm_repository.py
View file

@ -97,32 +97,37 @@ from copy import deepcopy
from ansible.module_utils.basic import AnsibleModule from ansible.module_utils.basic import AnsibleModule
def run_subscription_manager(module, arguments): class Rhsm(object):
# Execute subscription-manager with arguments and manage common errors def __init__(self, module):
rhsm_bin = module.get_bin_path('subscription-manager') self.module = module
if not rhsm_bin: self.rhsm_bin = self.module.get_bin_path('subscription-manager', required=True)
module.fail_json(msg='The executable file subscription-manager was not found in PATH') self.rhsm_kwargs = {
'environ_update': dict(LANG='C', LC_ALL='C', LC_MESSAGES='C'),
'expand_user_and_vars': False,
'use_unsafe_shell': False,
}
lang_env = dict(LANG='C', LC_ALL='C', LC_MESSAGES='C') def run_repos(self, arguments):
rc, out, err = module.run_command("%s %s" % (rhsm_bin, " ".join(arguments)), environ_update=lang_env) """
Execute `subscription-manager repos` with arguments and manage common errors
"""
rc, out, err = self.module.run_command(
[self.rhsm_bin, 'repos'] + arguments,
**self.rhsm_kwargs
)
if rc == 0 and out == 'This system has no repositories available through subscriptions.\n': if rc == 0 and out == 'This system has no repositories available through subscriptions.\n':
module.fail_json(msg='This system has no repositories available through subscriptions') self.module.fail_json(msg='This system has no repositories available through subscriptions')
elif rc == 1: elif rc == 1:
module.fail_json(msg='subscription-manager failed with the following error: %s' % err) self.module.fail_json(msg='subscription-manager failed with the following error: %s' % err)
else: else:
return rc, out, err return rc, out, err
def list_repositories(self):
def get_repository_list(module, list_parameter): """
# Generate RHSM repository list and return a list of dict Generate RHSM repository list and return a list of dict
if list_parameter == 'list_enabled': """
rhsm_arguments = ['repos', '--list-enabled'] rc, out, err = self.run_repos(['--list'])
elif list_parameter == 'list_disabled':
rhsm_arguments = ['repos', '--list-disabled']
elif list_parameter == 'list':
rhsm_arguments = ['repos', '--list']
rc, out, err = run_subscription_manager(module, rhsm_arguments)
skip_lines = [ skip_lines = [
'+----------------------------------------------------------+', '+----------------------------------------------------------+',
@ -174,9 +179,9 @@ def get_repository_list(module, list_parameter):
return repo_result return repo_result
def repository_modify(module, state, name, purge=False): def repository_modify(module, rhsm, state, name, purge=False):
name = set(name) name = set(name)
current_repo_list = get_repository_list(module, 'list') current_repo_list = rhsm.list_repositories()
updated_repo_list = deepcopy(current_repo_list) updated_repo_list = deepcopy(current_repo_list)
matched_existing_repo = {} matched_existing_repo = {}
for repoid in name: for repoid in name:
@ -191,7 +196,7 @@ def repository_modify(module, state, name, purge=False):
results = [] results = []
diff_before = "" diff_before = ""
diff_after = "" diff_after = ""
rhsm_arguments = ['repos'] rhsm_arguments = []
for repoid in matched_existing_repo: for repoid in matched_existing_repo:
if len(matched_existing_repo[repoid]) == 0: if len(matched_existing_repo[repoid]) == 0:
@ -236,7 +241,7 @@ def repository_modify(module, state, name, purge=False):
'after_header': "RHSM repositories"} 'after_header': "RHSM repositories"}
if not module.check_mode and changed: if not module.check_mode and changed:
rc, out, err = run_subscription_manager(module, rhsm_arguments) rc, out, err = rhsm.run_repos(rhsm_arguments)
results = out.splitlines() results = out.splitlines()
module.exit_json(results=results, changed=changed, repositories=updated_repo_list, diff=diff) module.exit_json(results=results, changed=changed, repositories=updated_repo_list, diff=diff)
@ -256,6 +261,8 @@ def main():
msg="Interacting with subscription-manager requires root permissions ('become: true')" msg="Interacting with subscription-manager requires root permissions ('become: true')"
) )
rhsm = Rhsm(module)
name = module.params['name'] name = module.params['name']
state = module.params['state'] state = module.params['state']
purge = module.params['purge'] purge = module.params['purge']
@ -268,7 +275,7 @@ def main():
collection_name='community.general', collection_name='community.general',
) )
repository_modify(module, state, name, purge) repository_modify(module, rhsm, state, name, purge)
if __name__ == '__main__': if __name__ == '__main__':

307
tests/unit/plugins/modules/test_rhsm_repository.py
View file

@ -14,6 +14,7 @@ __metaclass__ = type
import copy import copy
import fnmatch import fnmatch
import itertools
import json import json
from ansible.module_utils import basic from ansible.module_utils import basic
@ -143,6 +144,10 @@ Enabled: %s
return self.repos return self.repos
def flatten(iter_of_iters):
return list(itertools.chain.from_iterable(iter_of_iters))
# List with test repositories, directly from the Candlepin test data. # List with test repositories, directly from the Candlepin test data.
REPOS_LIST = [ REPOS_LIST = [
{ {
@ -239,9 +244,11 @@ REPOS = Repos(REPOS_LIST)
# The mock string for the output of `subscription-manager repos --list`. # The mock string for the output of `subscription-manager repos --list`.
REPOS_LIST_OUTPUT = REPOS.to_subman_list_output() REPOS_LIST_OUTPUT = REPOS.to_subman_list_output()
# MUST match what's in run_subscription_manager() in the module. # MUST match what's in the Rhsm class in the module.
SUBMAN_KWARGS = { SUBMAN_KWARGS = {
'environ_update': dict(LANG='C', LC_ALL='C', LC_MESSAGES='C'), 'environ_update': dict(LANG='C', LC_ALL='C', LC_MESSAGES='C'),
'expand_user_and_vars': False,
'use_unsafe_shell': False,
} }
@ -255,15 +262,21 @@ TEST_CASES = [
'id': 'test_enable_single', 'id': 'test_enable_single',
'run_command.calls': [ 'run_command.calls': [
( (
'/testbin/subscription-manager repos --list', [
'/testbin/subscription-manager',
'repos',
'--list',
],
SUBMAN_KWARGS, SUBMAN_KWARGS,
(0, REPOS_LIST_OUTPUT, '') (0, REPOS_LIST_OUTPUT, '')
), ),
( (
( [
'/testbin/subscription-manager repos' '/testbin/subscription-manager',
' --enable awesomeos-1000000000000023' 'repos',
), '--enable',
'awesomeos-1000000000000023',
],
SUBMAN_KWARGS, SUBMAN_KWARGS,
(0, '', '') (0, '', '')
), ),
@ -281,7 +294,11 @@ TEST_CASES = [
'id': 'test_enable_already_enabled', 'id': 'test_enable_already_enabled',
'run_command.calls': [ 'run_command.calls': [
( (
'/testbin/subscription-manager repos --list', [
'/testbin/subscription-manager',
'repos',
'--list',
],
SUBMAN_KWARGS, SUBMAN_KWARGS,
(0, REPOS_LIST_OUTPUT, '') (0, REPOS_LIST_OUTPUT, '')
), ),
@ -299,16 +316,23 @@ TEST_CASES = [
'id': 'test_enable_multiple', 'id': 'test_enable_multiple',
'run_command.calls': [ 'run_command.calls': [
( (
'/testbin/subscription-manager repos --list', [
'/testbin/subscription-manager',
'repos',
'--list',
],
SUBMAN_KWARGS, SUBMAN_KWARGS,
(0, REPOS_LIST_OUTPUT, '') (0, REPOS_LIST_OUTPUT, '')
), ),
( (
( [
'/testbin/subscription-manager repos' '/testbin/subscription-manager',
' --enable awesomeos-1000000000000023' 'repos',
' --enable content-label-no-gpg-32060' '--enable',
), 'awesomeos-1000000000000023',
'--enable',
'content-label-no-gpg-32060',
],
SUBMAN_KWARGS, SUBMAN_KWARGS,
(0, '', '') (0, '', '')
), ),
@ -326,16 +350,23 @@ TEST_CASES = [
'id': 'test_enable_multiple_mixed', 'id': 'test_enable_multiple_mixed',
'run_command.calls': [ 'run_command.calls': [
( (
'/testbin/subscription-manager repos --list', [
'/testbin/subscription-manager',
'repos',
'--list',
],
SUBMAN_KWARGS, SUBMAN_KWARGS,
(0, REPOS_LIST_OUTPUT, '') (0, REPOS_LIST_OUTPUT, '')
), ),
( (
( [
'/testbin/subscription-manager repos' '/testbin/subscription-manager',
' --enable awesomeos-1000000000000023' 'repos',
' --enable fake-content-38072' '--enable',
), 'awesomeos-1000000000000023',
'--enable',
'fake-content-38072',
],
SUBMAN_KWARGS, SUBMAN_KWARGS,
(0, '', '') (0, '', '')
), ),
@ -354,15 +385,21 @@ TEST_CASES = [
'id': 'test_purge_everything_but_one_disabled', 'id': 'test_purge_everything_but_one_disabled',
'run_command.calls': [ 'run_command.calls': [
( (
'/testbin/subscription-manager repos --list', [
'/testbin/subscription-manager',
'repos',
'--list',
],
SUBMAN_KWARGS, SUBMAN_KWARGS,
(0, REPOS_LIST_OUTPUT, '') (0, REPOS_LIST_OUTPUT, '')
), ),
( (
( [
'/testbin/subscription-manager repos' '/testbin/subscription-manager',
' --enable never-enabled-content-801' 'repos',
) + ''.join([' --disable ' + i for i in REPOS.ids_enabled() if i != 'never-enabled-content-801']), '--enable',
'never-enabled-content-801',
] + flatten([['--disable', i] for i in REPOS.ids_enabled() if i != 'never-enabled-content-801']),
SUBMAN_KWARGS, SUBMAN_KWARGS,
(0, '', '') (0, '', '')
), ),
@ -381,18 +418,27 @@ TEST_CASES = [
'id': 'test_purge_everything_but_one_enabled', 'id': 'test_purge_everything_but_one_enabled',
'run_command.calls': [ 'run_command.calls': [
( (
'/testbin/subscription-manager repos --list', [
'/testbin/subscription-manager',
'repos',
'--list',
],
SUBMAN_KWARGS, SUBMAN_KWARGS,
(0, REPOS_LIST_OUTPUT, '') (0, REPOS_LIST_OUTPUT, '')
), ),
( (
( [
'/testbin/subscription-manager repos' '/testbin/subscription-manager',
' --enable awesomeos-99000' 'repos',
' --disable content-label-27060' '--enable',
' --disable awesomeos-x86_64-99000' 'awesomeos-99000',
' --disable fake-content-38072' '--disable',
), 'content-label-27060',
'--disable',
'awesomeos-x86_64-99000',
'--disable',
'fake-content-38072',
],
SUBMAN_KWARGS, SUBMAN_KWARGS,
(0, '', '') (0, '', '')
), ),
@ -411,28 +457,47 @@ TEST_CASES = [
'id': 'test_enable_everything_purge_everything_but_one_enabled', 'id': 'test_enable_everything_purge_everything_but_one_enabled',
'run_command.calls': [ 'run_command.calls': [
( (
'/testbin/subscription-manager repos --list', [
'/testbin/subscription-manager',
'repos',
'--list',
],
SUBMAN_KWARGS, SUBMAN_KWARGS,
(0, REPOS.copy().enable('*').to_subman_list_output(), '') (0, REPOS.copy().enable('*').to_subman_list_output(), '')
), ),
( (
( [
'/testbin/subscription-manager repos' '/testbin/subscription-manager',
' --enable content-label-27060' 'repos',
' --disable never-enabled-content-801' '--enable',
' --disable never-enabled-content-100000000000060' 'content-label-27060',
' --disable awesomeos-x86_64-1000000000000023' '--disable',
' --disable awesomeos-ppc64-100000000000011' 'never-enabled-content-801',
' --disable awesomeos-99000' '--disable',
' --disable content-label-no-gpg-32060' 'never-enabled-content-100000000000060',
' --disable awesomeos-1000000000000023' '--disable',
' --disable awesomeos-x86-100000000000020' 'awesomeos-x86_64-1000000000000023',
' --disable awesomeos-x86_64-99000' '--disable',
' --disable awesomeos-s390x-99000' 'awesomeos-ppc64-100000000000011',
' --disable awesomeos-modifier-37080' '--disable',
' --disable awesomeos-i686-99000' 'awesomeos-99000',
' --disable fake-content-38072' '--disable',
), 'content-label-no-gpg-32060',
'--disable',
'awesomeos-1000000000000023',
'--disable',
'awesomeos-x86-100000000000020',
'--disable',
'awesomeos-x86_64-99000',
'--disable',
'awesomeos-s390x-99000',
'--disable',
'awesomeos-modifier-37080',
'--disable',
'awesomeos-i686-99000',
'--disable',
'fake-content-38072',
],
SUBMAN_KWARGS, SUBMAN_KWARGS,
(0, '', '') (0, '', '')
), ),
@ -450,23 +515,37 @@ TEST_CASES = [
'id': 'test_enable_all_awesomeos_star', 'id': 'test_enable_all_awesomeos_star',
'run_command.calls': [ 'run_command.calls': [
( (
'/testbin/subscription-manager repos --list', [
'/testbin/subscription-manager',
'repos',
'--list',
],
SUBMAN_KWARGS, SUBMAN_KWARGS,
(0, REPOS_LIST_OUTPUT, '') (0, REPOS_LIST_OUTPUT, '')
), ),
( (
( [
'/testbin/subscription-manager repos' '/testbin/subscription-manager',
' --enable awesomeos-x86_64-1000000000000023' 'repos',
' --enable awesomeos-ppc64-100000000000011' '--enable',
' --enable awesomeos-99000' 'awesomeos-x86_64-1000000000000023',
' --enable awesomeos-1000000000000023' '--enable',
' --enable awesomeos-x86-100000000000020' 'awesomeos-ppc64-100000000000011',
' --enable awesomeos-x86_64-99000' '--enable',
' --enable awesomeos-s390x-99000' 'awesomeos-99000',
' --enable awesomeos-modifier-37080' '--enable',
' --enable awesomeos-i686-99000' 'awesomeos-1000000000000023',
), '--enable',
'awesomeos-x86-100000000000020',
'--enable',
'awesomeos-x86_64-99000',
'--enable',
'awesomeos-s390x-99000',
'--enable',
'awesomeos-modifier-37080',
'--enable',
'awesomeos-i686-99000',
],
SUBMAN_KWARGS, SUBMAN_KWARGS,
(0, '', '') (0, '', '')
), ),
@ -485,18 +564,27 @@ TEST_CASES = [
'id': 'test_purge_everything_but_awesomeos_list', 'id': 'test_purge_everything_but_awesomeos_list',
'run_command.calls': [ 'run_command.calls': [
( (
'/testbin/subscription-manager repos --list', [
'/testbin/subscription-manager',
'repos',
'--list',
],
SUBMAN_KWARGS, SUBMAN_KWARGS,
(0, REPOS_LIST_OUTPUT, '') (0, REPOS_LIST_OUTPUT, '')
), ),
( (
( [
'/testbin/subscription-manager repos' '/testbin/subscription-manager',
' --enable awesomeos-99000' 'repos',
' --enable awesomeos-x86_64-99000' '--enable',
' --disable content-label-27060' 'awesomeos-99000',
' --disable fake-content-38072' '--enable',
), 'awesomeos-x86_64-99000',
'--disable',
'content-label-27060',
'--disable',
'fake-content-38072',
],
SUBMAN_KWARGS, SUBMAN_KWARGS,
(0, '', '') (0, '', '')
), ),
@ -514,7 +602,11 @@ TEST_CASES = [
'id': 'test_enable_nonexisting', 'id': 'test_enable_nonexisting',
'run_command.calls': [ 'run_command.calls': [
( (
'/testbin/subscription-manager repos --list', [
'/testbin/subscription-manager',
'repos',
'--list',
],
SUBMAN_KWARGS, SUBMAN_KWARGS,
(0, REPOS_LIST_OUTPUT, '') (0, REPOS_LIST_OUTPUT, '')
), ),
@ -533,15 +625,21 @@ TEST_CASES = [
'id': 'test_disable_single', 'id': 'test_disable_single',
'run_command.calls': [ 'run_command.calls': [
( (
'/testbin/subscription-manager repos --list', [
'/testbin/subscription-manager',
'repos',
'--list',
],
SUBMAN_KWARGS, SUBMAN_KWARGS,
(0, REPOS_LIST_OUTPUT, '') (0, REPOS_LIST_OUTPUT, '')
), ),
( (
( [
'/testbin/subscription-manager repos' '/testbin/subscription-manager',
' --disable awesomeos-99000' 'repos',
), '--disable',
'awesomeos-99000',
],
SUBMAN_KWARGS, SUBMAN_KWARGS,
(0, '', '') (0, '', '')
), ),
@ -560,15 +658,21 @@ TEST_CASES = [
'id': 'test_disable_single_using_absent', 'id': 'test_disable_single_using_absent',
'run_command.calls': [ 'run_command.calls': [
( (
'/testbin/subscription-manager repos --list', [
'/testbin/subscription-manager',
'repos',
'--list',
],
SUBMAN_KWARGS, SUBMAN_KWARGS,
(0, REPOS_LIST_OUTPUT, '') (0, REPOS_LIST_OUTPUT, '')
), ),
( (
( [
'/testbin/subscription-manager repos' '/testbin/subscription-manager',
' --disable awesomeos-99000' 'repos',
), '--disable',
'awesomeos-99000',
],
SUBMAN_KWARGS, SUBMAN_KWARGS,
(0, '', '') (0, '', '')
), ),
@ -587,7 +691,11 @@ TEST_CASES = [
'id': 'test_disable_already_disabled', 'id': 'test_disable_already_disabled',
'run_command.calls': [ 'run_command.calls': [
( (
'/testbin/subscription-manager repos --list', [
'/testbin/subscription-manager',
'repos',
'--list',
],
SUBMAN_KWARGS, SUBMAN_KWARGS,
(0, REPOS_LIST_OUTPUT, '') (0, REPOS_LIST_OUTPUT, '')
), ),
@ -607,15 +715,19 @@ TEST_CASES = [
'id': 'test_disable_already_disabled_and_purge', 'id': 'test_disable_already_disabled_and_purge',
'run_command.calls': [ 'run_command.calls': [
( (
'/testbin/subscription-manager repos --list', [
'/testbin/subscription-manager',
'repos',
'--list',
],
SUBMAN_KWARGS, SUBMAN_KWARGS,
(0, REPOS_LIST_OUTPUT, '') (0, REPOS_LIST_OUTPUT, '')
), ),
( (
( [
'/testbin/subscription-manager repos' '/testbin/subscription-manager',
' --disable never-enabled-content-801' 'repos',
) + ''.join([' --disable ' + i for i in REPOS.ids_enabled()]), ] + flatten([['--disable', i] for i in REPOS.ids_enabled()]),
SUBMAN_KWARGS, SUBMAN_KWARGS,
(0, '', '') (0, '', '')
), ),
@ -635,14 +747,19 @@ TEST_CASES = [
'id': 'test_disable_single_and_purge', 'id': 'test_disable_single_and_purge',
'run_command.calls': [ 'run_command.calls': [
( (
'/testbin/subscription-manager repos --list', [
'/testbin/subscription-manager',
'repos',
'--list',
],
SUBMAN_KWARGS, SUBMAN_KWARGS,
(0, REPOS_LIST_OUTPUT, '') (0, REPOS_LIST_OUTPUT, '')
), ),
( (
( [
'/testbin/subscription-manager repos' '/testbin/subscription-manager',
) + ''.join([' --disable ' + i for i in REPOS.ids_enabled()]), 'repos',
] + flatten([['--disable', i] for i in REPOS.ids_enabled()]),
SUBMAN_KWARGS, SUBMAN_KWARGS,
(0, '', '') (0, '', '')
), ),
@ -661,7 +778,11 @@ TEST_CASES = [
'id': 'test_disable_nonexisting', 'id': 'test_disable_nonexisting',
'run_command.calls': [ 'run_command.calls': [
( (
'/testbin/subscription-manager repos --list', [
'/testbin/subscription-manager',
'repos',
'--list',
],
SUBMAN_KWARGS, SUBMAN_KWARGS,
(0, REPOS_LIST_OUTPUT, '') (0, REPOS_LIST_OUTPUT, '')
), ),