Refactored federation_mapping{,_info} modules

Change-Id: I8ab45280badf7fc9448e2ebfc6e38648cc2dc1b7

.zuul.yaml

@@ -72,6 +72,7 @@
         dns
         dns_zone_info
         endpoint
+        federation_mapping
         floating_ip
         host_aggregate
         identity_domain_info
@@ -87,7 +88,6 @@
         keystone_domain
         keystone_federation_protocol
         keystone_idp
-        keystone_mapping
         logging
         loadbalancer
         network

ci/roles/federation_mapping/tasks/main.yml

@@ -12,10 +12,6 @@
       openstack.cloud.federation_mapping:
         state: 'absent'
         name: '{{ mapping_name }}'
-      register: delete_mapping
-    - assert:
-        that:
-        - delete_mapping is successful
 
     - name: 'Create mapping - CHECK_MODE'
       openstack.cloud.federation_mapping:
@@ -24,15 +20,16 @@
         rules: '{{ mapping_rules_1 }}'
       register: create_mapping
       check_mode: yes
+
     - assert:
         that:
-        - create_mapping is successful
         - create_mapping is changed
 
     - name: 'Fetch mapping info (mapping should be absent)'
       openstack.cloud.federation_mapping_info:
         name: '{{ mapping_name }}'
       register: mapping_info
+
     - assert:
         that:
         - mapping_info.mappings | length == 0
@@ -43,44 +40,45 @@
         name: '{{ mapping_name }}'
         rules: '{{ mapping_rules_1 }}'
       register: create_mapping
+
     - assert:
         that:
-        - create_mapping is successful
         - create_mapping is changed
-        - '"id" in create_mapping.mapping'
-        - '"name" in create_mapping.mapping'
-        - '"rules" in create_mapping.mapping'
         - create_mapping.mapping.id == mapping_name
         - create_mapping.mapping.name == mapping_name
         - create_mapping.mapping.rules | length == 1
 
+    - name: assert return values of federation_mapping module
+      assert:
+        that:
+          # allow new fields to be introduced but prevent fields from being removed
+          - expected_fields|difference(create_mapping.mapping.keys())|length == 0
+
     - name: 'Fetch mapping info - with name'
       openstack.cloud.federation_mapping_info:
         name: '{{ mapping_name }}'
       register: mapping_info
+
     - assert:
         that:
-        - mapping_info is successful
-        - '"mappings" in mapping_info'
         - mapping_info.mappings | length == 1
-        - mapping_0.id == mapping_name
+        - mapping_info.mappings[0].id == mapping_name
-        - mapping_0.name == mapping_name
+        - mapping_info.mappings[0].name == mapping_name
-        - mapping_0.rules | length == 1
+        - mapping_info.mappings[0].rules | length == 1
-      vars:
-        mapping_0: '{{ mapping_info.mappings[0] }}'
 
-    - name: Verify returned values
+    - name: Check info about mappings
       assert:
-        that: item in mapping_info.mappings[0]
+        that:
-      loop: "{{ expected_fields }}"
+          - mapping_info.mappings|length > 0
+          # allow new fields to be introduced but prevent fields from being removed
+          - expected_fields|difference(mapping_info.mappings[0].keys())|length == 0
 
     - name: 'Fetch mapping info - without name'
       openstack.cloud.federation_mapping_info: {}
       register: mapping_info
+
     - assert:
         that:
-        - mapping_info is successful
-        - '"mappings" in mapping_info'
         # In CI we generally have a clean slate, but this might
         # not be true for everyone...
         - mapping_info.mappings | length >= 1
@@ -94,9 +92,9 @@
         rules: '{{ mapping_rules_1 }}'
       register: create_mapping
       check_mode: yes
+
     - assert:
         that:
-        - create_mapping is successful
         - create_mapping is not changed
 
     - name: 'Create mapping (retry - no change)'
