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 specs for reconfiguring vm with OvirtSDK #22

Merged
merged 1 commit into from
May 23, 2017
Merged

Add specs for reconfiguring vm with OvirtSDK #22

merged 1 commit into from
May 23, 2017

Conversation

borod108
Copy link
Contributor

Add some specs to the reconfiguring vm process

@borod108 borod108 force-pushed the specs/reconfigure_vm branch from 7564e61 to fa8f904 Compare April 27, 2017 13:53
@borod108
Copy link
Contributor Author

@masayag please take a look

@borod108 borod108 force-pushed the specs/reconfigure_vm branch 2 times, most recently from bca9fb6 to 2e1a1c0 Compare April 27, 2017 14:18
agrare pushed a commit to agrare/manageiq-providers-ovirt that referenced this pull request May 16, 2017
@Fryguy
Merge pull request ManageIQ#22 from chessbyte/hakiri-badge
bc25be4 
Add Hakiri Badge
@miq-bot
Copy link
Member

miq-bot commented May 18, 2017

This pull request is not mergeable. Please rebase and repush.

@borod108 borod108 force-pushed the specs/reconfigure_vm branch from 2e1a1c0 to 9306d65 Compare May 21, 2017 08:40
@borod108
Copy link
Contributor Author

@masayag can you please review?

masayag
masayag reviewed May 21, 2017
View reviewed changes
spec/models/manageiq/providers/redhat/infra_manager_spec.rb
context "use_ovirt_engine_sdk is set to false" do
let(:use_ovirt_engine_sdk) { false }
it 'sends vm_reconfigure to the right ovirt_services' do
expect(@v4_strategy_instance).to receive(:vm_reconfigure).with(@vm, {})
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems incorrect: if sdk shouldn't be used, the v4 strategy instance shouldn't be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should. The vm_reconfigure should always run through v4 if it is available even when the use_ovirt_sdk is toggled off, this is by design.

masayag
masayag reviewed May 21, 2017
View reviewed changes
spec/models/manageiq/providers/redhat/infra_manager/ovirt_services/strategies/v4_spec.rb Outdated
end
end

context "memory is less than vms memory should be rounded up to" do
Copy link
Contributor

Choose a reason for hiding this comment

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

should there be any text after "up do" ? this seems like the sentence is ended prematurely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@masayag
Copy link
Contributor

masayag commented May 21, 2017

Tests look good, added few comments inline.

For v4 we support also reconfigure of vm's disks. Those aren't covered by the tests.
This can be added later on or we can settle with the exiting disks tests coverage we already have.

@borod108 borod108 force-pushed the specs/reconfigure_vm branch from 9306d65 to a86f931 Compare May 22, 2017 10:51
@miq-bot
Copy link
Member

miq-bot commented May 22, 2017

Checked commit borod108@a86f931 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks fine. 🍪

@borod108
Copy link
Contributor Author

@masayag if I fixed your comments properly and you do not have any more, can you approve this and assign oourfali?

masayag
masayag approved these changes May 23, 2017
View reviewed changes
Copy link
Contributor

@masayag masayag left a comment

Choose a reason for hiding this comment

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

@miq-bot assign @oourfali

@oourfali oourfali merged commit ed7a2da into ManageIQ:master May 23, 2017
@simaishi simaishi mentioned this pull request Apr 9, 2018
Merged
@simaishi
Copy link
Contributor

simaishi commented Apr 10, 2018 •
edited
Loading

Fine backport (to manageiq repo) details:

$ git log -1
commit e3114c4cfe39787f1192650df56b4251a879d52a
Author: Oved Ourfali <[email protected]>
Date:   Tue May 23 13:41:45 2017 +0300

    Merge pull request #22 from borod108/specs/reconfigure_vm
    
    Add specs for reconfiguring vm with OvirtSDK
    (cherry picked from commit ed7a2da288aeeabed8a73e39e130e131eb724d73)

@borod108 borod108 deleted the specs/reconfigure_vm branch December 4, 2018 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@masayag masayag masayag approved these changes

Assignees
No one assigned
Labels
fine/backported test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@borod108 @miq-bot @masayag @simaishi @oourfali