Skip to content
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

Add module redfish_command #43469

Merged
merged 7 commits into from
Aug 30, 2018
Merged

Add module redfish_command #43469

merged 7 commits into from
Aug 30, 2018

Conversation

jose-delarosa
Copy link
Contributor

@jose-delarosa jose-delarosa commented Jul 31, 2018

SUMMARY

Communicates with Out-Of-Band Controller through Redfish APIs
Sends a command to execute an action

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

redfish_command

ANSIBLE VERSION
ansible 2.7.0.dev0 (redfish_config ef50194a65) last updated 2018/07/30 17:19:04 (GMT -500)
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/root/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /share1/git/ansible/lib/ansible
  executable location = /share1/git/ansible/bin/ansible
  python version = 2.7.5 (default, Feb 20 2018, 09:19:12) [GCC 4.8.5 20150623 (Red Hat 4.8.5-28)]

ADDITIONAL INFORMATION

This PR is part of a group of modules. PR for first module is #41656 (merged).

Communicates with Out-Of-Band Controller through Redfish APIs
Sends a command to execute an action
@ansibot ansibot added affects_2.7 This issue/PR affects Ansible v2.7 community_review In order to be merged, this PR must follow the community review workflow. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. new_module This PR includes a new module. new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community. labels Jul 31, 2018
@jose-delarosa
Copy link
Contributor Author

ready_for_review

@ryansb ryansb removed the needs_triage Needs a first human triage before being processed. label Jul 31, 2018
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Aug 8, 2018
@samdoran samdoran self-requested a review August 14, 2018 20:01
Copy link
Contributor

@samdoran samdoran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see anywhere this module reports a change. If this is just sending arbitrary commands without a way to validate state, it should probably always report changed=True.

# execute only if we find an Account service resource
result = rf_utils._find_accountservice_resource(rf_uri)
if result['ret'] is False:
module.fail_json(msg=result['msg'])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrap all instances of result['msg'] within fail_json() in to_native().

# execute only if we find a Manager service resource
result = rf_utils._find_managers_resource(rf_uri)
if result['ret'] is False:
module.fail_json(msg=result['msg'])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming the output in result['msg'] is fairly descriptive of the specific type of error so that the user could tell what went wrong and how to fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The output of result['msg'] varies in case of error, it returns an HTTPError or URLError code based on different scenarios (in older OOB controllers some APIs are not available --> 404).

version_added: "2.7"
short_description: Manages Out-Of-Band controllers using Redfish APIs
description:
- Builds Redfish URIs locally and sends them to remote OOB controllers to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a bit more descriptive of what this module specifically does. Most modules build some sort of command or URI, so this doesn't really describe what this module actually does without inferring it from reading the documentation.

@ansibot ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Aug 15, 2018
@ansibot
Copy link
Contributor

ansibot commented Aug 15, 2018

The test ansible-test sanity --test ansible-doc --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/remote_management/redfish/redfish_command.py:0:0: missing documentation (or could not parse documentation): sequence item 1: expected string or Unicode, AnsibleMapping found

The test ansible-test sanity --test ansible-doc --python 2.7 [explain] failed with 1 error:

lib/ansible/modules/remote_management/redfish/redfish_command.py:0:0: missing documentation (or could not parse documentation): sequence item 1: expected string or Unicode, AnsibleMapping found

The test ansible-test sanity --test ansible-doc --python 3.5 [explain] failed with 1 error:

lib/ansible/modules/remote_management/redfish/redfish_command.py:0:0: missing documentation (or could not parse documentation): sequence item 1: expected str instance, AnsibleMapping found

The test ansible-test sanity --test ansible-doc --python 3.6 [explain] failed with 1 error:

lib/ansible/modules/remote_management/redfish/redfish_command.py:0:0: missing documentation (or could not parse documentation): sequence item 1: expected str instance, AnsibleMapping found

The test ansible-test sanity --test ansible-doc --python 3.7 [explain] failed with 1 error:

lib/ansible/modules/remote_management/redfish/redfish_command.py:0:0: missing documentation (or could not parse documentation): sequence item 1: expected str instance, AnsibleMapping found

The test ansible-test sanity --test docs-build [explain] failed with the error:

Command "/usr/bin/python test/sanity/code-smell/docs-build.py" returned exit status 1.
>>> Standard Error
Traceback (most recent call last):
  File "test/sanity/code-smell/docs-build.py", line 100, in <module>
    main()
  File "test/sanity/code-smell/docs-build.py", line 17, in main
    raise subprocess.CalledProcessError(sphinx.returncode, cmd, output=stdout, stderr=stderr)
subprocess.CalledProcessError: Command '['make', 'singlehtmldocs']' returned non-zero exit status 2.

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/modules/remote_management/redfish/redfish_command.py:255:1: E305 expected 2 blank lines after class or function definition, found 1

The test ansible-test sanity --test validate-modules [explain] failed with 3 errors:

lib/ansible/modules/remote_management/redfish/redfish_command.py:0:0: E305 DOCUMENTATION.description.1: expected str @ data['description'][1]. Got {'Manages OOB controller': 'Reboot, log management'}
lib/ansible/modules/remote_management/redfish/redfish_command.py:0:0: E305 DOCUMENTATION.description.2: expected str @ data['description'][2]. Got {'Manages OOB controller users': 'add, remove, update user properties and status'}
lib/ansible/modules/remote_management/redfish/redfish_command.py:0:0: E305 DOCUMENTATION.description.3: expected str @ data['description'][3]. Got {'Manages system power': 'Power on/off, graceful and forced reboot'}

click here for bot help

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Aug 15, 2018
@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Aug 16, 2018
@jose-delarosa
Copy link
Contributor Author

jose-delarosa commented Aug 16, 2018

Wow that was ugly, learned not to add colons where they don't belong in the DOCUMENTATION section.

@samdoran please let me know if there is anything else. When a URI is sent there is some upfront checking (Is URI valid? Does Redfish API exist?). If one of the HTTP methods fails, that is also captured and sent back.

However, if for example I send a command to create a user, and everything succeeds ( result['ret'] = True) there is no way to verify if the user was actually created, not unless I send a separate ListUsers command to verify. It is assumed that if all checks pass, the action (command or config) was executed successfully.

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Aug 24, 2018
@samdoran
Copy link
Contributor

I would recommend the module reports changed every time as long as there is not a failure since you're not able to verify based on the data back from the API if a change was actually made.

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Aug 24, 2018
@ansibot ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Aug 30, 2018
@samdoran samdoran merged commit fdb1d11 into ansible:devel Aug 30, 2018
@jose-delarosa
Copy link
Contributor Author

Thanks for your review @samdoran

@jose-delarosa jose-delarosa deleted the redfish_command branch September 14, 2018 02:56
@jose-delarosa jose-delarosa restored the redfish_command branch September 14, 2018 02:56
@jose-delarosa jose-delarosa deleted the redfish_command branch September 14, 2018 03:52
@ansible ansible locked and limited conversation to collaborators Jul 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.7 This issue/PR affects Ansible v2.7 module This issue/PR relates to a module. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. new_module This PR includes a new module. new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants