Added nd_remote_storage_location module to manage remote storage locations (DCNE-503) #165

sajagana · 2025-08-08T15:02:39Z

…

…te storage locations

shrsr · 2025-08-11T17:50:21Z

plugins/modules/nd_remote_storage_location.py

+        type: str
+  nas:
+    description:
+    - The Network-attached storage (NAS) configuration for the remote storage location.


Can we change this to?

The Network Attached Storage (NAS) configuration for the remote storage location.

…_location module

anvitha-jain · 2025-08-12T16:43:25Z

tests/integration/targets/nd_remote_storage_location/tasks/main.yml

+    output_level: info
+  register: nm_add_scp_remote_location_again
+
+- name: Query nas remote storage location to ensure it does not exists


is this required?

When I create or delete the NAS storage location, ND displays the message "There was a problem proxying the request." This is a temporary error and typically resolves itself after a few minutes. To handle the inconsistent behavior, I have added the necessary query task and included additional ignore error statements.

anvitha-jain · 2025-08-12T16:43:45Z

tests/integration/targets/nd_remote_storage_location/tasks/main.yml

+  check_mode: true
+  register: cm_add_nas_remote_location
+  when: query_nas_remote_location.current == {}
+  ignore_errors: true


why ignore_errors: true in check mode?

When I create or delete the NAS storage location, ND displays the message "There was a problem proxying the request." This is a temporary error and typically resolves itself after a few minutes. To handle the inconsistent behavior, I have added the necessary query task and included additional ignore error statements.

If that's the case, then can we add it a comment in the test file or make a note of this somewhere?

…mote_storage_location module test file

shrsr · 2025-08-14T18:50:06Z

plugins/modules/nd_remote_storage_location.py

+
+        if not module.check_mode:
+            if nd.existing:
+                nd.request(path, method="PUT", data=payload)


Is PUT op called even for the same configurations?
If yes, shall we leverage get_diff()?

Thank you for catching this!

…e and copied module utils nd.py file changes from CiscoDevNet#166 PR

akinross · 2025-08-18T08:58:03Z

plugins/module_utils/nd.py

@@ -3,6 +3,7 @@
 # Copyright: (c) 2021, Lionel Hercot (@lhercot) <[email protected]>
 # Copyright: (c) 2022, Cindy Zhao (@cizhao) <[email protected]>
 # Copyright: (c) 2022, Akini Ross (@akinross) <[email protected]>
+# Copyright: (c) 2025, Shreyas Srish (@shrsr) <[email protected]>


should this not be your details?

I copied the changes from Shreyas PR, let it be for now.

akinross · 2025-08-18T09:01:46Z

plugins/modules/nd_remote_storage_location.py

+    aliases: [ default_path, export_path ]
+  sftp_scp:
+    description:
+    - The SFTP/SCP configuration for the remote storage location.


should abbreviations be explained here like you do for NAS?

akinross · 2025-08-18T09:09:09Z

plugins/modules/nd_remote_storage_location.py

+    - The SFTP/SCP configuration for the remote storage location.
+    - This parameter and O(nas) are mutually exclusive.
+    type: dict
+    suboptions:


is there a reason that credential store key is not an option?

Currently credential store is not available to test the behaviour; it will be added once the credential store is available.

akinross · 2025-08-18T09:11:52Z

plugins/modules/nd_remote_storage_location.py

+  server_port:
+    description:
+    - The port number of the remote server.
+    type: int
+  server_name:
+    description:
+    - The IP address or hostname of the remote server.
+    type: str
+    aliases: [ ip ]
+  path:
+    description:
+    - The export path of the remote storage location.
+    type: str
+    aliases: [ default_path, export_path ]


from UI these seem required upon initial create? should we document / enforce this in input?

Added note in the document.

akinross · 2025-08-18T09:13:02Z

plugins/modules/nd_remote_storage_location.py

+        argument_spec=argument_spec,
+        supports_check_mode=True,
+        required_if=[
+            ["state", "present", ["name"]],


certain options seem to not be changeable after create from UI should we document this somehow?

…age_location module documentation

shrsr

LGTM

akinross

LGTM

gmicol · 2025-08-19T23:05:42Z

plugins/module_utils/utils.py

+def delete_none_values(obj_to_sanitize, recursive=True):
+    """
+    Removes keys with None values from a Python object, which can be either a list or a dictionary.
+    Optionally performs the operation recursively on nested structures.
+
+    :param obj_to_sanitize: The Python object to sanitize from None values. -> List or Dict
+    :param recursive: A boolean flag indicating whether to recursively sanitize nested objects. Defaults to True. -> bool
+    :return: A sanitized copy of the original Python object, with all keys with None values removed. -> List or Dict
+    """
+    if isinstance(obj_to_sanitize, dict):
+        sanitized_dict = {}
+        for item_key, item_value in obj_to_sanitize.items():
+            if recursive and isinstance(item_value, (dict, list)):
+                sanitized_dict[item_key] = delete_none_values(item_value, recursive)
+            elif item_value is not None:
+                sanitized_dict[item_key] = item_value
+        return sanitized_dict
+
+    elif isinstance(obj_to_sanitize, list):
+        sanitized_list = []
+        for item in obj_to_sanitize:
+            if recursive and isinstance(item, (dict, list)):
+                sanitized_list.append(delete_none_values(item, recursive))
+            elif item is not None:
+                sanitized_list.append(item)
+        return sanitized_list
+
+    else:
+        raise TypeError("Object to sanitize must be of type list or dict. Got {}".format(type(obj_to_sanitize)))


Why adding this function when we already have the sanitize (with sanitize_dict and sanitize_list) function in nd.py which does the same thing and expand that even more (and I am not talking about nd.sanitize)? maybe we should rename this existing function?

I do agree with moving a lot of the utility functions from nd.py to utils.py but we should be careful about making "duplicate" functions across the module_utils

Yes, I removed unused function from utils.py.

gmicol

LGTM

anvitha-jain

LGTM

shrsr

LGTM

samiib · 2025-08-27T13:08:03Z

tests/integration/targets/nd_remote_storage_location/tasks/main.yml

@sajagana sent you a question about the tests.

shrsr

LGTM

samiib

LGTM

anvitha-jain · 2025-09-02T18:36:20Z

tests/integration/targets/nd_remote_storage_location/tasks/main.yml

+    output_level: info
+  register: nm_add_scp_remote_location_again
+
+# To manage the NAS storage inconsistent 'There was a problem proxying the request' errors message, added ignore_errors and until statements.


Rephrase the comment, to make it more clear

To address the inconsistent 'There was a problem proxying the request' errors with NAS storage, 'ignore_errors' and 'until' statements were added.

…est file

[minor_change] Added nd_remote_storage_location module to manage remo…

fab4c95

…te storage locations

sajagana requested review from akinross, anvitha-jain, gmicol, lhercot, samiib and shrsr as code owners August 8, 2025 15:02

sajagana changed the title ~~Added nd_remote_storage_location module to manage remote storage locations~~ Added nd_remote_storage_location module to manage remote storage locations (DCNE-503) Aug 8, 2025

shrsr requested changes Aug 11, 2025

View reviewed changes

[ignore] Fixed the nas attribute description in the nd_remote_storage…

d626715

…_location module

sajagana requested a review from shrsr August 12, 2025 05:23

anvitha-jain requested changes Aug 12, 2025

View reviewed changes

sajagana requested a review from anvitha-jain August 13, 2025 15:10

[ignore] Added note to the NAS remote storage location tasks in nd_re…

b88d950

…mote_storage_location module test file

shrsr requested changes Aug 14, 2025

View reviewed changes

[ignore] Changed the update logic of nd_remote_storage_location modul…

d1ae8df

…e and copied module utils nd.py file changes from CiscoDevNet#166 PR

sajagana requested a review from shrsr August 18, 2025 04:43

akinross requested changes Aug 18, 2025

View reviewed changes

[ignore] Changed a few attributes name and updated the nd_remote_stor…

fa38e0f

…age_location module documentation

sajagana requested a review from akinross August 19, 2025 08:06

shrsr approved these changes Aug 19, 2025

View reviewed changes

akinross approved these changes Aug 19, 2025

View reviewed changes

gmicol requested changes Aug 19, 2025

View reviewed changes

[ignore] Removed delete_none_values function from utils.py

eeaae91

sajagana requested review from gmicol, shrsr and akinross August 20, 2025 08:16

gmicol approved these changes Aug 20, 2025

View reviewed changes

anvitha-jain approved these changes Aug 20, 2025

View reviewed changes

shrsr approved these changes Aug 26, 2025

View reviewed changes

samiib reviewed Aug 27, 2025

View reviewed changes

[ignore] Added remote_storage_location details to the inventory file

c6d90f9

sajagana requested review from samiib, shrsr, anvitha-jain and gmicol August 29, 2025 10:36

shrsr approved these changes Aug 29, 2025

View reviewed changes

samiib approved these changes Aug 31, 2025

View reviewed changes

anvitha-jain requested changes Sep 2, 2025

View reviewed changes

sajagana force-pushed the 503_remote_storage_location branch from c81e6d2 to 402e844 Compare September 11, 2025 10:40

[ignore] Rephrased the comment on nd_remote_storage_location module t…

6015f92

…est file

sajagana force-pushed the 503_remote_storage_location branch from 402e844 to 6015f92 Compare September 11, 2025 10:42

Added nd_remote_storage_location module to manage remote storage locations (DCNE-503) #165

Are you sure you want to change the base?

Added nd_remote_storage_location module to manage remote storage locations (DCNE-503) #165

Uh oh!

Conversation

sajagana commented Aug 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shrsr left a comment

Choose a reason for hiding this comment

Uh oh!

akinross left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmicol left a comment

Choose a reason for hiding this comment

Uh oh!

anvitha-jain left a comment

Choose a reason for hiding this comment

Uh oh!

shrsr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shrsr left a comment

Choose a reason for hiding this comment

Uh oh!

samiib left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!