@@ -105,9 +103,9 @@
         name: '{{ mapping_name }}'
         rules: '{{ mapping_rules_1 }}'
       register: create_mapping
+
     - assert:
         that:
-        - create_mapping is successful
         - create_mapping is not changed
         - create_mapping.mapping.id == mapping_name
         - create_mapping.mapping.name == mapping_name
@@ -120,9 +118,9 @@
         rules: '{{ mapping_rules_2 }}'
       register: update_mapping
       check_mode: yes
+
     - assert:
         that:
-        - update_mapping is successful
         - update_mapping is changed
 
     - name: 'Update mapping'
@@ -131,13 +129,10 @@
         name: '{{ mapping_name }}'
         rules: '{{ mapping_rules_2 }}'
       register: update_mapping
+
     - assert:
         that:
-        - update_mapping is successful
         - update_mapping is changed
-        - '"id" in update_mapping.mapping'
-        - '"name" in update_mapping.mapping'
-        - '"rules" in update_mapping.mapping'
         - update_mapping.mapping.id == mapping_name
         - update_mapping.mapping.name == mapping_name
         - update_mapping.mapping.rules | length == 1
@@ -148,13 +143,10 @@
         name: '{{ mapping_name }}'
         rules: '{{ mapping_rules_2 }}'
       register: update_mapping
+
     - assert:
         that:
-        - update_mapping is successful
         - update_mapping is not changed
-        - '"id" in update_mapping.mapping'
-        - '"name" in update_mapping.mapping'
-        - '"rules" in update_mapping.mapping'
         - update_mapping.mapping.id == mapping_name
         - update_mapping.mapping.name == mapping_name
         - update_mapping.mapping.rules | length == 1
@@ -165,13 +157,10 @@
         name: '{{ mapping_name_2 }}'
         rules: '{{ mapping_rules_1 }}'
       register: create_mapping
+
     - assert:
         that:
-        - create_mapping is successful
         - create_mapping is changed
-        - '"id" in create_mapping.mapping'
-        - '"name" in create_mapping.mapping'
-        - '"rules" in create_mapping.mapping'
         - create_mapping.mapping.id == mapping_name_2
         - create_mapping.mapping.name == mapping_name_2
         - create_mapping.mapping.rules | length == 1
@@ -180,24 +169,20 @@
       openstack.cloud.federation_mapping_info:
         name: '{{ mapping_name_2 }}'
       register: mapping_info
+
     - assert:
         that:
-        - mapping_info is successful
-        - '"mappings" in mapping_info'
         - mapping_info.mappings | length == 1
-        - mapping_0.id == mapping_name_2
+        - mapping_info.mappings[0].id == mapping_name_2
-        - mapping_0.name == mapping_name_2
+        - mapping_info.mappings[0].name == mapping_name_2
-        - mapping_0.rules | length == 1
+        - mapping_info.mappings[0].rules | length == 1
-      vars:
-        mapping_0: '{{ mapping_info.mappings[0] }}'
 
     - name: 'Fetch mapping info - without name'
       openstack.cloud.federation_mapping_info: {}
       register: mapping_info
+
     - assert:
         that:
-        - mapping_info is successful
-        - '"mappings" in mapping_info'
         # In CI we generally have a clean slate, but this might
         # not be true for everyone...
         - mapping_info.mappings | length >= 2
@@ -212,9 +197,9 @@
         name: '{{ mapping_name }}'
       register: delete_mapping
       check_mode: yes
+
     - assert:
         that:
-        - delete_mapping is successful
         - delete_mapping is changed
 
     - name: 'Delete mapping'
@@ -222,9 +207,9 @@
         state: 'absent'
         name: '{{ mapping_name }}'
       register: delete_mapping
+
     - assert:
         that:
-        - delete_mapping is successful
         - delete_mapping is changed
 
     - name: 'Delete mapping (retry - no change) - CHECK_MODE'
@@ -233,9 +218,9 @@
         name: '{{ mapping_name }}'
       register: delete_mapping
       check_mode: yes
