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.

Sign up for GitHub

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

Adding Physical Switch power action #392

Merged
merged 1 commit into from
Jun 20, 2018
Merged

Adding Physical Switch power action #392

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions app/controllers/api/physical_switches_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,59 @@ def refresh_resource(type, id, _data = nil)
end
end

def restart_resource(type, id, _data = nil)
perform_action(:restart, type, id)
end

private

def perform_action(action, type, id)
if single_resource?
enqueue_action_single_resource(action, type, id)
else
enqueue_action_multiple_resources(action, type, id)
end
end

#
# Enqueues the action for a single resource.
#
# @param [symbol] action - action to be enqueued
# @param [symbol] type - type of the resource
# @param [number] id - id of the resource
#
def enqueue_action_single_resource(action, type, id)
raise BadRequestError, "Must specify an id for changing a #{type} resource" unless id

physical_switch = resource_search(id, type, collection_class(type))

api_action(type, id) do
begin
desc = "Performing #{action} for #{physical_switch_ident(physical_switch)}"
api_log_info(desc)
task_id = queue_object_action(physical_switch, desc, :method_name => action, :role => :ems_operations)
action_result(true, desc, :task_id => task_id)
rescue StandardError => err
action_result(false, err.to_s)
end
end
end

#
# Enqueues the action for multiple resources.
# For multiple resources, when an error occurs, the error messages must
# be built individually for each resource. Always responding with status 200.
Copy link
Member

Choose a reason for hiding this comment

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

@douglasgabrieIt's not obvious what the difference is between enqueue_action_single_resource and enqueue_action_multiple_resources. Can you provide some more details?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure! Basically a single resource request failure will look like:

[status = 404]
{
    "error": {
        "kind": "not_found",
        "message": "Couldn't find PhysicalSwitch with 'id'=4 [WHERE \"switches\".\"type\" IN ('PhysicalSwitch', 'ManageIQ::Providers::Lenovo::PhysicalInfraManager::PhysicalSwitch')]",
        "klass": "ActiveRecord::RecordNotFound"
    }
}

However, a multiple resource request failure will look like:

[status = 200]
{
    "results": [
        {
            "success": true,
            "message": "Performing restart for Physical Switch id:1 name: 'Lenovo Switch'",
            "task_id": "2",
            "task_href": "http://localhost:3000/api/tasks/2",
            "href": "http://localhost:3000/api/physical_switches/1"
        },
        {
            "success": false,
            "message": "Couldn't find PhysicalSwitch with 'id'=9999 [WHERE \"switches\".\"type\" IN ('PhysicalSwitch', 'ManageIQ::Providers::Lenovo::PhysicalInfraManager::PhysicalSwitch')]"
        }
    ]
}

The main difference here is that for a single resource request, the entire response fails, see the status.

But, for multiple resources, the failures are self contained, once some resources could execute the action successfully and others could fail, see the status again.

This is why the enqueue_action_multiple_resources method has a rescue for these errors and inside it a graceful message is mounted for the user, which doesn't occurs in enqueue_action_single_resource.

It made the things more clear for you @gtanzillo ? Maybe add those examples in the method documentation would be a good option, what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that helps a lot. One more question -

How is id passed to enqueue_action_multiple_resources? Is it one id at a time or an array of ids?

Copy link
Member Author

Choose a reason for hiding this comment

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

One at time

#
# @param [symbol] action - action to be enqueued
# @param [symbol] type - type of the resource
# @param [number] id - id of the resource
#
def enqueue_action_multiple_resources(action, type, id)
enqueue_action_single_resource(action, type, id)
rescue ActiveRecord::RecordNotFound => err
action_result(false, _(err.message))
end

def ensure_resource_exists(type, id)
raise NotFoundError, "#{type} with id:#{id} not found" unless collection_class(type).exists?(id)
end
Expand Down
4 changes: 4 additions & 0 deletions config/api.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1847,13 +1847,17 @@
:identifier: physical_switch_show_list
- :name: refresh
:identifier: physical_switch_refresh
- :name: restart
:identifier: physical_switch_restart
:resource_actions:
:get:
- :name: read
:identifier: physical_switch_show
:post:
- :name: refresh
:identifier: physical_switch_refresh
- :name: restart
:identifier: physical_switch_restart
:pictures:
:description: Pictures
:options:
Expand Down
83 changes: 79 additions & 4 deletions spec/requests/physical_switches_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
describe "Physical Switches API" do
let(:physical_switch) { FactoryGirl.create(:physical_switch) }

context "GET /api/physical_switches" do
it "returns all Physical Switches" do
physical_switch = FactoryGirl.create(:physical_switch)
physical_switch

api_basic_authorize('physical_switch_show_list')

get(api_physical_switches_url)
Expand All @@ -17,7 +20,6 @@

context "GET /api/physical_switches/:id" do
it "returns a single Physical Switch" do
physical_switch = FactoryGirl.create(:physical_switch)
api_basic_authorize('physical_switch_show')

get(api_physical_switch_url(nil, physical_switch))
Expand All @@ -44,7 +46,6 @@

context "without an appropriate role" do
it "it responds with 403 Forbidden" do
physical_switch = FactoryGirl.create(:physical_switch)
api_basic_authorize

post(api_physical_switch_url(nil, physical_switch), :params => gen_request(:refresh))
Expand All @@ -63,7 +64,6 @@
end

it "refresh of a single Physical Switch" do
physical_switch = FactoryGirl.create(:physical_switch)
api_basic_authorize('physical_switch_refresh')

post(api_physical_switch_url(nil, physical_switch), :params => gen_request(:refresh))
Expand Down Expand Up @@ -97,4 +97,79 @@
end
end
end

describe "Physical Switches restart action" do
let(:action) { :restart }

context "For multiple resources" do
context "With an invalid id and valid id" do
it "returns a HTTP status 200, perform the action for the valid resource and fail for the invalid" do
api_basic_authorize(action_identifier(:physical_switches, action, :resource_actions, :post))

post(api_physical_switches_url, :params => gen_request(action, [{"href" => api_physical_switch_url(nil, physical_switch)}, {"href" => api_physical_switch_url(nil, 999_999)}]))

expected = {
"results" => a_collection_containing_exactly(
a_hash_including(
"message" => a_string_matching("Performing #{action} for Physical Switch id:#{physical_switch.id} name: '#{physical_switch.name}'"),
"success" => true,
"href" => api_physical_switch_url(nil, physical_switch)
),
a_hash_including(
"success" => false
)
)
}
expect(response.parsed_body).to include(expected)
expect(response).to have_http_status(:ok)
end
end

context "With a valid request" do
it "performs the action succesfully" do
api_basic_authorize(action_identifier(:physical_switches, action, :resource_actions, :post))

post(api_physical_switches_url, :params => gen_request(action, [{"href" => api_physical_switch_url(nil, physical_switch)}]))

expected = {
"results" => a_collection_containing_exactly(
a_hash_including(
"message" => a_string_matching("Performing #{action} for Physical Switch id:#{physical_switch.id} name: '#{physical_switch.name}'"),
"success" => true,
"href" => api_physical_switch_url(nil, physical_switch)
)
)
}
expect(response.parsed_body).to include(expected)
expect(response).to have_http_status(:ok)
end
end
end

context "For a single resource" do
context "With an invalid id and valid id" do
it "it responds with 404 Not Found" do
api_basic_authorize(action_identifier(:physical_switches, action, :resource_actions, :post))

post(api_physical_switch_url(nil, 999_999), :params => gen_request(action))

expect(response).to have_http_status(:not_found)
end
end

context "With a valid request" do
it "restarts the Physical Switch" do
api_basic_authorize(action_identifier(:physical_switches, action, :resource_actions, :post))

post(api_physical_switch_url(nil, physical_switch), :params => gen_request(action))

expect_single_action_result(
:success => true,
:message => a_string_matching("Performing #{action} for Physical Switch id:#{physical_switch.id} name: '#{physical_switch.name}'"),
:href => api_physical_switch_url(nil, physical_switch)
)
end
end
end
end
end