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 commands GetVolumeInventory / GetSessions / GetVirtualMedia #55145

Closed
wants to merge 8 commits into from

Conversation

Projects
None yet
4 participants
@renxulei
Copy link

commented Apr 11, 2019

SUMMARY

Add new commands GetVolumeInventory / GetSessions / GetVirtualMedia for redfish_facts module

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

redfish_facts

ADDITIONAL INFORMATION

@ansibot

This comment has been minimized.

@renxulei
Copy link
Author

left a comment

for PR#55145, I fixed the issues reported by sanity check, please help to check it again.

@billdodd
Copy link
Contributor

left a comment

Thanks for submitting the PR. Looks pretty good. I did have some requested changes. See inline comments below.

Show resolved Hide resolved lib/ansible/module_utils/redfish_utils.py
Show resolved Hide resolved lib/ansible/module_utils/redfish_utils.py Outdated
Show resolved Hide resolved lib/ansible/module_utils/redfish_utils.py Outdated
Show resolved Hide resolved lib/ansible/module_utils/redfish_utils.py Outdated
@@ -301,6 +315,8 @@ def main():
for command in command_list:
if command == "GetManagerNicInventory":
result["manager_nics"] = rf_utils.get_multi_nic_inventory(category)
elif command == "GetVirtualMedia":
result["virtual_media"] = rf_utils.get_multi_virtualmedia(category)

This comment has been minimized.

Copy link
@billdodd

billdodd Apr 12, 2019

Contributor

Since GetVirtualMedia is only a command in the "Manager" category (and not in the "Systems" category), you do not need to pass in a category param here. See also my previous get_multi_virtualmedia() comment in redfish_utils.py.

This comment has been minimized.

Copy link
@renxulei

renxulei Apr 13, 2019

Author

Thanks for your detailed explanation, billdodd. I will remove category parameter. Sine aggregate() is only for systems, I plan to keep get_multi_virtualmedia(), but I will remove all systems related logic. And I also found 1 issue in _find_managers_resource(), which can only keep last Manager uri in self.manager_uri, not one list. because manager_uri is being used by multiple positions, I plan to fix it in another PR.

This comment has been minimized.

Copy link
@billdodd

billdodd Apr 15, 2019

Contributor

@renxulei Yes, sorry about that. I missed that this was for Managers rather that Systems. Your update looks good. When you do the future PR to improve the handling of multiple managers, it may be a good idea the rename the aggregate() method to aggregate_systems() and create a new aggregate_managers() method.

This comment has been minimized.

Copy link
@renxulei

renxulei Apr 16, 2019

Author

@billdodd Thanks for your suggestion, I will fix the code as you suggested in future PR.

renxulei and others added some commits Apr 13, 2019

Update lib/ansible/module_utils/redfish_utils.py
Co-Authored-By: renxulei <2663829144@qq.com>
@renxulei
Copy link
Author

left a comment

@billdodd Update the codes as comments, please help to check it again, thanks!

@billdodd
Copy link
Contributor

left a comment

Reviewed and tested update. Looks good.

shipit

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

@renxulei this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

Revert "Merge remote-tracking branch 'upstream/devel' into devel"
This reverts commit e76fa3b, reversing
changes made to bf988db.
@ansibot

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

@renxulei This PR was evaluated as a potentially problematic PR for the following reasons:

  • More than 50 changed files.

Such PR can only be merged by human. Contact a Core team member to review this PR on IRC: #ansible-devel on irc.freenode.net

click here for bot help

@openstack-zuul

This comment has been minimized.

Copy link

commented Apr 16, 2019

Build succeeded (third-party-check pipeline).

@jose-delarosa

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

@renxulei I think you need to close this PR, something is definitely wrong here.

@renxulei

This comment has been minimized.

Copy link
Author

commented Apr 16, 2019

@jose-delarosa thanks for your confirmation, I thought I made misoperation on my forked project, I will close this PR.
@billdodd thanks for your review and suggestions, I will recover my project and create another PR to submit the codes. thanks for your help!

@renxulei renxulei closed this Apr 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.