Repository: ansible/ansible
Pull request: Changes to redfish-based parameters
Corrected redfish parameters to their Schematic counterparts

##### SUMMARY
Changes all playbook related variables to counterparts related to their Redfish terminology as best as possible, in order to maintain a similar vocabulary between the playbooks and Redfish.  In the event of conflicting terms, prefix with appropriate information.

user -> username
userid -> id
username -> new_username
password -> new_password
userrole -> roleid
bios_attr_name -> bios_attribute_name
mgr_attr_name -> manager_attribute_name
bios_attr_name -> bios_attribute_name
mgr_attr_name -> manager_attribute_name

Fixes #46549

##### ISSUE TYPE
- Bugfix Pull Request

##### COMPONENT NAME
lib/ansible/modules/remote_management/redfish

##### ADDITIONAL INFORMATION
No changed behavior.

Problem statement:
Make the Redfish input parameters consistent with the Redfish property names
<!--- Verify first that your issue is not already reported on GitHub -->
<!--- Also test if the latest release and devel branch are affected too -->
<!--- Complete *all* sections as described, this form is processed automatically -->

##### SUMMARY
<!--- Explain the problem briefly below -->
Some of the Redfish module input parameters correspond to Redfish property names. For example,
in the redfish_command.py module there are input parameters `username`, `userpswd` and `userrole`. These parameters correspond to Redfish properties `UserName`, `Password` and `RoleId` respectively. (This is not a complete list of the impacted parameters. Will need to scan the parameters of all 3 Redfish modules to get a full list.)

To the degree possible within the constraints of the Ansible and Python coding guidelines (like PEP8), these parameters should match their corresponding Redfish property names. By maintaining this consistency users of the Redfish modules will have an easier and more intuitive experience.

Note that the example above has some inherent difficulty. There are 2 input parameters for username and password each. One set for the account being acted on (created, updated or deleted) and one set for the basic auth credentials being supplied for the operation. So an exception or compromise will have to be made for that particular set of parameters.

And as suggested above, the parameter names may not be able to match the case of the Redfish property names due to PEP8 guidelines.


##### ISSUE TYPE
- Bug Report

##### COMPONENT NAME
<!--- Write the short name of the module, plugin, task or feature below -->
redfish_command.py
redfish_config.py
redfish_facts.py

##### ANSIBLE VERSION
<!--- Paste verbatim output from "ansible --version" between quotes -->
```paste below
ansible 2.8.0.dev0 (devel 693f3219d4) last updated 2018/09/20 12:07:29 (GMT -500)
  config file = None
  configured module search path = ['/Users/bdodd/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /Users/bdodd/Development/git/ansible/lib/ansible
  executable location = /Users/bdodd/Development/git/ansible/bin/ansible
  python version = 3.6.5 (default, Apr 25 2018, 14:26:36) [GCC 4.2.1 Compatible Apple LLVM 9.0.0 (clang-900.0.39.2)]
```

##### CONFIGURATION
<!--- Paste verbatim output from "ansible-config dump --only-changed" between quotes -->
```paste below
[no output]
```

##### OS / ENVIRONMENT
<!--- Provide all relevant information below, e.g. target OS versions, network device firmware, etc. -->
Targets are Redfish hosts.

##### STEPS TO REPRODUCE
<!--- Describe exactly how to reproduce the problem, using a minimal test-case -->
Run a playbook that has input parameters that correspond to Redfish properties and compare the parameter names used to the corresponding Redfish properties

<!--- Paste example playbooks or commands between quotes below -->
```yaml
---
- hosts: myhosts
  connection: local
  name: Add User
  gather_facts: False

  # User to add, modify or delete is defined in file group_vars/all

  tasks:

  # When adding a user, you must enable it afterwards
  - name: Add user
    redfish_command:
      category: Accounts
      command: AddUser
      baseuri: "{{ baseuri }}"
      user: "{{ user }}"
      password: "{{ password }}"
      userid: "{{ userid }}"
      username: "{{ username }}"
      userpswd: "{{ userpswd }}"
      userrole: "{{ userrole }}"
```

<!--- HINT: You can paste gist.github.com links for larger files -->

##### EXPECTED RESULTS
<!--- Describe what you expected to happen when running the steps above -->
The parameter names used should match (as closely as possible) the Redfish property names.


##### ACTUAL RESULTS
<!--- Describe what actually happened. If possible run with extra verbosity (-vvvv) -->
Some of the parameter names used do not match the Redfish property names.

<!--- Paste verbatim command output between quotes -->
```paste below

```

Hints from issue discussion:
Files identified in the description:
* [lib/ansible/modules/remote_management/redfish/redfish_command.py](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/remote_management/redfish/redfish_command.py)
* [lib/ansible/modules/remote_management/redfish/redfish_config.py](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/remote_management/redfish/redfish_config.py)
* [lib/ansible/modules/remote_management/redfish/redfish_facts.py](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/remote_management/redfish/redfish_facts.py)

If these files are inaccurate, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->
cc @jose-delarosa @mraineri @tomasg2012
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
Here's a list of the potentially impacted input parameters and some notes on them.

## module redfish_command.py
- `user` / `password` - These params are used for the basic auth credentials of the operator issuing the command. There are property names in Redfish that correspond to these params (`UserName` and `Password`).
- `username` / `userpswd` - These params are used for the username and password of the account to add, change, etc. The corresponding Redfish property names are `UserName` and `Password`. But note that these will conflict with the property names for the `user` and `password` params in the above bullet, so of course cannot use the same params.
- `userrole` - This param corresponds to the Redfish property name `RoleId`.
- `userid` - This param corresponds to the Redfish property name `Id` in the context of an account. The account Ids are often integers (e.g. 1-16 for a system with 16 accounts.)
- `bootdevice` - This param corresponds to one of two Redfish property names, either `BootSourceOverrideTarget` or `UefiTargetBootSourceOverride`.

## module redfish_config.py
- `user` / `password` - See comment in module redfish_command.py above
- `bios_attr_name` / `bios_attr_value` - These params do not correspond directly to Redfish property names. They make up key/value pairs within the "Attributes" property.
- `mgr_attr_name` / `mgr_attr_value` - These params do not correspond directly to Redfish property names. They make up key/value pairs within the "Attributes" property.

## module redfish_facts.py
- `user` / `password` - See comment in module redfish_command.py above

Bill, agree that some parameter names don't adhere to Redfish standards. This is more up your alley, feel free to submit a PR for better alignment.
Jose - will do. I'm going to try to get some feedback from the tools group also.
For consistency, let's make them align with the properties found in the Redfish data model.

With regards to the parameters for adding an account, let's use "new_username" and "new_password" to not conflict with the credentials for the operation itself.

@tomasg2012 could you please work on this one?
For the Attribute related ones, let's go with "attribute_name" and "attribute_value" (since they are properties in the "Attributes" data structure).
@mraineri - Thanks for reviewing and commenting on the property names. I like the naming you suggested.

Commit message: Updated doc information with "version_added"

Patch to review:
diff --git a/lib/ansible/modules/remote_management/redfish/redfish_command.py b/lib/ansible/modules/remote_management/redfish/redfish_command.py
index 6508d5e9948de1..39581e26bb0564 100644
--- a/lib/ansible/modules/remote_management/redfish/redfish_command.py
+++ b/lib/ansible/modules/remote_management/redfish/redfish_command.py
@@ -35,30 +35,35 @@
     required: true
     description:
       - Base URI of OOB controller
-  user:
+  username:
     required: true
     description:
       - User for authentication with OOB controller
+    version_added: "2.8"
   password:
     required: true
     description:
       - Password for authentication with OOB controller
-  userid:
+  id:
     required: false
     description:
       - ID of user to add/delete/modify
-  username:
+    version_added: "2.8"
+  new_username:
     required: false
     description:
       - name of user to add/delete/modify
-  userpswd:
+    version_added: "2.8"
+  new_password:
     required: false
     description:
       - password of user to add/delete/modify
-  userrole:
+    version_added: "2.8"
+  roleid:
     required: false
     description:
       - role of user to add/delete/modify
+    version_added: "2.8"
   bootdevice:
     required: false
     description:
@@ -73,7 +78,7 @@
       category: Systems
       command: PowerGracefulRestart
       baseuri: "{{ baseuri }}"
-      user: "{{ user }}"
+      username: "{{ username }}"
       password: "{{ password }}"
 
   - name: Set one-time boot device to {{ bootdevice }}
@@ -82,7 +87,7 @@
       command: SetOneTimeBoot
       bootdevice: "{{ bootdevice }}"
       baseuri: "{{ baseuri }}"
-      user: "{{ user }}"
+      username: "{{ username }}"
       password: "{{ password }}"
 
   - name: Add and enable user
@@ -90,38 +95,38 @@
       category: Accounts
       command: AddUser,EnableUser
       baseuri: "{{ baseuri }}"
-      user: "{{ user }}"
-      password: "{{ password }}"
-      userid: "{{ userid }}"
       username: "{{ username }}"
-      userpswd: "{{ userpswd }}"
-      userrole: "{{ userrole }}"
+      password: "{{ password }}"
+      id: "{{ id }}"
+      new_username: "{{ new_username }}"
+      new_password: "{{ new_password }}"
+      roleid: "{{ roleid }}"
 
   - name: Disable and delete user
     redfish_command:
       category: Accounts
       command: ["DisableUser", "DeleteUser"]
       baseuri: "{{ baseuri }}"
-      user: "{{ user }}"
+      username: "{{ username }}"
       password: "{{ password }}"
-      userid: "{{ userid }}"
+      id: "{{ id }}"
 
   - name: Update user password
     redfish_command:
       category: Accounts
       command: UpdateUserPassword
       baseuri: "{{ baseuri }}"
-      user: "{{ user }}"
+      username: "{{ username }}"
       password: "{{ password }}"
-      userid: "{{ userid }}"
-      userpswd: "{{ userpswd }}"
+      id: "{{ id }}"
+      new_password: "{{ new_password }}"
 
   - name: Clear Manager Logs
     redfish_command:
       category: Manager
       command: ClearLogs
       baseuri: "{{ baseuri }}"
-      user: "{{ user }}"
+      username: "{{ username }}"
       password: "{{ password }}"
 '''
 
@@ -156,12 +161,12 @@ def main():
             category=dict(required=True),
             command=dict(required=True, type='list'),
             baseuri=dict(required=True),
-            user=dict(required=True),
+            username=dict(required=True),
             password=dict(required=True, no_log=True),
-            userid=dict(),
-            username=dict(),
-            userpswd=dict(no_log=True),
-            userrole=dict(),
+            id=dict(),
+            new_username=dict(),
+            new_password=dict(no_log=True),
+            roleid=dict(),
             bootdevice=dict(),
         ),
         supports_check_mode=False
@@ -171,14 +176,14 @@ def main():
     command_list = module.params['command']
 
     # admin credentials used for authentication
-    creds = {'user': module.params['user'],
+    creds = {'user': module.params['username'],
              'pswd': module.params['password']}
 
     # user to add/modify/delete
-    user = {'userid': module.params['userid'],
-            'username': module.params['username'],
-            'userpswd': module.params['userpswd'],
-            'userrole': module.params['userrole']}
+    user = {'userid': module.params['id'],
+            'username': module.params['new_username'],
+            'userpswd': module.params['new_password'],
+            'userrole': module.params['roleid']}
 
     # Build root URI
     root_uri = "https://" + module.params['baseuri']
diff --git a/lib/ansible/modules/remote_management/redfish/redfish_config.py b/lib/ansible/modules/remote_management/redfish/redfish_config.py
index 51aa8f18c6127b..146101fbd68f43 100644
--- a/lib/ansible/modules/remote_management/redfish/redfish_config.py
+++ b/lib/ansible/modules/remote_management/redfish/redfish_config.py
@@ -34,34 +34,39 @@
     required: true
     description:
       - Base URI of OOB controller
-  user:
+  username:
     required: true
     description:
       - User for authentication with OOB controller
+    version_added: "2.8"
   password:
     required: true
     description:
       - Password for authentication with OOB controller
-  bios_attr_name:
+  bios_attribute_name:
     required: false
     description:
       - name of BIOS attribute to update
     default: 'null'
-  bios_attr_value:
+    version_added: "2.8"
+  bios_attribute_value:
     required: false
     description:
       - value of BIOS attribute to update
     default: 'null'
-  mgr_attr_name:
+    version_added: "2.8"
+  manager_attribute_name:
     required: false
     description:
       - name of Manager attribute to update
     default: 'null'
-  mgr_attr_value:
+    version_added: "2.8"
+  manager_attribute_value:
     required: false
     description:
       - value of Manager attribute to update
     default: 'null'
+    version_added: "2.8"
 
 author: "Jose Delarosa (github: jose-delarosa)"
 '''
@@ -71,30 +76,30 @@
     redfish_config:
       category: Systems
       command: SetBiosAttributes
-      bios_attr_name: BootMode
-      bios_attr_value: Uefi
+      bios_attribute_name: BootMode
+      bios_attribute_value: Uefi
       baseuri: "{{ baseuri }}"
-      user: "{{ user }}"
+      username: "{{ username }}"
       password: "{{ password }}"
 
   - name: Set BootMode to Legacy BIOS
     redfish_config:
       category: Systems
       command: SetBiosAttributes
-      bios_attr_name: BootMode
-      bios_attr_value: Bios
+      bios_attribute_name: BootMode
+      bios_attribute_value: Bios
       baseuri: "{{ baseuri }}"
-      user: "{{ user }}"
+      username: "{{ username }}"
       password: "{{ password }}"
 
   - name: Enable PXE Boot for NIC1
     redfish_config:
       category: Systems
       command: SetBiosAttributes
-      bios_attr_name: PxeDev1EnDis
-      bios_attr_value: Enabled
+      bios_attribute_name: PxeDev1EnDis
+      bios_attribute_value: Enabled
       baseuri: "{{ baseuri }}"
-      user: "{{ user }}"
+      username: "{{ username }}"
       password: "{{ password }}"
 
   - name: Set BIOS default settings
@@ -102,37 +107,37 @@
       category: Systems
       command: SetBiosDefaultSettings
       baseuri: "{{ baseuri }}"
-      user: "{{ user }}"
+      username: "{{ username }}"
       password: "{{ password }}"
 
   - name: Enable NTP in the OOB Controller
     redfish_config:
       category: Manager
       command: SetManagerAttributes
-      mgr_attr_name: NTPConfigGroup.1.NTPEnable
-      mgr_attr_value: Enabled
+      manager_attribute_name: NTPConfigGroup.1.NTPEnable
+      manager_attribute_value: Enabled
       baseuri: "{{ baseuri }}"
-      user: "{{ user}}"
+      username: "{{ username}}"
       password: "{{ password }}"
 
   - name: Set NTP server 1 to {{ ntpserver1 }} in the OOB Controller
     redfish_config:
       category: Manager
       command: SetManagerAttributes
-      mgr_attr_name: NTPConfigGroup.1.NTP1
-      mgr_attr_value: "{{ ntpserver1 }}"
+      manager_attribute_name: NTPConfigGroup.1.NTP1
+      manager_attribute_value: "{{ ntpserver1 }}"
       baseuri: "{{ baseuri }}"
-      user: "{{ user}}"
+      username: "{{ username}}"
       password: "{{ password }}"
 
   - name: Set Timezone to {{ timezone }} in the OOB Controller
     redfish_config:
       category: Manager
       command: SetManagerAttributes
-      mgr_attr_name: Time.1.Timezone
-      mgr_attr_value: "{{ timezone }}"
+      manager_attribute_name: Time.1.Timezone
+      manager_attribute_value: "{{ timezone }}"
       baseuri: "{{ baseuri }}"
-      user: "{{ user}}"
+      username: "{{ username}}"
       password: "{{ password }}"
 '''
 
@@ -163,12 +168,12 @@ def main():
             category=dict(required=True),
             command=dict(required=True, type='list'),
             baseuri=dict(required=True),
-            user=dict(required=True),
+            username=dict(required=True),
             password=dict(required=True, no_log=True),
-            mgr_attr_name=dict(default='null'),
-            mgr_attr_value=dict(default='null'),
-            bios_attr_name=dict(default='null'),
-            bios_attr_value=dict(default='null'),
+            manager_attribute_name=dict(default='null'),
+            manager_attribute_value=dict(default='null'),
+            bios_attribute_name=dict(default='null'),
+            bios_attribute_value=dict(default='null'),
         ),
         supports_check_mode=False
     )
@@ -177,15 +182,15 @@ def main():
     command_list = module.params['command']
 
     # admin credentials used for authentication
-    creds = {'user': module.params['user'],
+    creds = {'username': module.params['username'],
              'pswd': module.params['password']}
 
     # Manager attributes to update
-    mgr_attributes = {'mgr_attr_name': module.params['mgr_attr_name'],
-                      'mgr_attr_value': module.params['mgr_attr_value']}
+    mgr_attributes = {'mgr_attr_name': module.params['manager_attribute_name'],
+                      'mgr_attr_value': module.params['manager_attribute_value']}
     # BIOS attributes to update
-    bios_attributes = {'bios_attr_name': module.params['bios_attr_name'],
-                       'bios_attr_value': module.params['bios_attr_value']}
+    bios_attributes = {'bios_attr_name': module.params['bios_attribute_name'],
+                       'bios_attr_value': module.params['bios_attribute_value']}
 
     # Build root URI
     root_uri = "https://" + module.params['baseuri']
diff --git a/lib/ansible/modules/remote_management/redfish/redfish_facts.py b/lib/ansible/modules/remote_management/redfish/redfish_facts.py
index 20b49737829110..734c571b64f13d 100644
--- a/lib/ansible/modules/remote_management/redfish/redfish_facts.py
+++ b/lib/ansible/modules/remote_management/redfish/redfish_facts.py
@@ -34,10 +34,11 @@
     required: true
     description:
       - Base URI of OOB controller
-  user:
+  username:
     required: true
     description:
       - User for authentication with OOB controller
+    version_added: "2.8"
   password:
     required: true
     description:
@@ -52,7 +53,7 @@
       category: Systems
       command: GetCpuInventory
       baseuri: "{{ baseuri }}"
-      user: "{{ user }}"
+      username: "{{ username }}"
       password: "{{ password }}"
 
   - name: Get fan inventory
@@ -60,13 +61,13 @@
       category: Chassis
       command: GetFanInventory
       baseuri: "{{ baseuri }}"
-      user: "{{ user }}"
+      username: "{{ username }}"
       password: "{{ password }}"
 
   - name: Get default inventory information
     redfish_facts:
       baseuri: "{{ baseuri }}"
-      user: "{{ user }}"
+      username: "{{ username }}"
       password: "{{ password }}"
 
   - name: Get several inventories
@@ -74,21 +75,21 @@
       category: Systems
       command: GetNicInventory,GetPsuInventory,GetBiosAttributes
       baseuri: "{{ baseuri }}"
-      user: "{{ user }}"
+      username: "{{ username }}"
       password: "{{ password }}"
 
   - name: Get default system inventory and user information
     redfish_facts:
       category: Systems,Accounts
       baseuri: "{{ baseuri }}"
-      user: "{{ user }}"
+      username: "{{ username }}"
       password: "{{ password }}"
 
   - name: Get default system, user and firmware information
     redfish_facts:
       category: ["Systems", "Accounts", "Update"]
       baseuri: "{{ baseuri }}"
-      user: "{{ user }}"
+      username: "{{ username }}"
       password: "{{ password }}"
 
   - name: Get all information available in the Manager category
@@ -96,7 +97,7 @@
       category: Manager
       command: all
       baseuri: "{{ baseuri }}"
-      user: "{{ user }}"
+      username: "{{ username }}"
       password: "{{ password }}"
 
   - name: Get all information available in all categories
@@ -104,7 +105,7 @@
       category: all
       command: all
       baseuri: "{{ baseuri }}"
-      user: "{{ user }}"
+      username: "{{ username }}"
       password: "{{ password }}"
 '''
 
@@ -147,14 +148,14 @@ def main():
             category=dict(type='list', default=['Systems']),
             command=dict(type='list'),
             baseuri=dict(required=True),
-            user=dict(required=True),
+            username=dict(required=True),
             password=dict(required=True, no_log=True),
         ),
         supports_check_mode=False
     )
 
     # admin credentials used for authentication
-    creds = {'user': module.params['user'],
+    creds = {'user': module.params['username'],
              'pswd': module.params['password']}
 
     # Build root URI


Code review comments:
diff_hunk: @@ -177,15 +182,15 @@ def main():
     command_list = module.params['command']
 
     # admin credentials used for authentication
-    creds = {'user': module.params['user'],
+    creds = {'username': module.params['username'],
path: lib/ansible/modules/remote_management/redfish/redfish_config.py
text: @user1:
Key name should stay "user":

```
    creds = {'user': module.params['username'],
```