-
Notifications
You must be signed in to change notification settings - Fork 25
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 computer systems to inventory #8
Conversation
I am marking this pull request as wip since needs ManageIQ/manageiq#17557 merged first. @miq-bot add_label wip |
Since computer systems are in 1:1 relation with the physical servers and carry no data on its own, we add them to the database at the same time as the servers.
3d5721d
to
260a668
Compare
Checked commit xlab-si@260a668 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
@miq-bot remove_label wip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor comment regarding additional test, if necessary.
Otherwise LGTM
@@ -24,6 +24,7 @@ def assert_ems | |||
expect(ems.physical_servers.count).to eq(1) | |||
expect(ems.physical_servers.map(&:ems_ref)).to match_array([server_id]) | |||
expect(ems.physical_server_details.count).to eq(1) | |||
expect(ems.computer_systems.count).to eq(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be a simple assert for the computer_system
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not add it because there is not much to check here, since ComputerSystem model only contains two fields: id and type of its managed entity. I can check if those fields match the server's, but this only checks if the refresh system is working as expected and has nothing to do with Redfish provider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
Add computer systems to inventory
Since computer systems are in 1:1 relation with the physical servers
and carry no data on its own, we add them to the database at the same
time as the servers.
@miq-bot assign @gtanzillo
@miq-bot add_reviewer @gberginc
@miq-bot add_reviewer @matejart