-
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
Validate CPU and Memory Hot-Plug settings in reconfigure #12275
Validate CPU and Memory Hot-Plug settings in reconfigure #12275
Conversation
3dee296
to
a5615f9
Compare
a5615f9
to
dad3b89
Compare
dad3b89
to
d81a8f0
Compare
@gmcculloug is this a good place to do this type of validation? |
d81a8f0
to
cd7b2a3
Compare
Yes, I think this makes sense. @lfu Please review. |
cd7b2a3
to
a908c66
Compare
Thanks @gmcculloug , added some spec tests and taking out of WIP |
Linking another BZ to this PR https://bugzilla.redhat.com/show_bug.cgi?id=1396631 |
@@ -5,7 +5,7 @@ | |||
:name => 'test_vm', | |||
:raw_power_state => 'poweredOff', | |||
:storage => FactoryGirl.create(:storage, :name => 'storage'), | |||
:hardware => FactoryGirl.create(:hardware, :cpu2x2, :ram1GB, :virtual_hw_version => "07") | |||
:hardware => FactoryGirl.create(:hardware, :cpu4x2, :ram1GB, :virtual_hw_version => "07") |
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.
@agrare What is this change for?
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.
That was so I didn't have to override the default options when I wanted to test just memory or just cpu, I just set it to match https://github.com/agrare/manageiq/blob/a908c662e367e695a58e365550bce7f769b0058f/spec/models/manageiq/providers/vmware/infra_manager/vm/reconfigure_spec.rb#L65
If you look at the rest of the changes in that commit I was able to remove a bunch of code that set the options to the current value, for example a908c66#diff-fafec56c2822f5c78227a8a11313bc6fL124
LGTM other than the minor question |
This depends on #12145 which had a sql migration so setting this to |
a908c66
to
0110761
Compare
Checked commits agrare/manageiq@4783ce6~...0110761 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1 app/models/manageiq/providers/vmware/infra_manager/vm/reconfigure.rb
spec/models/manageiq/providers/vmware/infra_manager/vm/reconfigure_spec.rb
|
@gmcculloug can this be merged? |
It can! Thanks. |
This validates a number of cases related to reconfigure on powered-on VMs that would lead to sometimes cryptic VMware error messages.
The cases covered are:
https://bugzilla.redhat.com/show_bug.cgi?id=1386843