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

Jump to bottom

Add support for Physical Servers refresh #189

Merged
merged 1 commit into from
Nov 21, 2017
Merged

Add support for Physical Servers refresh #189

merged 1 commit into from
Nov 21, 2017

Conversation

MaysaMacedo
Copy link
Member

@MaysaMacedo MaysaMacedo commented Nov 8, 2017 •
edited
Loading

This PR enables:

  • Refresh of one or more Physical Servers.

Related to #2547

@MaysaMacedo MaysaMacedo changed the title Physical Servers refresh Add support for Physical Servers refresh Nov 8, 2017
@MaysaMacedo MaysaMacedo mentioned this pull request Nov 8, 2017
Closed
@MaysaMacedo
Copy link
Member Author

MaysaMacedo commented Nov 8, 2017 •
edited
Loading

@agrare @imtayadeway Could you take a look on this PR?

@miq-bot
Copy link
Member

miq-bot commented Nov 8, 2017

@MaysaMacedo unrecognized command 'add_lab', ignoring...

Accepted commands are: add_label, assign, close_issue, move_issue, remove_label, rm_label, set_milestone

@MaysaMacedo
Copy link
Member Author

@miq-bot add_label gaprindashvili/yes,enhancement

abellotti
abellotti reviewed Nov 13, 2017
View reviewed changes
Copy link
Member

@abellotti abellotti left a comment

Choose a reason for hiding this comment

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

will also need specs, both single resource and bulk

app/controllers/api/physical_servers_controller.rb Outdated
@@ -40,6 +40,24 @@ def restart_mgmt_controller_resource(type, id, _data)
change_resource_state(:restart_mgmt_controller, type, id)
end

def refresh_physical_server(physical_server)
Copy link
Member

Choose a reason for hiding this comment

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

maybe move this to private section

Copy link
Member Author

Choose a reason for hiding this comment

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

@abellotti @jntullo Thanks for your feedback, just fixed it.

@abellotti abellotti self-assigned this Nov 13, 2017
@abellotti abellotti requested a review from jntullo November 13, 2017 17:08
@MaysaMacedo MaysaMacedo force-pushed the physical_server_refresh branch from 1c4caaf to 2076fac Compare November 15, 2017 17:29
@MaysaMacedo
Copy link
Member Author

@abellotti @jntullo Thanks for your feedback, I've updated it.

@AndreyMenezes
Copy link
Member

@abellotti @jntullo Could anyone review this PR?

imtayadeway
imtayadeway suggested changes Nov 20, 2017
View reviewed changes
Copy link
Contributor

@imtayadeway imtayadeway left a comment

Choose a reason for hiding this comment

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

@MaysaMacedo this looks great! Just a few requests above. Thanks!

app/controllers/api/physical_servers_controller.rb Outdated
@@ -40,6 +40,16 @@ def restart_mgmt_controller_resource(type, id, _data)
change_resource_state(:restart_mgmt_controller, type, id)
end

def refresh_resource(type, id = nil, _data = nil)
raise BadRequestError, "Must specify an id for refreshing a #{type} resource" unless id
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a test for this condition?

app/controllers/api/physical_servers_controller.rb Outdated
@@ -40,6 +40,16 @@ def restart_mgmt_controller_resource(type, id, _data)
change_resource_state(:restart_mgmt_controller, type, id)
end

def refresh_resource(type, id = nil, _data = nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for the default id = nil here? Does anything call this without the second argument?

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right. No need for this here.

spec/requests/physical_servers_spec.rb Outdated

describe "Physical Server refresh action" do
context "with an invalid id" do
it "fails to refresh a physical server" do
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit, but it "fails..." suggests that something is wrong....possibly "it responds with 404 Not Found" or similar?

spec/requests/physical_servers_spec.rb Outdated
end

context "without an appropriate role" do
it "fails to refresh a physical server" do
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly....avoid variations of it "fails...."

jntullo
jntullo reviewed Nov 20, 2017
View reviewed changes
Copy link

@jntullo jntullo left a comment

Choose a reason for hiding this comment

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

Minor nit, but looks good! ✨

spec/requests/physical_servers_spec.rb Outdated
context "without an appropriate role" do
it "fails to refresh a physical server" do
ps = FactoryGirl.create(:physical_server)

Copy link

Choose a reason for hiding this comment

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

nit - can you remove this empty line? setting up the authorization and creating the factory are both part of the setup, they can be together. Same with similar instances below.

@MaysaMacedo MaysaMacedo force-pushed the physical_server_refresh branch 3 times, most recently from 008baed to 49deac3 Compare November 20, 2017 20:59
@MaysaMacedo MaysaMacedo force-pushed the physical_server_refresh branch from 49deac3 to b285cf8 Compare November 21, 2017 12:33
@miq-bot
Copy link
Member

miq-bot commented Nov 21, 2017

Checked commit MaysaMacedo@b285cf8 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 1 offense detected

**

  • 💣 💥 🔥 🚒 - Linter/Yaml - missing config files

@MaysaMacedo
Copy link
Member Author

@imtayadeway @jntullo Thanks! Just updated.

@abellotti
Copy link
Member

LGTM!! Thanks @MaysaMacedo for the API Enhancement!! 👍

@abellotti abellotti added this to the Sprint 74 Ending Nov 27, 2017 milestone Nov 21, 2017
@abellotti abellotti merged commit 2311c73 into ManageIQ:master Nov 21, 2017
simaishi pushed a commit that referenced this pull request Nov 27, 2017
@abellotti @simaishi
Merge pull request #189 from MaysaMacedo/physical_server_refresh
8f63787 
Add support for Physical Servers refresh
(cherry picked from commit 2311c73)
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit 8f63787c368bf3eec14575bc2b24a1be3c66426f
Author: Alberto Bellotti <abellotti@users.noreply.github.com>
Date:   Tue Nov 21 10:55:52 2017 -0500

    Merge pull request #189 from MaysaMacedo/physical_server_refresh
    
    Add support for Physical Servers refresh
    (cherry picked from commit 2311c73ec9b8569d1e4c6c5588201e31f775b6dd)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@imtayadeway imtayadeway imtayadeway approved these changes

@jntullo jntullo jntullo approved these changes

@abellotti abellotti abellotti approved these changes

Assignees

@abellotti abellotti

Labels
enhancement gaprindashvili/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@MaysaMacedo @miq-bot @AndreyMenezes @abellotti @simaishi @imtayadeway @jntullo