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.

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

Fix deleting a disk from VM #348

Merged
merged 1 commit into from
Mar 11, 2019
Merged

Conversation

borod108
Copy link
Contributor

Removing a disk was not handled correctly

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1666593

@borod108 borod108 requested a review from masayag March 10, 2019 13:07
@borod108 borod108 changed the title Fix deleting a disk from VM [wip]Fix deleting a disk from VM Mar 10, 2019
@miq-bot miq-bot added the wip label Mar 10, 2019
@borod108 borod108 force-pushed the bugs/fix1666593 branch 2 times, most recently from 06681ee to 1fcf320 Compare March 10, 2019 14:47
@borod108 borod108 changed the title [wip]Fix deleting a disk from VM Fix deleting a disk from VM Mar 10, 2019
@borod108 borod108 requested a review from agrare March 10, 2019 14:48
@miq-bot miq-bot removed the wip label Mar 10, 2019
@borod108
Copy link
Contributor Author

@agrare @masayag what do you think?

@@ -272,7 +272,6 @@ def vm_reconfigure(vm, options = {})
# Retrieve the current representation of the virtual machine:
# TODO: no need to retreive vm here, only if memory is updated
vm = vm_service.get

Copy link
Contributor

Choose a reason for hiding this comment

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

seems this space line is required

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added it back

@miq-bot
Copy link
Member

miq-bot commented Mar 11, 2019

Checked commit borod108@70f427f with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 6 offenses detected

spec/models/manageiq/providers/redhat/infra_manager/ovirt_services/strategies/v4_spec.rb

disk_specs.each do |disk_spec|
attachment_service = attachments_service.attachment_service(disk_spec['disk_name'])
disk_spec = disk_spec.with_indifferent_access
Copy link
Member

Choose a reason for hiding this comment

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

@borod108 just curious, why do you need with_indifferent_access here? Is the 'disk_name' key not always a string ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When the request comes from api it is, at least sometimes, a symbol (this was one of the things that went wrong here)

Copy link
Member

Choose a reason for hiding this comment

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

Interesting ok thanks

@agrare agrare self-assigned this Mar 11, 2019
Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

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

LGTM

@agrare agrare merged commit 70f427f into ManageIQ:master Mar 11, 2019
agrare added a commit that referenced this pull request Mar 11, 2019
@agrare agrare added this to the Sprint 107 Ending Mar 18, 2019 milestone Mar 11, 2019
simaishi pushed a commit that referenced this pull request Mar 12, 2019
@simaishi
Copy link
Contributor

Hammer backport details:

$ git log -1
commit 292239cc99e49cebe0b84e68ef85c36fa24b3e70
Author: Adam Grare <agrare@redhat.com>
Date:   Mon Mar 11 10:03:03 2019 -0400

    Merge pull request #348 from borod108/bugs/fix1666593
    
    Fix deleting a disk from VM
    
    (cherry picked from commit c4063dd78b9c0fac8131ae51d68d92fb994de0be)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1687846

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants