Fixed automountkey code review issues.

Fixed several issues found during code review and change
AutomountkeyModule to use IPAAnsibleModule instead of deprecated
FreeIPABaseModule.

README-automountkey.md

@@ -4,7 +4,7 @@ Automountkey module
 Description
 -----------
 
-The automountkey module allows the addition and removal of keys within an automount map. 
+The automountkey module allows management of keys within an automount map.
 
 It is desgined to follow the IPA api as closely as possible while ensuring ease of use.
 
@@ -38,16 +38,15 @@ ipaserver.test.local
 ```
 
 
-Example playbook to ensure presence of an automount map:
+Example playbook to ensure presence of an automount key:
 
 ```yaml
 ---
-- name: Playbook to add an automount map
+- name: Playbook to manage automount key
   hosts: ipaserver
-  become: true
 
   tasks:
-  - name: create key TestKey
+  - name: ensure automount key TestKey is present
     ipaautomountkey:
       ipaadmin_password: SomeADMINpassword
       location: TestLocation
@@ -55,14 +54,6 @@ Example playbook to ensure presence of an automount map:
       key: TestKey
       info: 192.168.122.1:/exports
       state: present
-
-  - name: ensure key TestKey is absent
-    ipaautomountkey:
-      ipaadmin_password: SomeADMINpassword
-      location: TestLocation
-      mapname: TestMap
-      key: TestKey
-      state: absent
 ```
 
 Example playbook to rename an automount map:
@@ -70,23 +61,40 @@ Example playbook to rename an automount map:
 ```yaml
 ---
 - name: Playbook to add an automount map
-  - name: ensure key TestKey has been renamed to NewKeyName
+  hosts: ipaserver
+
+  tasks:
+  - name: ensure aumount key TestKey is renamed to NewKeyName
     ipaautomountkey:
       ipaadmin_password: password01
       automountlocationcn: TestLocation
       automountmapname: TestMap
       automountkey: TestKey
       newname: NewKeyName
-      state: rename
+      state: renamed
+```
+
+Example playbook to ensure an automount key is absent:
+
+```yaml
+---
+- name: Playbook to manage an automount key
+  hosts: ipaserver
+
+  tasks:
+  - name: ensure automount key TestKey is absent
+    ipaautomountkey:
+      ipaadmin_password: SomeADMINpassword
+      location: TestLocation
+      mapname: TestMap
+      key: TestKey
+      state: absent
 ```
 
 
 Variables
 =========
 
-ipaautomountkey
--------
-
 Variable | Description | Required
 -------- | ----------- | --------
 `ipaadmin_principal` | The admin principal is a string and defaults to `admin` | no
@@ -94,9 +102,9 @@ Variable | Description | Required
 `location` \| `automountlocationcn` \| `automountlocation` | Location name. | yes
 `mapname` \|  `map` \| `automountmapname` \| `automountmap` | Map the key belongs to | yes
 `key` \| `name` \| `automountkey` | Automount key to manage | yes
-`newkey` \| newname` \| `newautomountkey` | the name to change the key to if state is `rename` | yes when state is `rename`
+`rename` \| `new_name` \| `newautomountkey` | the name to change the key to if state is `renamed` | yes when state is `renamed`
 `info` \| `information` \| `automountinformation` | Mount information for the key | yes when state is `present`
-`state` | The state to ensure. It can be one of `present`, or `absent`, default: `present`. | no
+`state` | The state to ensure. It can be one of `present`, `absent` or `renamed`, default: `present`. | no
 
 Authors
 =======

playbooks/automount/automountkey-present.yml

+---
+- name: Playbook to manage an automout key
+  hosts: ipaserver
+
+  tasks:
+  - name: Ensure autmount key is present
+    ipaautomountkey:
+      ipaadmin_password: SomeADMINpassword
+      location: TestLocation
+      mapname: TestMap
+      key: TestKey
+      info: 192.168.122.1:/exports
+      state: present

playbooks/automount/automountkey-renamed.yml

+---
+- name: Playbook to manage an automount key
+  hosts: ipaserver
+
+  tasks:
+  - name: Ensure aumount key TestKey is renamed to NewKeyName
+    ipaautomountkey:
+      ipaadmin_password: password01
+      automountlocationcn: TestLocation
+      automountmapname: TestMap
+      automountkey: TestKey
+      newname: NewKeyName
+      state: renamed

playbooks/automount/automoutkey-absent.yml

+---
+- name: Playbook to manage an automount key
+  hosts: ipaserver
+
+  tasks:
+  - name: Ensure autmount key is present
+    ipaautomountkey:
+      ipaadmin_password: SomeADMINpassword
+      location: TestLocation
+      mapname: TestMap
+      key: TestKey
+      state: absent

plugins/modules/ipaautomountkey.py

@@ -20,6 +20,10 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+from __future__ import (absolute_import, division, print_function)
+
+__metaclass__ = type
+
 ANSIBLE_METADATA = {
     "metadata_version": "1.0",
     "supported_by": "community",
@@ -54,18 +58,18 @@ options:
     required: True
     choices: ["name", "automountkey"]
   newkey:
-    description: key to change to if state=rename
+    description: key to change to if state is 'renamed'
     required: True
     choices: ["newname", "newautomountkey"]
   info:
     description: Mount information for the key
     required: True
-    choices: ["information", "newinfo", "automountinformation"]
+    choices: ["information", "automountinformation"]
   state:
     description: State to ensure
     required: False
     default: present
-    choices: ["present", "absent", "rename"]
+    choices: ["present", "absent", "renamed"]
 '''
 
 EXAMPLES = '''
@@ -91,126 +95,140 @@ RETURN = '''
 '''
 
 from ansible.module_utils.ansible_freeipa_module import (
-    FreeIPABaseModule, ipalib_errors
+    IPAAnsibleModule, ipalib_errors
 )
 
 
-class AutomountKey(FreeIPABaseModule):
+class AutomountKey(IPAAnsibleModule):
 
-    ipa_param_mapping = {
-        'automountkey': "key",
-        'automountmapautomountmapname': "mapname",
-    }
+    def __init__(self, *args, **kwargs):
+        # pylint: disable=super-with-arguments
+        super(AutomountKey, self).__init__(*args, **kwargs)
+        self.commands = []
 
-    def get_key(self, location, mapname, keyname):
-        resp = dict()
+    def get_key(self, location, mapname, key):
         try:
-            resp = self.api_command("automountkey_show",
-                                    location,
-                                    {"automountmapautomountmapname": mapname,
-                                     "automountkey": keyname})
+            args = {
+                "automountmapautomountmapname": mapname,
+                "automountkey": key,
+                "all": True,
+            }
+            resp = self.ipa_command("automountkey_show", location, args)
         except ipalib_errors.NotFound:
-            pass
-
-        return resp.get("result", None)
+            return None
+        else:
+            return resp.get("result")
 
     def check_ipa_params(self):
-        if not self.ipa_params.info and self.ipa_params.state == "present":
+        invalid = []
+        state = self.params_get("state")
+        if state == "present":
+            invalid = ["rename"]
+            if not self.params_get("info"):
                 self.fail_json(msg="Value required for argument 'info'")
 
-        if self.ipa_params.state == "rename" and \
-           self.ipa_params.newname is None:
-            self.fail_json(msg="newname is required if state = 'rename'")
+        if state == "rename":
+            invalid = ["info"]
+            if not self.params_get("rename"):
+                self.fail_json(msg="Value required for argument 'renamed'")
+
+        if state == "absent":
+            invalid = ["info", "rename"]
+
+        self.params_fail_used_invalid(invalid, state)
+
+    @staticmethod
+    def get_args(mapname, key, info, rename):
+        _args = {}
+        if mapname:
+            _args["automountmapautomountmapname"] = mapname
+        if key:
+            _args["automountkey"] = key
+        if info:
+            _args["automountinformation"] = info
+        if rename:
+            _args["rename"] = rename
+        return _args
 
     def define_ipa_commands(self):
-        args = self.get_ipa_command_args()
-        key = self.get_key(self.ipa_params.location,
-                           self.ipa_params.mapname,
-                           self.ipa_params.key)
+        state = self.params_get("state")
+        location = self.params_get("location")
+        mapname = self.params_get("mapname")
+        key = self.params_get("key")
+        info = self.params_get("info")
+        rename = self.params_get("rename")
+
+        args = self.get_args(mapname, key, info, rename)
+
+        res_find = self.get_key(location, mapname, key)
 
-        if self.ipa_params.state == "present":
-            if key is None:
+        if state == "present":
+            if res_find is None:
                 # does not exist and is wanted
-                args["automountinformation"] = self.ipa_params.info
-                self.add_ipa_command(
-                    "automountkey_add",
-                    name=self.ipa_params.location,
-                    args=args
-                )
-            elif key is not None:
+                self.commands.append([location, "automountkey_add", args])
+            else:
                 # exists and is wanted, check for changes
-                if self.ipa_params.info != \
-                        key.get('automountinformation', [None])[0]:
-                    args["newautomountinformation"] = self.ipa_params.info
-                    self.add_ipa_command(
-                        "automountkey_mod",
-                        name=self.ipa_params.location,
-                        args=args
+                if info not in res_find.get("automountinformation"):
+                    self.commands.append([location, "automountkey_mod", args])
+
+        if state == "renamed":
+            if res_find is None:
+                self.fail_json(
+                    msg=(
+                        "Cannot rename inexistent key: '%s', '%s', '%s'"
+                        % (location, mapname, key)
                     )
-        elif self.ipa_params.state == "rename":
-            if key is not None:
-                newkey = self.get_key(self.ipa_params.location,
-                                      self.ipa_params.mapname,
-                                      self.ipa_params.newname)
-
-                args["rename"] = self.ipa_params.newname
-                if newkey is None:
-                    self.add_ipa_command(
-                        "automountkey_mod",
-                        name=self.ipa_params.location,
-                        args=args
                 )
-        else:
+            self.commands.append([location, "automountkey_mod", args])
+
+        if state == "absent":
             # if key exists and self.ipa_params.state == "absent":
-            if key is not None:
-                self.add_ipa_command(
-                    "automountkey_del",
-                    name=self.ipa_params.location,
-                    args=args
-                )
+            if res_find is not None:
+                self.commands.append([location, "automountkey_del", args])
 
 
 def main():
     ipa_module = AutomountKey(
         argument_spec=dict(
-            ipaadmin_principal=dict(type="str",
-                                    default="admin"
-                                    ),
-            ipaadmin_password=dict(type="str",
-                                   required=False,
-                                   no_log=True
-                                   ),
-            state=dict(type='str',
+            state=dict(
+                type='str',
+                choices=['present', 'absent', 'renamed'],
+                required=None,
                 default='present',
-                       choices=['present', 'absent', 'rename']
             ),
-            location=dict(type="str",
+            location=dict(
+                type="str",
                 aliases=["automountlocationcn", "automountlocation"],
-                          default=None,
-                          required=True
+                required=True,
             ),
-            newname=dict(type="str",
-                         aliases=["newkey", "new_name",
-                                  "new_key", "newautomountkey"],
-                         default=None,
-                         required=False
+            rename=dict(
+                type="str",
+                aliases=["new_name", "newautomountkey"],
+                required=False,
             ),
-            mapname=dict(type="str",
+            mapname=dict(
+                type="str",
                 aliases=["map", "automountmapname", "automountmap"],
-                         default=None,
-                         required=True
+                required=True,
             ),
-            key=dict(type="str",
+            key=dict(
+                type="str",
+                aliases=["name", "automountkey"],
                 required=True,
-                     aliases=["name", "automountkey"]
             ),
-            info=dict(type="str",
-                      aliases=["information", "newinfo",
-                               "automountinformation"]
+            info=dict(
+                type="str",
+                aliases=["information", "automountinformation"],
+                required=False,
             ),
         ),
     )
-    ipa_module.ipa_run()
+    ipaapi_context = ipa_module.params_get("ipaapi_context")
+    with ipa_module.ipa_connect(context=ipaapi_context):
+        ipa_module.check_ipa_params()
+        ipa_module.define_ipa_commands()
+        changed = ipa_module.execute_ipa_commands(ipa_module.commands)
+    ipa_module.exit_json(changed=changed)
 
 
 if __name__ == "__main__":

tests/automount/test_automountkey.yml

 ---
 - name: Test automountmap
-  hosts: ipaserver
-  become: true
-  gather_facts: false
+  hosts: "{{ ipa_test_host | default('ipaserver') }}"
+  become: no
+  gather_facts: no
 
   tasks:
-  - name: ensure location TestLocation is absent
+  - name: ensure test location TestLocation is present
     ipaautomountlocation:
       ipaadmin_password: SomeADMINpassword
       name: TestLocation
-      state: absent
 
-#  - name: ensure map TestMap is absent
- #   ipaautomountmap:
- #     ipaadmin_password: SomeADMINpassword
- #     name: TestMap
- #     location: TestLocation
- #     state: absent
-#
-#  - name: ensure key TestKey is absent
-#    ipaautomountkey:
-#      ipaadmin_password: SomeADMINpassword
-#      automountlocationcn: TestLocation
-#      automountmapname: TestMap
-#      automountkey: TestKey
-#      state: absent
-
-  - name: create location TestLocation
-    ipaautomountlocation:
-      ipaadmin_password: SomeADMINpassword
-      name: TestLocation
-      state: present
-
-  - name: create map TestMap 
+  - name: ensure test map TestMap is present
     ipaautomountmap:
       ipaadmin_password: SomeADMINpassword
       name: TestMap
       location: TestLocation
-      desc: "this is a test map that should be deleted by the test"
 
+  - name: ensure key NewKeyName is absent
+    ipaautomountkey:
+      ipaadmin_password: SomeADMINpassword
+      location: TestLocation
+      map: TestMap
+      key: NewKeyName
+      state: absent
+
+  - name: ensure key TestKey is absent
+    ipaautomountkey:
+      ipaadmin_password: SomeADMINpassword
+      location: TestLocation
+      map: TestMap
+      key: NewKeyName
+      state: absent
+
+  - block:
     ### test the key creation, and modification
     - name: ensure key TestKey is present
       ipaautomountkey:
         ipaadmin_password: SomeADMINpassword
-      automountlocationcn: TestLocation
-      automountmapname: TestMap
-      automountkey: TestKey
-      automountinformation: 192.168.122.1:/exports
+        location: TestLocation
+        map: TestMap
+        key: TestKey
+        info: 192.168.122.1:/exports
         state: present
       register: result
       failed_when: result.failed or not result.changed
@@ -54,10 +48,10 @@
     - name: ensure key TestKey is present again
       ipaautomountkey:
         ipaadmin_password: SomeADMINpassword
-      automountlocationcn: TestLocation
-      automountmapname: TestMap
-      automountkey: TestKey
-      automountinformation: 192.168.122.1:/exports
+        location: TestLocation
+        map: TestMap
+        key: TestKey
+        info: 192.168.122.1:/exports
         state: present
       register: result
       failed_when: result.failed or result.changed
@@ -66,10 +60,10 @@
     - name: ensure key TestKey information has been updated
       ipaautomountkey:
         ipaadmin_password: SomeADMINpassword
-      automountlocationcn: TestLocation
-      automountmapname: TestMap
-      automountkey: TestKey
-      automountinformation: 192.168.122.1:/nfsshare
+        location: TestLocation
+        map: TestMap
+        key: TestKey
+        info: 192.168.122.1:/nfsshare
         state: present
       register: result
       failed_when: result.failed or not result.changed
@@ -77,10 +71,10 @@
     - name: ensure key TestKey information has been updated again
       ipaautomountkey:
         ipaadmin_password: SomeADMINpassword
-      automountlocationcn: TestLocation
-      automountmapname: TestMap
-      automountkey: TestKey
-      automountinformation: 192.168.122.1:/nfsshare
+        location: TestLocation
+        map: TestMap
+        key: TestKey
+        info: 192.168.122.1:/nfsshare
         state: present
       register: result
       failed_when: result.failed or result.changed
@@ -89,92 +83,72 @@
     - name: ensure key TestKey has been renamed to NewKeyName
       ipaautomountkey:
         ipaadmin_password: SomeADMINpassword
-      automountlocationcn: TestLocation
-      automountmapname: TestMap
-      automountkey: TestKey
-      automountinformation: 192.168.122.1:/nfsshare
-      newname: NewKeyName
-      state: rename
+        location: TestLocation
+        map: TestMap
+        key: TestKey
+        new_name: NewKeyName
+        state: renamed
       register: result
       failed_when: result.failed or not result.changed
 
-  - name: ensure key TestKey does not exist
+    - name: ensure key TestKey is absent
       ipaautomountkey:
         ipaadmin_password: SomeADMINpassword
-      automountlocationcn: TestLocation
-      automountmapname: TestMap
-      automountkey: NewKeyName
-      automountinformation: 192.168.122.1:/nfsshare
-      state: present
+        location: TestLocation
+        map: TestMap
+        key: TestKey
+        state: absent
       register: result
       failed_when: result.failed or result.changed
 
-  - name: ensure key NewKeyName does exist
+    - name: ensure key NewKeyName is present
       ipaautomountkey:
         ipaadmin_password: SomeADMINpassword
-      automountlocationcn: TestLocation
-      automountmapname: TestMap
-      automountkey: NewKeyName
-      automountinformation: 192.168.122.1:/nfsshare
+        location: TestLocation
+        map: TestMap
+        key: NewKeyName
+        info: 192.168.122.1:/nfsshare
         state: present
       register: result
       failed_when: result.failed or result.changed
 
-  - name: ensure key TestKey has been renamed to NewKeyName again
-    ipaautomountkey:
-      ipaadmin_password: SomeADMINpassword
-      automountlocationcn: TestLocation
-      automountmapname: TestMap
-      automountkey: TestKey
-      automountinformation: 192.168.122.1:/nfsshare
-      newname: NewKeyName
-      state: rename
-    register: result
-    failed_when: result.failed or result.changed
-
-  - name: ensure failure when state=present and newname is not set
+    - name: ensure failure when state is renamed and newname is not set
       ipaautomountkey:
         ipaadmin_password: SomeADMINpassword
-      automountlocationcn: TestLocation
-      automountmapname: TestMap
-      automountkey: TestKey
-      automountinformation: 192.168.122.1:/nfsshare
-      state: rename
+        location: TestLocation
+        map: TestMap
+        key: TestKey
+        state: renamed
       register: result
       failed_when: not result.failed
 
-
     ### cleanup after the tests
+    always:
     - name: ensure key NewKeyName is absent
       ipaautomountkey:
         ipaadmin_password: SomeADMINpassword
-      automountlocationcn: TestLocation
-      automountmapname: TestMap
-      automountkey: NewKeyName
+        location: TestLocation
+        map: TestMap
+        key: NewKeyName
         state: absent
-    register: result
-    failed_when: result.failed or not result.changed
 
-  - name: ensure key TestKey is absent again
+    - name: ensure key TestKey is absent
       ipaautomountkey:
         ipaadmin_password: SomeADMINpassword
-      automountlocationcn: TestLocation
-      automountmapname: TestMap
-      automountkey: NewKeyName
+        location: TestLocation
+        map: TestMap
+        key: NewKeyName
         state: absent
-    register: result
-    failed_when: result.failed or result.changed
 
-  - name: ensure map TestMap is removed
+    - name: ensure map TestMap is absent
       ipaautomountmap:
         ipaadmin_password: SomeADMINpassword
         name: TestMap
         location: TestLocation
         state: absent
 
-  - name: ensure location TestLocation is removed
+    - name: ensure location TestLocation is absent
       ipaautomountlocation:
         ipaadmin_password: SomeADMINpassword
         name: TestLocation
         state: absent
-

tests/automount/test_automountkey_client_context.yml

+---
+- name: Test automountkey
+  hosts: ipaclients, ipaserver
+  become: no
+  gather_facts: no
+
+  tasks:
+  - name: Include FreeIPA facts.
+    include_tasks: ../env_freeipa_facts.yml
+
+  # Test will only be executed if host is not a server.
+  - name: Execute with server context in the client.
+    ipaautomountkey:
+      ipaadmin_password: SomeADMINpassword
+      ipaapi_context: server
+      location: NoLocation
+      map: NoMap
+      key: ThisShouldNotWork
+    register: result
+    failed_when: not (result.failed and result.msg is regex("No module named '*ipaserver'*"))
+    when: ipa_host_is_client
+
+# Import basic module tests, and execute with ipa_context set to 'client'.
+# If ipaclients is set, it will be executed using the client, if not,
+# ipaserver will be used.
+#
+# With this setup, tests can be executed against an IPA client, against
+# an IPA server using "client" context, and ensure that tests are executed
+# in upstream CI.
+
+- name: Test automountlocation using client context, in client host.
+  import_playbook: test_automountkey.yml
+  when: groups['ipaclients']
+  vars:
+    ipa_test_host: ipaclients
+
+- name: Test automountlocation using client context, in server host.
+  import_playbook: test_automountkey.yml
+  when: groups['ipaclients'] is not defined or not groups['ipaclients']
+  vars:
+    ipa_context: client