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

266 idrac_network.py fails with AttributeError: 'NoneType' object has no attribute 'iDRAC' #268

Conversation

grantcurell
Copy link
Contributor

@grantcurell grantcurell commented Apr 16, 2021

README.md:

  • Added TOC
  • Based on user feedback took @anupamaloke 's answer and provided an explanation for the required shares along with how to easily handle the share
  • Changed the header Playbooks to Sample Playbooks so user's would have a better idea of what was there

dellemc_configure_idrac_eventing.py
dellemc_configure_idrac_services.py
dellemc_idrac_lc_attributes.py
dellemc_system_lockdown_mode.py
idrac_bios.py
idrac_firmware.py
idrac_network.py
idrac_syslog.pyidrac_timezone_ntp.py:

All files:

  • optimized imports

Signed-off-by: Grant Curell grant_curell@dell.com

@grantcurell grantcurell force-pushed the 266-idrac_networkpy-fails-with-attribute branch 3 times, most recently from c1a3af8 to f1ee21f Compare May 3, 2021 14:36
Copy link
Contributor

@jagadeeshnv jagadeeshnv left a comment

Choose a reason for hiding this comment

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

@grantcurell
Apologies for the delay. The content looks fine, but need these changes.
Please change the title to something like "Handling invalid share and unused imports cleanup" or whichever is more appropriate considering all the modules modified in this PR.
As a first set of comments.

  1. The README.md changes can be a separated from this PR for now.
  2. Please update the content to the latest
  3. We will review the message and get back on it
  4. Please exclude ome and redfish modules from this PR and lets keep it to idrac only

@jagadeeshnv
Copy link
Contributor

@grantcurell Please update the file tests/unit/plugins/modules/test_idrac_firmware.py, then the checks will pass.

@grantcurell
Copy link
Contributor Author

Apologies - I'll be traveling tomorrow. I'll take a look Monday and make the changes

@grantcurell
Copy link
Contributor Author

@grantcurell
Apologies for the delay. The content looks fine, but need these changes.
Please change the title to something like "Handling invalid share and unused imports cleanup" or whichever is more appropriate considering all the modules modified in this PR.
As a first set of comments.

  1. The README.md changes can be a separated from this PR for now.
  2. Please update the content to the latest
  3. We will review the message and get back on it
  4. Please exclude ome and redfish modules from this PR and lets keep it to idrac only

I just want to make sure I understand correctly, the request is to delete the changes to all the files in the commit except idrac_network.py and remove the fixes for #194 and #160 and leave both of those tickets open?

@jagadeeshnv
Copy link
Contributor

@grantcurell

  • Please retain the changes to all iDRAC modules - idrac_*
  • Delete the README and ome_* modules from this PR.(Preferably move to a new PR)
  • Change the message as below for the iDRAC modules
module.fail_json(msg="Unable to access the share. Ensure that the share name, "
                             "share mount, and share credentials provided are correct.")

dellemc_configure_idrac_eventing.py
dellemc_configure_idrac_services.py
dellemc_idrac_lc_attributes.py
dellemc_system_lockdown_mode.py
idrac_bios.py
idrac_firmware.py
idrac_network.py
idrac_syslog.pyidrac_timezone_ntp.py:

Added error handling such that if a share is used and not valid the module gracefully fails.

- Fixes dell#266
- Fixes dell#160

Signed-off-by: Grant Curell <grant_curell@dell.com>
@grantcurell grantcurell force-pushed the 266-idrac_networkpy-fails-with-attribute branch from f1ee21f to b0a5b65 Compare June 16, 2021 13:53
@jagadeeshnv
Copy link
Contributor

@grantcurell Can you enable "Allow edits from maintainers.” so that we can update tests/unit/plugins/modules/test_idrac_firmware.py for the checks to pass and the merge this PR

@jagadeeshnv jagadeeshnv self-requested a review June 24, 2021 06:33
jagadeeshnv
jagadeeshnv previously approved these changes Jun 24, 2021
To resolve UT error and indentation error, time being we are skipping this file from the PR. Rest of the content we are good to accept.
jagadeeshnv
jagadeeshnv previously approved these changes Jun 24, 2021
@jagadeeshnv
Copy link
Contributor

@grantcurell
plugins/modules/idrac_firmware.py had issues with indentation and duplicate if conditions and UT checks for this was failing as UT file was not committed. We tried to exclude idrac_firmware.py from the commit, Since our release activity is in progress today(6/24)

Can you please fix the issues or restore the idrac_firmware module without any changes so that checks will pass.
For your quick reference, PR #288 has the required changes.

@grantcurell
Copy link
Contributor Author

Apologies for not being more responsive. I took on a new project and am working pretty well at max capacity. I'll be out of town tomorrow but I'll take a look when I'm at the airport Monday

@rajeevarakkal
Copy link
Contributor

Apologies for not being more responsive. I took on a new project and am working pretty well at max capacity. I'll be out of town tomorrow but I'll take a look when I'm at the airport Monday

No problem, since our release is due, we will merge now and we will push the necessary UT correction immediately

@rajeevarakkal rajeevarakkal merged commit b86a04d into dell:collections Jun 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants