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

Don't reset IPMI setting when update inventory #1162

Merged
merged 2 commits into from
Jan 12, 2024

Conversation

masa-orca
Copy link
Collaborator

@masa-orca masa-orca commented Jan 11, 2024

SUMMARY
ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

zabbix_host module

Copy link

codecov bot commented Jan 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f2287d6) 79.67% compared to head (b3b0812) 79.54%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1162      +/-   ##
==========================================
- Coverage   79.67%   79.54%   -0.14%     
==========================================
  Files          34       34              
  Lines        4311     4264      -47     
  Branches     1151     1127      -24     
==========================================
- Hits         3435     3392      -43     
+ Misses        519      517       -2     
+ Partials      357      355       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -0,0 +1,2 @@
bugfixes:
- zabbix_host - Don't reset IPMI setting when update inventory
Copy link
Collaborator

@BGmot BGmot Jan 11, 2024

Choose a reason for hiding this comment

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

- - zabbix_host - Don't reset IPMI setting when update inventory
+ - zabbix_host - Don't reset IPMI setting when update inventory data of a host

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks. I have changed to the sentence which you wrote.

@BGmot
Copy link
Collaborator

BGmot commented Jan 11, 2024

I don't understand why you treat let's say ipmi_privilege and ipmi_username differently? why some of them checked for being in zabbix_host_obj and some not?

@masa-orca
Copy link
Collaborator Author

Thank you for reviewing.

ipmi_privilege and ipmi_username differently?

It is no meaning. I add checks about the parameters.

why some of them checked for being in zabbix_host_obj and some not?

  • We need to set the minimum parameters.
  • reseting IPMI setting was coused by unnecessary parameters.
  • In current source code, we cannot update value of string parameters (e.g. ipmi_username) with empty string.
    • To evaluate empty string result is false, so, the module skips adding parameter.
test_string = ""
if test_string:
    print('Update a parameter')  # This print function is never called.

@BGmot
Copy link
Collaborator

BGmot commented Jan 12, 2024

Thank you very much @masa-orca -)

@BGmot BGmot merged commit 93b1fc1 into ansible-collections:main Jan 12, 2024
27 of 28 checks passed
@masa-orca masa-orca deleted the ipmi branch January 13, 2024 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

zabbix_host module: updating host inventory data causes IPMI values to be reset
2 participants