From 3c2026126488f1166bc6ce87c7b655e6c027c86d Mon Sep 17 00:00:00 2001
From: Alexei Znamensky <103110+russoz@users.noreply.github.com>
Date: Thu, 4 May 2023 08:45:35 +1200
Subject: [PATCH] gconftool2: fix change output (#6270)
* gconftool2: fix change output
* add changelog frag
* gconftool2: improve visibility on the output
* fix obtaining updated value after `set`
* use issue URL in the changelog fragment
* fix further issues
* fix return value docs + changelog frag
* Update plugins/modules/gconftool2.py
Co-authored-by: Felix Fontein <felix@fontein.de>
* fix return value doc
---------
Co-authored-by: Felix Fontein <felix@fontein.de>
---
.../fragments/6270-gconftool2-changed.yml | 5 +
plugins/modules/gconftool2.py | 41 +++++--
tests/unit/plugins/modules/test_gconftool2.py | 103 +++++++++++++++++-
3 files changed, 134 insertions(+), 15 deletions(-)
create mode 100644 changelogs/fragments/6270-gconftool2-changed.yml
diff --git a/changelogs/fragments/6270-gconftool2-changed.yml b/changelogs/fragments/6270-gconftool2-changed.yml
new file mode 100644
index 0000000000..dff6d71748
--- /dev/null
+++ b/changelogs/fragments/6270-gconftool2-changed.yml
@@ -0,0 +1,5 @@
+bugfixes:
+ - gconftool2 - fix ``changed`` result always being ``true`` (https://github.com/ansible-collections/community.general/issues/6028).
+ - gconftool2 - remove requirement of parameter ``value`` when ``state=absent`` (https://github.com/ansible-collections/community.general/issues/6028).
+breaking_changes:
+ - gconftool2 - fix processing of ``gconftool-2`` when ``key`` does not exist, returning ``null`` instead of empty string for both ``value`` and ``previous_value`` return values (https://github.com/ansible-collections/community.general/issues/6028).
diff --git a/plugins/modules/gconftool2.py b/plugins/modules/gconftool2.py
index 949e92b306..704bfb18d9 100644
--- a/plugins/modules/gconftool2.py
+++ b/plugins/modules/gconftool2.py
@@ -35,12 +35,13 @@ options:
type: str
description:
- Preference keys typically have simple values such as strings,
- integers, or lists of strings and integers. This is ignored if the state
- is "get". See man gconftool-2(1).
+ integers, or lists of strings and integers.
+ This is ignored unless I(state=present). See man gconftool-2(1).
value_type:
type: str
description:
- - The type of value being set. This is ignored if the state is "get".
+ - The type of value being set.
+ This is ignored unless I(state=present). See man gconftool-2(1).
choices: [ bool, float, int, string ]
state:
type: str
@@ -56,8 +57,8 @@ options:
See man gconftool-2(1).
direct:
description:
- - Access the config database directly, bypassing server. If direct is
- specified then the config_source must be specified as well.
+ - Access the config database directly, bypassing server. If I(direct) is
+ specified then the I(config_source) must be specified as well.
See man gconftool-2(1).
type: bool
default: false
@@ -73,17 +74,26 @@ EXAMPLES = """
RETURN = '''
key:
- description: The key specified in the module parameters
+ description: The key specified in the module parameters.
returned: success
type: str
sample: /desktop/gnome/interface/font_name
value_type:
- description: The type of the value that was changed
+ description: The type of the value that was changed.
returned: success
type: str
sample: string
value:
- description: The value of the preference key after executing the module
+ description:
+ - The value of the preference key after executing the module or C(null) if key is removed.
+ - From community.general 7.0.0 onwards it returns C(null) for a non-existent I(key), and returns C("") before that.
+ returned: success
+ type: str
+ sample: "Serif 12"
+ previous_value:
+ description:
+ - The value of the preference key before executing the module.
+ - From community.general 7.0.0 onwards it returns C(null) for a non-existent I(key), and returns C("") before that.
returned: success
type: str
sample: "Serif 12"
@@ -95,7 +105,6 @@ from ansible_collections.community.general.plugins.module_utils.gconftool2 impor
class GConftool(StateModuleHelper):
- change_params = ('value', )
diff_params = ('value', )
output_params = ('key', 'value_type')
facts_params = ('key', 'value_type')
@@ -111,7 +120,6 @@ class GConftool(StateModuleHelper):
),
required_if=[
('state', 'present', ['value', 'value_type']),
- ('state', 'absent', ['value']),
('direct', True, ['config_source']),
],
supports_check_mode=True,
@@ -125,6 +133,7 @@ class GConftool(StateModuleHelper):
self.vars.set('previous_value', self._get(), fact=True)
self.vars.set('value_type', self.vars.value_type)
+ self.vars.set('_value', self.vars.previous_value, output=False, change=True)
self.vars.set_meta('value', initial_value=self.vars.previous_value)
self.vars.set('playbook_value', self.vars.value, fact=True)
@@ -132,7 +141,8 @@ class GConftool(StateModuleHelper):
def process(rc, out, err):
if err and fail_on_err:
self.ansible.fail_json(msg='gconftool-2 failed with error: %s' % (str(err)))
- self.vars.value = out.rstrip()
+ out = out.rstrip()
+ self.vars.value = None if out == "" else out
return self.vars.value
return process
@@ -148,11 +158,18 @@ class GConftool(StateModuleHelper):
def state_absent(self):
with self.runner("state key", output_process=self._make_process(False)) as ctx:
ctx.run()
+ if self.verbosity >= 4:
+ self.vars.run_info = ctx.run_info
self.vars.set('new_value', None, fact=True)
+ self.vars._value = None
def state_present(self):
with self.runner("direct config_source value_type state key value", output_process=self._make_process(True)) as ctx:
- self.vars.set('new_value', ctx.run(), fact=True)
+ ctx.run()
+ if self.verbosity >= 4:
+ self.vars.run_info = ctx.run_info
+ self.vars.set('new_value', self._get(), fact=True)
+ self.vars._value = self.vars.new_value
def main():
diff --git a/tests/unit/plugins/modules/test_gconftool2.py b/tests/unit/plugins/modules/test_gconftool2.py
index f01f15ef82..31d4df29f2 100644
--- a/tests/unit/plugins/modules/test_gconftool2.py
+++ b/tests/unit/plugins/modules/test_gconftool2.py
@@ -36,7 +36,6 @@ TEST_CASES = [
(0, '100\n', '',),
),
],
- 'new_value': '100',
}
],
[
@@ -50,11 +49,10 @@ TEST_CASES = [
(0, '', "No value set for `/desktop/gnome/background/picture_filename'\n",),
),
],
- 'new_value': None,
}
],
[
- {'state': 'present', 'key': '/desktop/gnome/background/picture_filename', 'value': '200', 'value_type': 'int'},
+ {'state': 'present', 'key': '/desktop/gnome/background/picture_filename', 'value': 200, 'value_type': 'int'},
{
'id': 'test_simple_element_set',
'run_command.calls': [
@@ -66,10 +64,104 @@ TEST_CASES = [
(
['/testbin/gconftool-2', '--type', 'int', '--set', '/desktop/gnome/background/picture_filename', '200'],
{'environ_update': {'LANGUAGE': 'C', 'LC_ALL': 'C'}, 'check_rc': True},
+ (0, '', '',),
+ ),
+ (
+ ['/testbin/gconftool-2', '--get', '/desktop/gnome/background/picture_filename'],
+ {'environ_update': {'LANGUAGE': 'C', 'LC_ALL': 'C'}, 'check_rc': True},
(0, '200\n', '',),
),
],
'new_value': '200',
+ 'changed': True,
+ }
+ ],
+ [
+ {'state': 'present', 'key': '/desktop/gnome/background/picture_filename', 'value': 200, 'value_type': 'int'},
+ {
+ 'id': 'test_simple_element_set_idempotency_int',
+ 'run_command.calls': [
+ (
+ ['/testbin/gconftool-2', '--get', '/desktop/gnome/background/picture_filename'],
+ {'environ_update': {'LANGUAGE': 'C', 'LC_ALL': 'C'}, 'check_rc': True},
+ (0, '200\n', '',),
+ ),
+ (
+ ['/testbin/gconftool-2', '--type', 'int', '--set', '/desktop/gnome/background/picture_filename', '200'],
+ {'environ_update': {'LANGUAGE': 'C', 'LC_ALL': 'C'}, 'check_rc': True},
+ (0, '', '',),
+ ),
+ (
+ ['/testbin/gconftool-2', '--get', '/desktop/gnome/background/picture_filename'],
+ {'environ_update': {'LANGUAGE': 'C', 'LC_ALL': 'C'}, 'check_rc': True},
+ (0, '200\n', '',),
+ ),
+ ],
+ 'new_value': '200',
+ 'changed': False,
+ }
+ ],
+ [
+ {'state': 'present', 'key': '/apps/gnome_settings_daemon/screensaver/start_screensaver', 'value': 'false', 'value_type': 'bool'},
+ {
+ 'id': 'test_simple_element_set_idempotency_bool',
+ 'run_command.calls': [
+ (
+ ['/testbin/gconftool-2', '--get', '/apps/gnome_settings_daemon/screensaver/start_screensaver'],
+ {'environ_update': {'LANGUAGE': 'C', 'LC_ALL': 'C'}, 'check_rc': True},
+ (0, 'false\n', '',),
+ ),
+ (
+ ['/testbin/gconftool-2', '--type', 'bool', '--set', '/apps/gnome_settings_daemon/screensaver/start_screensaver', 'false'],
+ {'environ_update': {'LANGUAGE': 'C', 'LC_ALL': 'C'}, 'check_rc': True},
+ (0, '', '',),
+ ),
+ (
+ ['/testbin/gconftool-2', '--get', '/apps/gnome_settings_daemon/screensaver/start_screensaver'],
+ {'environ_update': {'LANGUAGE': 'C', 'LC_ALL': 'C'}, 'check_rc': True},
+ (0, 'false\n', '',),
+ ),
+ ],
+ 'new_value': 'false',
+ 'changed': False,
+ }
+ ],
+ [
+ {'state': 'absent', 'key': '/desktop/gnome/background/picture_filename'},
+ {
+ 'id': 'test_simple_element_unset',
+ 'run_command.calls': [
+ (
+ ['/testbin/gconftool-2', '--get', '/desktop/gnome/background/picture_filename'],
+ {'environ_update': {'LANGUAGE': 'C', 'LC_ALL': 'C'}, 'check_rc': True},
+ (0, '200\n', '',),
+ ),
+ (
+ ['/testbin/gconftool-2', '--unset', '/desktop/gnome/background/picture_filename'],
+ {'environ_update': {'LANGUAGE': 'C', 'LC_ALL': 'C'}, 'check_rc': True},
+ (0, '', '',),
+ ),
+ ],
+ 'changed': True,
+ }
+ ],
+ [
+ {'state': 'absent', 'key': '/apps/gnome_settings_daemon/screensaver/start_screensaver'},
+ {
+ 'id': 'test_simple_element_unset_idempotency',
+ 'run_command.calls': [
+ (
+ ['/testbin/gconftool-2', '--get', '/apps/gnome_settings_daemon/screensaver/start_screensaver'],
+ {'environ_update': {'LANGUAGE': 'C', 'LC_ALL': 'C'}, 'check_rc': True},
+ (0, '', '',),
+ ),
+ (
+ ['/testbin/gconftool-2', '--unset', '/apps/gnome_settings_daemon/screensaver/start_screensaver'],
+ {'environ_update': {'LANGUAGE': 'C', 'LC_ALL': 'C'}, 'check_rc': True},
+ (0, '', '',),
+ ),
+ ],
+ 'changed': False,
}
],
]
@@ -101,6 +193,11 @@ def test_gconftool2(mocker, capfd, patch_gconftool2, testcase):
print("testcase =\n%s" % testcase)
print("results =\n%s" % results)
+ if 'changed' in testcase:
+ assert results.get('changed', False) == testcase['changed']
+ if 'new_value' in testcase:
+ assert results.get('new_value', None) == testcase['new_value']
+
for conditional_test_result in ('value',):
if conditional_test_result in testcase:
assert conditional_test_result in results, "'{0}' not found in {1}".format(conditional_test_result, results)