-
Notifications
You must be signed in to change notification settings - Fork 21
Add a new module nd_local_user for local users on Nexus Dashboard (DCNE-524) #168
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 1 commit
60f2d00
5563927
8d8666c
6719346
5454245
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,274 @@ | ||||||
| #!/usr/bin/python | ||||||
| # -*- coding: utf-8 -*- | ||||||
|
|
||||||
| # Copyright: (c) 2025, Gaspard Micol (@gmicol) <[email protected]> | ||||||
|
|
||||||
| # GNU General Public License v3.0+ (see LICENSE or https://www.gnu.org/licenses/gpl-3.0.txt) | ||||||
|
|
||||||
| from __future__ import absolute_import, division, print_function | ||||||
|
|
||||||
| __metaclass__ = type | ||||||
|
|
||||||
| ANSIBLE_METADATA = {"metadata_version": "1.1", "status": ["preview"], "supported_by": "community"} | ||||||
|
|
||||||
| DOCUMENTATION = r""" | ||||||
| --- | ||||||
| module: nd_local_user | ||||||
| version_added: "1.4.0" | ||||||
| short_description: Manage local users on Nexus Dashboard | ||||||
| description: | ||||||
| - Manage local users on Cisco Nexus Dashboard (ND). | ||||||
| - It supports creating, updating, querying, and deleting local users. | ||||||
|
||||||
| - It supports creating, updating, querying, and deleting local users. | |
| - This module supports creating, updating, querying, and deleting local users. |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no preference on this
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use one of the ways? - either camelCase or snake ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| register: all_keys | |
| register: query_all |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change to?
Delete a local user
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we rename this to login_id_path or something? think updated is to generic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is empty list required to be set in roles, or should we also sanitize this part?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is sanitize_dict() required here when we sanitize the entire payload in the next step?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is PUT op called even for the same configuration?
If yes, shall we leverage get_diff()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added get_diff() function for pre-checking PUT operation
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use updated_path instead of formatting the path again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're right, forgot to fix that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.