From c3f2e4679cb02a479a36667e984caa3292059c89 Mon Sep 17 00:00:00 2001 From: Jakub Libosvar Date: Mon, 9 Jul 2018 15:42:41 +0200 Subject: [PATCH] openvswitch_db: Make 'key' parameter optional (#42110) The OVSDB schema consists of typed columns. The 'key' parameter is required only for columns with type of a 'map'. This patch makes 'key' an optional parameter to allow setting values for other column types like int. Fixes #42108 (cherry picked from commit 26b0908270ec54db241cada516b56abd79e2e2cc) --- .../openvswitch_db_make_key_optional.yaml | 2 + .../modules/network/ovs/openvswitch_db.py | 53 +++++++++++++++---- .../targets/openvswitch_db/tests/basic.yaml | 41 +++++++++++++- .../network/ovs/test_openvswitch_db.py | 23 ++++++++ 4 files changed, 108 insertions(+), 11 deletions(-) create mode 100644 changelogs/fragments/openvswitch_db_make_key_optional.yaml diff --git a/changelogs/fragments/openvswitch_db_make_key_optional.yaml b/changelogs/fragments/openvswitch_db_make_key_optional.yaml new file mode 100644 index 00000000000..7f1c660c205 --- /dev/null +++ b/changelogs/fragments/openvswitch_db_make_key_optional.yaml @@ -0,0 +1,2 @@ +bugfixes: + - openvswitch_db - make 'key' argument optional https://github.com/ansible/ansible/issues/42108 diff --git a/lib/ansible/modules/network/ovs/openvswitch_db.py b/lib/ansible/modules/network/ovs/openvswitch_db.py index 1dd75563d3e..a13486126ad 100644 --- a/lib/ansible/modules/network/ovs/openvswitch_db.py +++ b/lib/ansible/modules/network/ovs/openvswitch_db.py @@ -48,9 +48,10 @@ options: description: - Identifies the column in the record. key: - required: true + required: false description: - - Identifies the key in the record column + - Identifies the key in the record column, when the column is a map + type. value: required: true description: @@ -87,11 +88,23 @@ EXAMPLES = ''' record: br-int col: other_config key: disable-in-band + +# Mark port with tag 10 +- openvswitch_db: + table: Port + record: port0 + col: tag + value: 10 ''' import re from ansible.module_utils.basic import AnsibleModule +# Regular expression for map type, must not be empty +NON_EMPTY_MAP_RE = re.compile(r'{.+}') +# Regular expression for a map column type +MAP_RE = re.compile(r'{.*}') + def map_obj_to_commands(want, have, module): """ Define ovs-vsctl command to meet desired state """ @@ -102,8 +115,16 @@ def map_obj_to_commands(want, have, module): templatized_command = "%(ovs-vsctl)s -t %(timeout)s remove %(table)s %(record)s " \ "%(col)s %(key)s=%(value)s" commands.append(templatized_command % module.params) + elif module.params['key'] is None: + templatized_command = "%(ovs-vsctl)s -t %(timeout)s remove %(table)s %(record)s " \ + "%(col)s" + commands.append(templatized_command % module.params) else: - if 'key' not in have.keys(): + if module.params['key'] is None: + templatized_command = "%(ovs-vsctl)s -t %(timeout)s set %(table)s %(record)s " \ + "%(col)s=%(value)s" + commands.append(templatized_command % module.params) + elif 'key' not in have.keys(): templatized_command = "%(ovs-vsctl)s -t %(timeout)s add %(table)s %(record)s " \ "%(col)s %(key)s=%(value)s" commands.append(templatized_command % module.params) @@ -125,8 +146,16 @@ def map_config_to_obj(module): match = re.search(r'^' + module.params['col'] + r'(\s+):(\s+)(.*)$', out, re.M) col_value = match.group(3) + + # Map types require key argument + has_key = module.params['key'] is not None + is_map = MAP_RE.match(col_value) + if is_map and not has_key: + module.fail_json( + msg="missing required arguments: key for map type of column") + col_value_to_dict = {} - if col_value and col_value != '{}': + if NON_EMPTY_MAP_RE.match(col_value): for kv in col_value[1:-1].split(', '): k, v = kv.split('=') col_value_to_dict[k.strip()] = v.strip() @@ -137,9 +166,12 @@ def map_config_to_obj(module): 'col': module.params['col'], } - if module.params['key'] in col_value_to_dict: - obj['key'] = module.params['key'] - obj['value'] = col_value_to_dict[module.params['key']] + if has_key and is_map: + if module.params['key'] in col_value_to_dict: + obj['key'] = module.params['key'] + obj['value'] = col_value_to_dict[module.params['key']] + else: + obj['value'] = col_value.strip() return obj @@ -149,10 +181,13 @@ def map_params_to_obj(module): 'table': module.params['table'], 'record': module.params['record'], 'col': module.params['col'], - 'key': module.params['key'], 'value': module.params['value'] } + key = module.params['key'] + if key is not None: + obj['key'] = key + return obj @@ -163,7 +198,7 @@ def main(): 'table': {'required': True}, 'record': {'required': True}, 'col': {'required': True}, - 'key': {'required': True}, + 'key': {'required': False}, 'value': {'required': True}, 'timeout': {'default': 5, 'type': 'int'}, } diff --git a/test/integration/targets/openvswitch_db/tests/basic.yaml b/test/integration/targets/openvswitch_db/tests/basic.yaml index fcc14410ebd..9447715c7c3 100644 --- a/test/integration/targets/openvswitch_db/tests/basic.yaml +++ b/test/integration/targets/openvswitch_db/tests/basic.yaml @@ -33,7 +33,7 @@ that: - "result.changed == false" -- name: Change column value +- name: Change column value in a map openvswitch_db: table: Bridge record: br-test @@ -46,7 +46,7 @@ that: - "result.changed == true" -- name: Change column value again (idempotent) +- name: Change column value in a map again (idempotent) openvswitch_db: table: Bridge record: br-test @@ -59,6 +59,43 @@ that: - "result.changed == false" +- name: Change column value + openvswitch_db: + table: Bridge + record: br-test + col: stp_enable + value: true + register: result + +- assert: + that: + - "result.changed == true" + +- name: Change column value again (idempotent) + openvswitch_db: + table: Bridge + record: br-test + col: stp_enable + value: true + register: result + +- assert: + that: + - "result.changed == false" + +- name: Try to set value on a map type without a key (negative) + ignore_errors: true + openvswitch_db: + table: Bridge + record: br-test + col: other_config + value: true + register: result + +- assert: + that: + - "result.failed == true" + - name: Remove bridge openvswitch_db: table: Bridge diff --git a/test/units/modules/network/ovs/test_openvswitch_db.py b/test/units/modules/network/ovs/test_openvswitch_db.py index b99924c4b25..88efe1e7a3d 100644 --- a/test/units/modules/network/ovs/test_openvswitch_db.py +++ b/test/units/modules/network/ovs/test_openvswitch_db.py @@ -41,6 +41,12 @@ test_name_side_effect_matrix = { 'test_openvswitch_db_present_updates_key': [ (0, 'openvswitch_db_disable_in_band_true.cfg', None), (0, None, None)], + 'test_openvswitch_db_present_missing_key_on_map': [ + (0, 'openvswitch_db_disable_in_band_true.cfg', None), + (0, None, None)], + 'test_openvswitch_db_present_stp_enable': [ + (0, 'openvswitch_db_disable_in_band_true.cfg', None), + (0, None, None)], } @@ -122,3 +128,20 @@ class TestOpenVSwitchDBModule(TestOpenVSwitchModule): commands=['/usr/bin/ovs-vsctl -t 5 set Bridge test-br other_config' ':disable-in-band=False'], test_name='test_openvswitch_db_present_updates_key') + + def test_openvswitch_db_present_missing_key_on_map(self): + set_module_args(dict(state='present', + table='Bridge', record='test-br', + col='other_config', + value='False')) + self.execute_module( + failed=True, + test_name='test_openvswitch_db_present_idempotent') + + def test_openvswitch_db_present_stp_enable(self): + set_module_args(dict(state='present', + table='Bridge', record='test-br', + col='stp_enable', + value='False')) + self.execute_module(changed=True, + test_name='test_openvswitch_db_present_stp_enable')