-
Notifications
You must be signed in to change notification settings - Fork 897
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
Added new firmware collection api #14476
Conversation
app/models/ems_refresh.rb
Outdated
@@ -14,6 +14,7 @@ module EmsRefresh | |||
extend EmsRefresh::SaveInventoryConfiguration | |||
extend EmsRefresh::SaveInventoryAutomation | |||
extend EmsRefresh::SaveInventoryOrchestrationStacks | |||
extend EmsRefresh::SaveInventoryPhysicalInfra |
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.
This line is already in master https://github.com/ManageIQ/manageiq/blob/master/app/models/ems_refresh.rb#L6
@@ -20,6 +20,7 @@ def save_ems_inventory(ems, hashes, target = nil) | |||
when ManageIQ::Providers::StorageManager then save_ems_storage_inventory(ems, hashes, target) | |||
when ManageIQ::Providers::MiddlewareManager then save_ems_middleware_inventory(ems, hashes, target) | |||
when ManageIQ::Providers::DatawarehouseManager then save_ems_datawarehouse_inventory(ems, hashes, target) | |||
when ManageIQ::Providers::PhysicalInfraManager then save_ems_physical_infra_inventory(ems, hashes, target) |
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.
This line is already in master https://github.com/ManageIQ/manageiq/blob/master/app/models/ems_refresh/save_inventory.rb#L14
We have alias for EmsCloud
, EmsInfra
, and EmsPhysicalInfra
which is why it's not written out fully and you may not have noticed it.
This pull request is not mergeable. Please rebase and repush. |
d0aa13a
to
2aa8384
Compare
2aa8384
to
73ca1d9
Compare
This pull request is not mergeable. Please rebase and repush. |
73ca1d9
to
2531025
Compare
@miq-bot remove_label wip |
config/api.yml
Outdated
:description: Firmwares | ||
:options: | ||
- :collection | ||
- :subcollection |
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.
which primary collection is declaring this as a subcollection ?
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.
This should be the PhysicalServer collection. The PhysicalServer collection is defined in PR #14028.
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.
@rodneyhbrown7 it looks like in the linked PR it's not declaring this as a subcollection. It may be best to leave this out in this PR, or until you are ready to add it to the phyical server config
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.
@abellotti @imtayadeway @rodneyhbrown7 I removed the firmwares subcollection option for now. If this PR gets merged before the linked PR (#14028), the linked PR will be updated to add this as a subcollection.
|
||
it 'query Firmwares' do | ||
FactoryGirl.create(:firmware) | ||
test_collection_query(:firmwares, "/api/firmwares", Firmware) |
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.
leverage firmwares_url() for all tests.
spec/requests/api/firmwares_spec.rb
Outdated
expect_single_resource_query("resource_id" => 1) | ||
end | ||
end | ||
end |
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.
add test for invalid role and such.
spec/requests/api/firmwares_spec.rb
Outdated
expect_single_resource_query("id" => 1) | ||
expect_single_resource_query("name" => "UEFI") | ||
expect_single_resource_query("version" => "D7E152CUS-2.11") | ||
expect_single_resource_query("resource_id" => 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.
these can be a single call, expect_single_resource_query("id" => 1, "name" => ...)
/cc @imtayadeway |
Ping @imtayadeway can I borrow your 👀 for a quick review here ? Thanks !! 🎵 |
b65d306
to
e7b1458
Compare
spec/requests/api/firmwares_spec.rb
Outdated
describe "display firmware details" do | ||
context "with a valid role" do | ||
it "shows its properties" do | ||
fw = FactoryGirl.create(:firmware, :id => 1, :name => "UEFI", |
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.
You probably don't want to set the id
here - that's the database's job and could result in some erratic behavior
spec/requests/api/firmwares_spec.rb
Outdated
|
||
context "with an invalid role" do | ||
it "fails to show its properties" do | ||
fw = FactoryGirl.create(:firmware, :id => 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.
As above
@abellotti @rodneyhbrown7 this LGTM with just a couple of nits above 👍 |
Checked commits rodneyhbrown7/manageiq@36bf074~...6266155 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
Thanks @rodneyhbrown7 for enhancing the API. |
Adds a REST API for firmware elements
updates the product features yaml file
Adds a firmwares_controller