Apply suggestions from code review

Added suggestions from @IamLunchbox

Co-authored-by: IamLunchbox <[email protected]>

Author: JanaHoch

plugins/module_utils/proxmox.py

@@ -78,13 +78,13 @@ def compare_list_of_dicts(existing_list, new_list, uid, params_to_ignore=None):
     Use case - for firewall rules we will be getting a list of rules from user.
     We want to filter out which rules needs to be updated and which rules are completely new and needs to be created
 
-    @param existing_list: Existing values example - list of existing rules
-    @param new_list: New values example - list of rules passed to module
-    @param uid: unique identifier in dict. It should always be present in both lists - in case of firewall rules it's pos
-    @param params_to_ignore:  list of params we want to ignore which are present in existing_list's dict.
+    :param existing_list: Existing values example - list of existing rules
+    :param new_list: New values example - list of rules passed to module
+    :param uid: unique identifier in dict. It should always be present in both lists - in case of firewall rules it's pos
+    :param params_to_ignore:  list of params we want to ignore which are present in existing_list's dict.
                             In case of firewall rules we want to ignore ['digest', 'ipversion']
 
-    @return: returns 2 list items 1st is the list of items which are completely new and needs to be created
+    :return: returns 2 list items 1st is the list of items which are completely new and needs to be created
             2nd is a list of items which needs to be updated
     """
     if params_to_ignore is None:

plugins/module_utils/proxmox_sdn.py

@@ -104,7 +104,7 @@ def get_aliases(self, firewall_obj):
         """Get aliases for IP/CIDR at given firewall endpoint level
 
         :param firewall_obj: Firewall endpoint as a ProxmoxResource e.g. self.proxmox_api.cluster().firewall
-                            If it is None it'll return empty list
+                            If it is None it'll return an empty list
         :return: List of aliases and corresponding IP/CIDR
         """
         if firewall_obj is None:
@@ -123,14 +123,13 @@ def get_fw_rules(self, rules_obj, pos=None):
         :param pos: Rule position if it is None it'll return all rules
         :return: Firewall rules as a list of dict
         """
-        if pos is not None:
-            rules_obj = getattr(rules_obj(), str(pos))
-        try:
-            return rules_obj.get()
-        except Exception as e:
-            self.module.fail_json(
-                msg=f'Failed to retrieve firewall rules: {e}'
-            )
+        if pos:
+            try:
+                return rules_obj(str(pos)).get()
+            except Exception as e:
+                self.module.fail_json(
+                    msg=f'Failed to retrieve firewall rules: {e}'
+                )
 
     def get_groups(self):
         """Get firewall security groups

plugins/modules/proxmox_firewall.py

@@ -14,9 +14,8 @@
 short_description: Manage firewall rules in Proxmox
 version_added: "1.4.0"
 description:
-    - create/update/delete FW rules at cluster/group/vnet/node/vm level
-    - Create/delete firewall security groups
-    - get firewall rules at cluster/group/vnet/node/vm level
+    - Create/update/delete firewall rules at cluster/group/vnet/node/vm level. 
+    - Create/delete firewall security groups.
 author: 'Jana Hoch <[email protected]> (!UNKNOWN)'
 attributes:
   check_mode:
@@ -26,16 +25,15 @@
 options:
   state:
     description:
-      - create/update/delete firewall rules or security group
-      - if state is not provided then it will just list firewall rules at level
+      - Create/update/delete firewall rules or security group. 
     type: str
     choices:
       - present
       - absent
     default: present
   update:
     description:
-      - If O(state=present) and if 1 or more rule/alias already exists it will update them
+      - If O(state=present) and if one or more rule/alias already exists it will update them. 
     type: bool
     default: true
   level:
@@ -52,22 +50,22 @@
   node:
     description:
       - Name of the node.
-      - only needed when level is node.
+      - Only needed when O(level=node).
     type: str
   vmid:
     description:
       - ID of the VM to which the rule applies.
-      - only needed when level is vm.
+      - Only needed when O(level=vm).
     type: int
   vnet:
     description:
       - Name of the virtual network for the rule.
-      - only needed when level is vnet.
+      - Only needed when O(level=vnet).
     type: str
   pos:
     description:
       - Position of the rule in the list.
-      - only needed if deleting rule or trying to list it
+      - Only needed if O(state=absent).
     type: int
   group_conf:
     description:
@@ -77,7 +75,7 @@
   group:
     description:
       - Name of the group to which the rule belongs.
-      - only needed when level is group or group_conf is True.
+      - Only needed when O(level=group) or O(group_conf=true).
     type: str
   comment:
     description:
@@ -86,23 +84,23 @@
     type: str
   aliases:
     description:
-      - List of aliases
-      - Alias can only be created/updated/deleted at cluster or VM level
+      - List of aliases.
+      - Alias can only be created/updated/deleted at cluster or VM level.
     type: list
     elements: dict
     suboptions:
       name:
-        description: Alias name
+        description: Alias name.
         type: str
         required: true
       cidr:
         description:
-          - CIDR for alias
-          - only needed when O(state=present) or O(state=update)
+          - CIDR for alias.
+          - Only needed when O(state=present) or O(state=update).
         type: str
         required: false
       comment:
-        description: Comment for Alias
+        description: Comment for alias.
         type: str
         required: false
   rules:
@@ -215,7 +213,7 @@
     api_token_id: "{{ pc.proxmox.api_token_id }}"
     api_token_secret: "{{ vault.proxmox.api_token_secret }}"
     api_host: "{{ pc.proxmox.api_host }}"
-    validate_certs: no
+    validate_certs: false
     level: cluster
     state: present
     rules:
@@ -224,43 +222,43 @@
         source: 1.1.1.1
         log: nolog
         pos: 9
-        enable: True
+        enable: true
       - type: out
         action: ACCEPT
         source: 1.0.0.1
         pos: 10
-        enable: True
+        enable: true
 
 - name: Update Cluster level firewall rules
   community.proxmox.proxmox_firewall:
     api_user: "{{ pc.proxmox.api_user }}"
     api_token_id: "{{ pc.proxmox.api_token_id }}"
     api_token_secret: "{{ vault.proxmox.api_token_secret }}"
     api_host: "{{ pc.proxmox.api_host }}"
-    validate_certs: no
+    validate_certs: false
     level: cluster
     state: present
-    update: True
+    update: true
     rules:
       - type: out
         action: ACCEPT
         source: 8.8.8.8
         log: nolog
         pos: 9
-        enable: False
+        enable: false
       - type: out
         action: ACCEPT
         source: 8.8.4.4
         pos: 10
-        enable: False
+        enable: false
 
 - name: Delete cluster level firewall rule at pos 10
   community.proxmox.proxmox_firewall:
     api_user: "{{ pc.proxmox.api_user }}"
     api_token_id: "{{ pc.proxmox.api_token_id }}"
     api_token_secret: "{{ vault.proxmox.api_token_secret }}"
     api_host: "{{ pc.proxmox.api_host }}"
-    validate_certs: no
+    validate_certs: false
     level: cluster
     state: absent
     pos: 10
@@ -271,8 +269,8 @@
     api_token_id: "{{ pc.proxmox.api_token_id }}"
     api_token_secret: "{{ vault.proxmox.api_token_secret }}"
     api_host: "{{ pc.proxmox.api_host }}"
-    validate_certs: no
-    group_conf: True
+    validate_certs: false
+    group_conf: true
     state: present
     group: test
 
@@ -282,8 +280,8 @@
     api_token_id: "{{ pc.proxmox.api_token_id }}"
     api_token_secret: "{{ vault.proxmox.api_token_secret }}"
     api_host: "{{ pc.proxmox.api_host }}"
-    validate_certs: no
-    group_conf: True
+    validate_certs: false
+    group_conf: true
     state: absent
     group: test
 
@@ -293,7 +291,7 @@
     api_token_id: "{{ pc.proxmox.api_token_id }}"
     api_token_secret: "{{ vault.proxmox.api_token_secret }}"
     api_host: "{{ pc.proxmox.api_host }}"
-    validate_certs: no
+    validate_certs: false
     state: present
     aliases:
       - name: test1
@@ -307,9 +305,9 @@
     api_token_id: "{{ pc.proxmox.api_token_id }}"
     api_token_secret: "{{ vault.proxmox.api_token_secret }}"
     api_host: "{{ pc.proxmox.api_host }}"
-    validate_certs: no
+    validate_certs: false
     state: present
-    update: True
+    update: true
     aliases:
       - name: test1
         cidr: '10.10.1.0/28'
@@ -322,7 +320,7 @@
     api_token_id: "{{ pc.proxmox.api_token_id }}"
     api_token_secret: "{{ vault.proxmox.api_token_secret }}"
     api_host: "{{ pc.proxmox.api_host }}"
-    validate_certs: no
+    validate_certs: false
     state: absent
     aliases:
       - name: test1
@@ -433,7 +431,7 @@ def validate_params(self):
                 return True
             else:
                 self.module.fail_json(
-                    msg="When State is absent either group_conf should be true or pos/aliases must be present but not both"
+                    msg="When state is absent either group_conf should be true or pos/aliases must be present but not both"
                 )
 
     def run(self):
@@ -447,7 +445,7 @@ def run(self):
         group = self.params.get("group")
         group_conf = self.params.get("group_conf")
 
-        if rules is not None:
+        if rules:
             for rule in rules:
                 rule['icmp-type'] = rule.get('icmp_type')
                 rule['enable'] = ansible_to_proxmox_bool(rule.get('enable'))
@@ -479,20 +477,20 @@ def run(self):
         if state == "present":
             if group_conf:
                 self.group_present(group=group, comment=self.params.get('comment'))
-            if rules is not None:
+            if rules:
                 self.fw_rules_present(rules_obj=rules_obj, rules=rules, update=update)
-            if aliases is not None:
+            if aliases:
                 self.aliases_present(firewall_obj=firewall_obj, level=level, aliases=aliases, update=update)
         elif state == "absent":
             if self.params.get('pos'):
                 self.fw_rule_absent(rules_obj=rules_obj, pos=self.params.get('pos'))
             if group_conf:
                 self.group_absent(group_name=group)
-            if aliases is not None:
+            if aliases:
                 self.aliases_absent(firewall_obj=firewall_obj, aliases=aliases)
 
     def aliases_present(self, firewall_obj, level, aliases, update):
-        if firewall_obj is None or level not in ['cluster', 'vm']:
+        if not firewall_obj or level not in ['cluster', 'vm']:
             self.module.fail_json(
                 msg='Aliases can only be created at cluster or VM level'
             )

plugins/modules/proxmox_firewall_info.py

@@ -14,9 +14,7 @@
 short_description: Manage firewall rules in Proxmox
 version_added: "1.4.0"
 description:
-    - create/update/delete FW rules at cluster/group/vnet/node/vm level
-    - Create/delete firewall security groups
-    - get firewall rules at cluster/group/vnet/node/vm level
+    - Get firewall rules at cluster/group/vnet/node/vm level.
 author: 'Jana Hoch <[email protected]> (!UNKNOWN)'
 options:
   level:
@@ -33,27 +31,26 @@
   node:
     description:
       - Name of the node.
-      - only needed when level is node.
+      - Only needed when O(level=node).
     type: str
   vmid:
     description:
       - ID of the VM to which the rule applies.
-      - only needed when level is vm.
+      - Only needed when O(level=vm).
     type: int
   vnet:
     description:
       - Name of the virtual network for the rule.
-      - only needed when level is vnet.
+      - Only needed when O(level=vnet).
     type: str
   pos:
     description:
       - Position of the rule in the list.
-      - only needed if deleting rule or trying to list it
     type: int
   group:
     description:
       - Name of the group to which the rule belongs.
-      - only needed when level is group or group_conf is True.
+      - Only needed when O(level=group).
     type: str
 extends_documentation_fragment:
   - community.proxmox.proxmox.actiongroup_proxmox
@@ -69,7 +66,7 @@
     api_token_id: "{{ pc.proxmox.api_token_id }}"
     api_token_secret: "{{ vault.proxmox.api_token_secret }}"
     api_host: "{{ pc.proxmox.api_host }}"
-    validate_certs: no
+    validate_certs: false
     level: cluster
 """
 
@@ -78,7 +75,7 @@
     description:
       - List of firewall security groups.
       - This will always be given for cluster level regardless of the level passed.
-      - Because only at cluster level we can have firewall security groups
+      - Because only at cluster level we can have firewall security groups.
     returned: on success
     type: list
     elements: str
@@ -87,8 +84,8 @@
 
 aliases:
     description:
-      - list of alias present at given level
-      - aliases are only available for cluster and VM level so if any other level it'll be empty list
+      - List of alias present at given level.
+      - Aliases are only available for cluster and VM level so if any other level it'll be empty list.
     returned: on success
     type: list
     elements: dict