+
     - assert:
         that:
-        - delete_mapping is successful
         - delete_mapping is not changed
 
     - name: 'Delete mapping (retry - no change) '
@@ -243,15 +228,16 @@
         state: 'absent'
         name: '{{ mapping_name }}'
       register: delete_mapping
+
     - assert:
         that:
-        - delete_mapping is successful
         - delete_mapping is not changed
 
     - name: 'Fetch mapping info after deletion'
       openstack.cloud.federation_mapping_info:
         name: '{{ mapping_name }}'
       register: mapping_info
+
     - assert:
         that:
         - mapping_info.mappings | length == 0
@@ -261,9 +247,9 @@
         state: 'absent'
         name: '{{ mapping_name_2 }}'
       register: delete_mapping
+
     - assert:
         that:
-        - delete_mapping is successful
         - delete_mapping is changed
 
   always:

ci/run-collection.yml

@@ -18,6 +18,7 @@
       tags: dns
       when: sdk_version is version(0.28, '>=')
     - { role: endpoint, tags: endpoint }
+    - { role: federation_mapping, tags: federation_mapping }
     - { role: floating_ip, tags: floating_ip }
     - { role: host_aggregate, tags: host_aggregate }
     - { role: identity_domain_info, tags: identity_domain_info }
@@ -31,9 +32,6 @@
     - { role: image_info, tags: image_info }
     - { role: keypair, tags: keypair }
     - { role: keystone_domain, tags: keystone_domain }
-    - role: keystone_mapping
-      tags: keystone_mapping
-      when: sdk_version is version(0.44, '>=')
     - role: keystone_idp
       tags: keystone_idp
       when: sdk_version is version(0.44, '>=')

plugins/modules/federation_mapping.py

@@ -4,7 +4,7 @@
 # Copyright: Ansible Project
 # GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
 
-DOCUMENTATION = '''
+DOCUMENTATION = r'''
 ---
 module: federation_mapping
 short_description: Manage a federation mapping
@@ -18,19 +18,13 @@ options:
     required: true
     type: str
     aliases: ['id']
-  state:
-    description:
-      - Whether the mapping should be C(present) or C(absent).
-    choices: ['present', 'absent']
-    default: present
-    type: str
   rules:
     description:
-      - The rules that comprise the mapping.  These are pairs of I(local) and
+      - The rules that comprise the mapping. These are pairs of I(local) and
-        I(remote) definitions.  For more details on how these work please see
+        I(remote) definitions. For more details on how these work please see
         the OpenStack documentation
         U(https://docs.openstack.org/keystone/latest/admin/federation/mapping_combinations.html).
-      - Required if I(state=present)
+      - Required if I(state) is C(present).
     type: list
     elements: dict
     suboptions:
@@ -46,14 +40,22 @@ options:
         required: true
         type: list
         elements: dict
+  state:
+    description:
+      - Whether the mapping should be C(present) or C(absent).
+    choices: ['present', 'absent']
+    default: present
+    type: str
+notes:
+    - Name equals the ID of a mapping.
 requirements:
   - "python >= 3.6"
-  - "openstacksdk >= 0.44"
+  - "openstacksdk"
 extends_documentation_fragment:
   - openstack.cloud.openstack
 '''
 
-EXAMPLES = '''
+EXAMPLES = r'''
 - name: Create a new mapping
   openstack.cloud.federation_mapping:
     cloud: example_cloud
@@ -77,7 +79,23 @@ EXAMPLES = '''
     state: absent
 '''
 
-RETURN = '''
+RETURN = r'''
+mapping:
+  description: Dictionary describing the federation mapping.
+  returned: always
+  type: dict
+  contains:
+    id:
+      description: The id of the mapping
+      type: str
+      sample: "ansible-test-mapping"
+    name:
+      description: Name of the mapping. Equal to C(id).
+      type: str
+      sample: "ansible-test-mapping"
+    rules:
+      description: List of rules for the mapping
+      type: list
 '''
 
 from ansible_collections.openstack.cloud.plugins.module_utils.openstack import OpenStackModule
@@ -86,108 +104,94 @@ from ansible_collections.openstack.cloud.plugins.module_utils.openstack import O
 class IdentityFederationMappingModule(OpenStackModule):
     argument_spec = dict(
         name=dict(required=True, aliases=['id']),
+        rules=dict(
+            type='list',
+            elements='dict',
+            options=dict(
+                local=dict(required=True, type='list', elements='dict'),
+                remote=dict(required=True, type='list', elements='dict')
+            )),
         state=dict(default='present', choices=['absent', 'present']),
-        rules=dict(type='list', elements='dict', options=dict(
-            local=dict(required=True, type='list', elements='dict'),
-            remote=dict(required=True, type='list', elements='dict')
-        )),
     )
+
     module_kwargs = dict(
         required_if=[('state', 'present', ['rules'])],
         supports_check_mode=True
     )
 
-    def normalize_mapping(self, mapping):
-        """
-        Normalizes the mapping definitions so that the outputs are consistent with
-        the parameters
-
-        - "name" (parameter) == "id" (SDK)
-        """
-        if mapping is None:
-            return None
-
-        _mapping = mapping.to_dict()
-        _mapping['name'] = mapping['id']
-        return _mapping
-
-    def create_mapping(self, name):
-        """
-        Attempt to create a Mapping
-
-        returns: A tuple containing the "Changed" state and the created mapping
-        """
-
-        if self.ansible.check_mode:
-            return (True, None)
-
-        rules = self.params.get('rules')
-
-        mapping = self.conn.identity.create_mapping(id=name, rules=rules)
-        return (True, mapping)
-
-    def delete_mapping(self, mapping):
-        """
-        Attempt to delete a Mapping
-
-        returns: the "Changed" state
-        """
-        if mapping is None:
-            return False
-
-        if self.ansible.check_mode:
-            return True
-
-        self.conn.identity.delete_mapping(mapping)
-        return True
-
-    def update_mapping(self, mapping):
-        """
-        Attempt to delete a Mapping
-
-        returns: The "Changed" state and the the new mapping
-        """
-
-        current_rules = mapping.rules
-        new_rules = self.params.get('rules')
-
-        # Nothing to do
-        if current_rules == new_rules:
-            return (False, mapping)
-
-        if self.ansible.check_mode:
-            return (True, None)
-
-        new_mapping = self.conn.identity.update_mapping(mapping, rules=new_rules)
-        return (True, new_mapping)
-
     def run(self):
-        """ Module entry point """
+        state = self.params['state']
 
-        name = self.params.get('name')
+        id = self.params['name']
-        state = self.params.get('state')
+        mapping = self.conn.identity.find_mapping(id)
-        changed = False
 
-        mapping = self.conn.identity.find_mapping(name)
+        if self.ansible.check_mode:
+            self.exit_json(changed=self._will_change(state, mapping))
 
-        if state == 'absent':
+        if state == 'present' and not mapping:
-            if mapping is not None:
+            # Create mapping
-                changed = self.delete_mapping(mapping)
+            mapping = self._create()
-            self.exit_json(changed=changed)
+            self.exit_json(changed=True,
+                           mapping=mapping.to_dict(computed=False))
 
-        # state == 'present'
+        elif state == 'present' and mapping:
+            # Update mapping
+            update = self._build_update(mapping)
+            if update:
+                mapping = self._update(mapping, update)
+
+            self.exit_json(changed=bool(update),
+                           mapping=mapping.to_dict(computed=False))
+
+        elif state == 'absent' and mapping:
+            # Delete mapping
+            self._delete(mapping)
+            self.exit_json(changed=True)
+
+        elif state == 'absent' and not mapping:
+            # Do nothing
+            self.exit_json(changed=False)
+
+    def _build_update(self, mapping):
+        update = {}
+
+        if len(self.params['rules']) < 1:
+            self.fail_json(msg='At least one rule must be passed')
+
+        attributes = dict((k, self.params[k]) for k in ['rules']
+                          if k in self.params and self.params[k] is not None
+                          and self.params[k] != mapping[k])
+
+        if attributes:
+            update['attributes'] = attributes
+
+        return update
+
+    def _create(self):
+        return self.conn.identity.create_mapping(id=self.params['name'],
+                                                 rules=self.params['rules'])
+
+    def _delete(self, mapping):
+        self.conn.identity.delete_mapping(mapping.id)
+
+    def _update(self, mapping, update):
+        attributes = update.get('attributes')
+        if attributes:
+            mapping = self.conn.identity.update_mapping(mapping.id,
+                                                        **attributes)
+
+        return mapping
+
+    def _will_change(self, state, mapping):
+        if state == 'present' and not mapping:
+            return True
+        elif state == 'present' and mapping:
+            return bool(self._build_update(mapping))
+        elif state == 'absent' and mapping:
+            return True
         else:
-            if len(self.params.get('rules')) < 1:
+            # state == 'absent' and not mapping:
-                self.fail_json(msg='At least one rule must be passed')
+            return False
-
-            if mapping is None:
-                (changed, mapping) = self.create_mapping(name)
-                mapping = self.normalize_mapping(mapping)
-                self.exit_json(changed=changed, mapping=mapping)
-            else:
-                (changed, new_mapping) = self.update_mapping(mapping)
-                new_mapping = self.normalize_mapping(new_mapping)
-                self.exit_json(mapping=new_mapping, changed=changed)
 
 
 def main():

plugins/modules/federation_mapping_info.py

@@ -4,19 +4,21 @@
 # Copyright: Ansible Project
 # GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
 
-DOCUMENTATION = '''
+DOCUMENTATION = r'''
 ---
 module: federation_mapping_info
-short_description: Get the information about the available federation mappings
+short_description: Fetch Keystone federation mappings
 author: OpenStack Ansible SIG
 description:
-  - Fetch federation mappings.
+  - Fetch Keystone federation mappings.
 options:
   name:
     description:
-      - The name of the mapping to fetch.
+      - ID or name of the federation mapping.
     type: str
     aliases: ['id']
+notes:
+    - Name equals the ID of a federation mapping.
 requirements:
   - "python >= 3.6"
   - "openstacksdk"
@@ -24,38 +26,34 @@ extends_documentation_fragment:
   - openstack.cloud.openstack
 '''
 
-EXAMPLES = '''
+EXAMPLES = r'''
-- name: Fetch a specific mapping
+- name: Fetch all federation mappings
+  openstack.cloud.federation_mapping_info:
+    cloud: example_cloud
+
+- name: Fetch federation mapping by name
   openstack.cloud.federation_mapping_info:
     cloud: example_cloud
     name: example_mapping
-
-- name: Fetch all mappings
-  openstack.cloud.federation_mapping_info:
-    cloud: example_cloud
 '''
 
-RETURN = '''
+RETURN = r'''
 mappings:
-  description:
+  description: List of federation mapping dictionaries.
-    - List of federation mappings
+  returned: always
   type: list
   elements: dict
-  returned: always
   contains:
     id:
-      description:
+      description: The id of the mapping
-        - The id of the mapping
       type: str
       sample: "ansible-test-mapping"
     name:
-      description:
+      description: Name of the mapping. Equal to C(id).
-        - The name of the mapping
       type: str
       sample: "ansible-test-mapping"
     rules:
-      description:
+      description: List of rules for the mapping
-        - List of rules for the mapping
       type: list
 '''
 
@@ -72,7 +70,7 @@ class IdentityFederationMappingInfoModule(OpenStackModule):
     )
 
     def run(self):
-        # name is defined as id for mappings
+        # name is id for federation mappings
         id = self.params['name']
 
         if id: