Add a new module nd_local_user for local users on Nexus Dashboard (DCNE-524) #168
Conversation
gmicol
requested review from
akinross,
anvitha-jain,
lhercot,
sajagana,
samiib and
shrsr
as code owners
gmicol
added
enhancement
…s Dashboard v4.1.0 or higher.
gmicol
force-pushed
the
add_nd_local_user
branch
from
2585919 to
60f2d00
Compare
lhercot
added
jira-sync
github-actions
bot
changed the title
plugins/modules/nd_local_user.py
Outdated
| - name: Query all local users | ||
| cisco.nd.nd_local_user: | ||
| state: query | ||
| register: all_keys |
There was a problem hiding this comment.
| register: all_keys | |
| register: query_all |
| - ansible_local_user | ||
| - ansible_local_user_2 | ||
|
|
||
| #CREATE |
There was a problem hiding this comment.
| #CREATE | |
| # CREATE |
Fix all other places.
plugins/modules/nd_local_user.py
Outdated
| elif state == "absent": | ||
| if nd.existing: | ||
| if not module.check_mode: | ||
| nd.request(path="{0}/{1}".format(path, login_id), method="DELETE") |
There was a problem hiding this comment.
Can you use updated_path instead of formatting the path again?
There was a problem hiding this comment.
you're right, forgot to fix that
plugins/modules/nd_local_user.py
Outdated
| --- | ||
| module: nd_local_user | ||
| version_added: "1.4.0" | ||
| short_description: Manage local users on Nexus Dashboard |
There was a problem hiding this comment.
| short_description: Manage local users on Nexus Dashboard | |
| short_description: Manage local users on Cisco Nexus Dashboard |
plugins/modules/nd_local_user.py
Outdated
| state: query | ||
| register: query_all | ||
| - name: Delete an local user |
There was a problem hiding this comment.
Can we change to?
Delete a local user
plugins/modules/nd_local_user.py
Outdated
|
|
||
| if not module.check_mode: | ||
| if nd.existing: | ||
| nd.existing = nd.request(path=updated_path, method="PUT", data=payload) |
There was a problem hiding this comment.
Is PUT op called even for the same configuration?
If yes, shall we leverage get_diff()?
There was a problem hiding this comment.
Added get_diff() function for pre-checking PUT operation
| }, | ||
| } | ||
| if reuse_limitation or time_interval_limitation: | ||
| payload["passwordPolicy"] = sanitize_dict( |
There was a problem hiding this comment.
Is sanitize_dict() required here when we sanitize the entire payload in the next step?
There was a problem hiding this comment.
It's required as nd.sanitize() is not recursive and just sanitize one level deep which is not enough in this case. Maybe, we should address this issue in the future so that the nd.sanitize() can be recursive as well
plugins/modules/nd_local_user.py
Outdated
| suboptions: | ||
| name: | ||
| description: | ||
| - The name of the Security Domain to which give the local user access. |
There was a problem hiding this comment.
Can we please change this to... if it's more meaningful?
- The name of the Security Domain to which the local user is given access.
…ance Documentation and test file.
| state: present | ||
| register: result | ||
| - name: Create local user with minimal configuration |
There was a problem hiding this comment.
Is this a new standard? It's not that I disagree with it, but should we then track this as new thing we want to introduce to all our modules?
There was a problem hiding this comment.
This is something that was introduced with the new nd_api_key module. So I thought this has become the new standard but if not, we should discuss if we should add this creation task with minimum config in the EXAMPLES section.
I think this is interesting to have as it makes visually clearer what attributes are required when creating a new object.
@sajagana, @shrsr, @samiib, @lhercot, @anvitha-jain what do you think?
There was a problem hiding this comment.
I have no preference on this
plugins/modules/nd_local_user.py
Outdated
|
|
||
| path = "/api/v1/infra/aaa/localUsers" | ||
| if login_id: | ||
| updated_path = "{0}/{1}".format(path, login_id) |
There was a problem hiding this comment.
can we rename this to login_id_path or something? think updated is to generic
| payload["rbac"] = { | ||
| "domains": { | ||
| security_domain.get("name"): { | ||
| "roles": [USER_ROLES_MAPPING.get(role) for role in security_domain["roles"]] if isinstance(security_domain.get("roles"), list) else [], |
There was a problem hiding this comment.
is empty list required to be set in roles, or should we also sanitize this part?
There was a problem hiding this comment.
This is the only way I found to provide an empty list of roles assigned to a security domain. Assigning an None value to "roles" or sanitizing the key will result in an API error.
Also, as providing an empty list works, I wonder if this is a normal behavior or a bug as it seems weird to not assign any role to a local user for a security domain.
plugins/modules/nd_local_user.py
Outdated
| short_description: Manage local users on Cisco Nexus Dashboard | ||
| description: | ||
| - Manage local users on Cisco Nexus Dashboard (ND). | ||
| - It supports creating, updating, querying, and deleting local users. |
There was a problem hiding this comment.
| - It supports creating, updating, querying, and deleting local users. | |
| - This module supports creating, updating, querying, and deleting local users. |
plugins/modules/nd_local_user.py
Outdated
| - cisco.nd.modules | ||
| - cisco.nd.check_mode | ||
| notes: | ||
| - This module is only supported on Nexus Dashboard having version 4.1.0 or higher. |
There was a problem hiding this comment.
Should this note be in the module description instead?
- This module is only supported on ND v4.1.0 and later.
Our ansible-mso modules are worded like this, should we keep it similar?
gmicol
requested review from
samiib,
shrsr,
akinross,
anvitha-jain and
sajagana
This module is only supported on ND v.4.1.0 or higher.