Merge pull request #703 from singh-lovepreet1/enhance/redfish_powerstate

[redfish_powerstate] - Resolved code smells

plugins/modules/redfish_powerstate.py

@@ -214,10 +214,26 @@ def fetch_ac_powerstate_details(module, system_id_res_data, action_id_res):
             'current_state': current_state, 'allowable_power_state': allowable_final_power_state})
 
 
+def perform_uri_fetch(session_obj, module, system_id_res, resource_id_list, resource_id):
+    reset_type = module.params.get("reset_type")
+    if system_id_res in resource_id_list:
+        system_id_res_resp = session_obj.invoke_request("GET", system_id_res)
+        system_id_res_data = system_id_res_resp.json_data
+        action_id_res = system_id_res_data.get("Actions")
+        if action_id_res:
+            if reset_type:
+                fetch_powerstate_details(system_id_res_data, action_id_res)
+            else:
+                fetch_ac_powerstate_details(module, system_id_res_data, action_id_res)
+        else:
+            module.exit_json(msg=TARGET_DEVICE_NOT_SUPPORTED, skipped=True)
+    else:
+        module.exit_json(msg=INVALID_DEVICE_ID.format(resource_id), skipped=True)
+
+
 def fetch_power_uri_resource(module, session_obj, reset_type_map=None):
     try:
         resource_id = module.params.get("resource_id")
-        reset_type = module.params.get("reset_type")
         static_resource_id_resource = None
         if resource_id:
             static_resource_id_resource = "{0}{1}{2}".format(session_obj.root_uri, reset_type_map + "/", resource_id)
@@ -227,19 +243,7 @@ def fetch_power_uri_resource(module, session_obj, reset_type_map=None):
         if system_members and len(system_members) > 0:
             resource_id_list = [system_id["@odata.id"] for system_id in system_members if "@odata.id" in system_id]
             system_id_res = static_resource_id_resource or resource_id_list[0]
-            if system_id_res in resource_id_list:
-                system_id_res_resp = session_obj.invoke_request("GET", system_id_res)
-                system_id_res_data = system_id_res_resp.json_data
-                action_id_res = system_id_res_data.get("Actions")
-                if action_id_res:
-                    if reset_type:
-                        fetch_powerstate_details(system_id_res_data, action_id_res)
-                    else:
-                        fetch_ac_powerstate_details(module, system_id_res_data, action_id_res)
-                else:
-                    module.exit_json(msg=TARGET_DEVICE_NOT_SUPPORTED, skipped=True)
-            else:
-                module.exit_json(msg=INVALID_DEVICE_ID.format(resource_id), skipped=True)
+            perform_uri_fetch(session_obj, module, system_id_res, resource_id_list, resource_id)
         else:
             module.exit_json(msg=TARGET_DEVICE_NOT_SUPPORTED, skipped=True)
     except HTTPError as err:
@@ -353,6 +357,14 @@ def run_change_power_state(redfish_session_obj, module):
         module.exit_json(msg="The device is already powered {0}.".format(current_power_state.lower()), changed=False)
 
 
+def power_cycle_check_mode(module):
+    current_power_state = powerstate_map["current_state"]
+    if module.check_mode:
+        if current_power_state.lower() != 'off':
+            module.exit_json(msg=INITIAL_DESIRED_STATE_ERROR)
+        module.exit_json(msg=CHANGES_FOUND, changed=True)
+
+
 def run_change_ac_power_cycle(redfish_session_obj, module):
     check_firmware_version(module, redfish_session_obj)
     oem_reset_type = module.params["oem_reset_type"]
@@ -371,11 +383,7 @@ def run_change_ac_power_cycle(redfish_session_obj, module):
     is_valid_reset_type(apply_reset_type, powerstate_map["allowable_enums"], module)
     if final_pwr_state:
         is_valid_final_pwr_state(final_pwr_state, powerstate_map["allowable_power_state"], module)
-    current_power_state = powerstate_map["current_state"]
-    if module.check_mode:
-        if current_power_state.lower() != 'off':
-            module.exit_json(msg=INITIAL_DESIRED_STATE_ERROR)
-        module.exit_json(msg=CHANGES_FOUND, changed=True)
+    power_cycle_check_mode(module)
     payload = prepare_payload(current_vendor_dict)
     power_uri = powerstate_map["power_uri"]
     try:

roles/idrac_server_powerstate/meta/argument_specs.yml

@@ -23,40 +23,52 @@ argument_specs:
       validate_certs:
         description:
           - If C(false), the SSL certificates will not be validated.
-          - Configure C(false) only on personally controlled sites where self-signed certificates are used.
+          - Configure C(false) only on personally controlled sites where
+            self-signed certificates are used.
         type: bool
         default: true
       ca_path:
         description:
-          - The Privacy Enhanced Mail (PEM) file that contains a CA certificate to be used for the validation.
+          - The Privacy Enhanced Mail (PEM) file that contains a CA certificate
+            to be used for the validation.
         type: path
       https_timeout:
         description: The HTTPS socket level timeout in seconds.
         type: int
         default: 30
       resource_id:
         description:
-          - This option is the unique identifier of the device being managed. For example, U(https://<I(baseuri)>/redfish/v1/Systems/<I(resource_id)>).
+          - This option is the unique identifier of the device being managed.
+            For example,
+            U(https://<I(baseuri)>/redfish/v1/Systems/<I(resource_id)>).
           - This option is mandatory for I(base_uri) with multiple devices.
-          - To get the device details, use the API U(https://<I(baseuri)>/redfish/v1/Systems) for reset_type operation and
-            U(https://<I(baseuri)>/redfish/v1/Chassis) for oem_reset_type operation.
+          - To get the device details,
+            use the API U(https://<I(baseuri)>/redfish/v1/Systems) for
+            reset_type operation and U(https://<I(baseuri)>/redfish/v1/Chassis)
+            for oem_reset_type operation.
         required: false
         type: str
       reset_type:
         description:
           - This option resets the device.
           - C(ForceOff) turns off the device immediately.
           - C(ForceOn) turns on the device immediately.
-          - C(ForceRestart) turns off the device immediately, and then restarts the server.
-          - C(GracefulRestart) performs graceful shutdown of the device, and then restarts the device.
-          - C(GracefulShutdown) performs a graceful shutdown of the device, and then turns off the device.
-          - C(Nmi) sends a diagnostic interrupt to the device. This option is usually a nonmaskable interrupt (NMI) on x86 systems.
+          - C(ForceRestart) turns off the device immediately,
+            and then restarts the server.
+          - C(GracefulRestart) performs graceful shutdown of the device,
+            and then restarts the device.
+          - C(GracefulShutdown) performs a graceful shutdown of the device,
+            and then turns off the device.
+          - C(Nmi) sends a diagnostic interrupt to the device. This option is
+            usually a nonmaskable interrupt (NMI) on x86 systems.
           - C(On) turns on the device.
           - C(PowerCycle) performs a power cycle on the device.
-          - C(PushPowerButton) simulates the pressing of a physical power button on the device.
+          - C(PushPowerButton) simulates the pressing of a physical power
+            button on the device.
           - I(reset_type) is mutually exclusive with I(oem_reset_type).
-          - When a power control operation is performed, which is not supported on the device, an error message is displayed
-            with the list of operations that can be performed.
+          - When a power control operation is performed, which is not supported
+            on the device, an error message is displayed with the list of
+            operations that can be performed.
         required: false
         type: str
         choices:
@@ -73,10 +85,12 @@ argument_specs:
           ]
       oem_reset_type:
         description:
-          - This parameter initiates a complete Alternate Current (AC) power cycle of the server which is equivalent to disconnecting power
+          - This parameter initiates a complete Alternate Current (AC) power
+            cycle of the server which is equivalent to disconnecting power
             cables using OEM API.
           - I(oem_reset_type) is mutually exclusive with I(reset_type).
-          - If the value of 'final_power_state' is not provided, the default value is 'Off'.
+          - If the value of 'final_power_state' is not provided, the default
+            value is 'Off'.
         required: false
         type: dict
         version_added: 9.5.0

tests/integration/targets/idrac_redfish_powerstate/tests/invalid.yaml

@@ -221,17 +221,36 @@
       dellemc.openmanage.redfish_powerstate:
         reset_type: "On"
 
-    - name: Negative - Performing oem_reset_type when server is powered On
+    - name: Performing oem_reset_type when server is powered On - Check Mode
+      dellemc.openmanage.redfish_powerstate:
+        oem_reset_type:
+          dell:
+            reset_type: "PowerCycle"
+            final_power_state: "Off"
+      check_mode: true
+      register: cm_powering_off_when_on
+      when: firmware_ver >= '7.00.60'
+
+    - name: Verify status of tasks - Check Mode
+      ansible.builtin.assert:
+        that:
+          - not cm_powering_off_when_on.changed
+          - error_msg == cm_powering_off_when_on.msg
+      vars:
+        error_msg: "No changes found to be applied because system is in power
+         ON state."
+      when: firmware_ver >= '7.00.60'
+
+    - name: Performing oem_reset_type when server is powered On - Normal Mode
       dellemc.openmanage.redfish_powerstate:
         oem_reset_type:
           dell:
             reset_type: "PowerCycle"
             final_power_state: "Off"
-      ignore_errors: true
       register: powering_off_when_on
       when: firmware_ver >= '7.00.60'
 
-    - name: Verify status of tasks
+    - name: Verify status of tasks - Normal Mode
       ansible.builtin.assert:
         that:
           - powering_off_when_on.skipped

tests/integration/targets/idrac_redfish_powerstate/tests/oem_reset_type_valid.yaml

@@ -30,13 +30,28 @@
       ansible.builtin.set_fact:
         firmware_ver: "{{ idrac_output.json.FirmwareVersion }}"
 
-    - name: Performing oem_reset_type when server is powered Off
-      dellemc.openmanage.redfish_powerstate:
+    - name: Performing oem_reset_type when server is powered Off - Check Mode
+      dellemc.openmanage.redfish_powerstate: &oem_reset_type_off
         oem_reset_type:
           dell:
             reset_type: "PowerCycle"
             final_power_state: "Off"
-      ignore_errors: true
+      check_mode: true
+      register: cm_powered_off
+      when: firmware_ver >= '7.00.60'
+
+    - name: Verify status of tasks - Check Mode
+      ansible.builtin.assert:
+        that:
+          - cm_powered_off.changed
+          - valid_msg == cm_powered_off.msg
+      vars:
+        valid_msg: "Changes found to be applied."
+      when: firmware_ver >= '7.00.60'
+
+    - name: Performing oem_reset_type when server is powered Off - Normal Mode
+      dellemc.openmanage.redfish_powerstate:
+        <<: *oem_reset_type_off
       register: powered_off
       when: firmware_ver >= '7.00.60'
 
@@ -52,7 +67,7 @@
       retries: 10
       delay: 30
 
-    - name: Verify status of tasks
+    - name: Verify status of tasks - Normal Mode
       ansible.builtin.assert:
         that:
           - powered_off.changed
@@ -63,13 +78,28 @@
           power-cycle operation."
       when: firmware_ver >= '7.00.60'
 
-    - name: Performing oem_reset_type when server is powered On
-      dellemc.openmanage.redfish_powerstate:
+    - name: Performing oem_reset_type when server is powered On - Check Mode
+      dellemc.openmanage.redfish_powerstate: &oem_reset_type_on
         oem_reset_type:
           dell:
             reset_type: "PowerCycle"
             final_power_state: "On"
-      ignore_errors: true
+      check_mode: true
+      register: cm_powered_on
+      when: firmware_ver >= '7.00.60'
+
+    - name: Verify status of tasks - Check Mode
+      ansible.builtin.assert:
+        that:
+          - cm_powered_on.changed
+          - valid_msg == cm_powered_on.msg
+      vars:
+        valid_msg: "Changes found to be applied."
+      when: firmware_ver >= '7.00.60'
+
+    - name: Performing oem_reset_type when server is powered On - Normal Mode
+      dellemc.openmanage.redfish_powerstate:
+        <<: *oem_reset_type_on
       register: powered_on
       when: firmware_ver >= '7.00.60'