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

Updated requested method to fix vm_reconfigure via rest-api. #413

Merged

Conversation

billfitzgerald0120
Copy link
Contributor

@billfitzgerald0120 billfitzgerald0120 commented Aug 27, 2018

Issuing a vm_reconfigure disk_remove via rest-api was failing.
Very minor change (:disk_name was changed to 'disk_name')

disk_num = disk['disk_name'].match(/_(\d).vmdk/)"

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

The rest-api call for vm_reconfigure uses the options[:disk_remove] hash which contains a string key of 'disk_name'. The quota requested method failed since it was expecting disk_name as a symbol.

The UI also uses the options[:disk_remove] hash with a string key of 'disk_name', which doesn't fail the same way in quota because the hash type is HashWithIndifferentAccess. So, the method wasn't able to find the correct value (nil), but didn't fail. Although this ticket is specifically about quota, the actual vm_reconfigure works when initiated by the UI, but does not work properly when initiated by the rest-api.
Will open a new ticket to track the vm_configure rest-api issue.

@billfitzgerald0120
Copy link
Contributor Author

@miq-bot add_label bug

@miq-bot miq-bot added the bug label Aug 27, 2018
@@ -158,7 +158,7 @@ def requested_storage(args_hash)
end
if args_hash[:resource].options[:disk_remove]
args_hash[:resource].options[:disk_remove].each do |disk|
disk_num = disk[:disk_name].match(/_(\d).vmdk/)
disk_num = disk['disk_name'].match(/_(\d).vmdk/)
Copy link
Contributor

Choose a reason for hiding this comment

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

@billfitzgerald0120 Is this a symbol versus string issue, has it worked before with a symbol?
Do we have spec for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mkanoor @tinaafitz I re-created on 5.8.4.5 using the rest-api. The UI worked with the original code but not when using the rest-api. I tested this on master with the same results. There is a requested_spec but it doesn't test the disk remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mkanoor @tinaafitz I added a test in requested_spec for disk_remove, please review

Copy link
Contributor

Choose a reason for hiding this comment

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

@billfitzgerald0120 Is the spec file missing from this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mkanoor I just added the spec file.

…st-api.

Issuing a vm_reconfigure disk_remove via rest-api was failing.
Very minor change (:disk_name was changed to 'disk_name')

disk_num = disk['disk_name'].match(/_(\d).vmdk/)"

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

Added a test for disk_remove
@miq-bot
Copy link
Member

miq-bot commented Sep 4, 2018

Checked commit billfitzgerald0120@011643f with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🏆

@billfitzgerald0120
Copy link
Contributor Author

@mkanoor @gmcculloug @tinaafitz The requested method was asking for a symbol. The rest-api uses a hash which caused the requested method to fail and create an Error.
The UI uses a HashWithIndifferentAccess and the method wasn't able to find the correct value (nil) but didn't fail and the disk was removed.

I change the requested method to use a string and the requested method works for both the UI and the rest-api, but the rest-api fails while trying to delete the disk. We will need to fix this in a different PR.

@billfitzgerald0120
Copy link
Contributor Author

We tried to use get_option but that returned an array and we need a hash.

Copy link
Member

@tinaafitz tinaafitz left a comment

Choose a reason for hiding this comment

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

@billfitzgerald0120 looks good.

@mkanoor mkanoor merged commit 758b168 into ManageIQ:master Sep 4, 2018
@mkanoor mkanoor added this to the Sprint 94 Ending Sept 10, 2018 milestone Sep 4, 2018
@JPrause
Copy link
Member

JPrause commented Sep 14, 2018

@miq-bot add_label blocker

@JPrause
Copy link
Member

JPrause commented Sep 14, 2018

@billfitzgerald0120 if this can be backported, can you add the gaprindashvili/yes label.

@billfitzgerald0120
Copy link
Contributor Author

@miq-bot add_label gaprindashvili/yes

simaishi pushed a commit that referenced this pull request Sep 18, 2018
Updated requested method to fix vm_reconfigure via rest-api.

(cherry picked from commit 758b168)

https://bugzilla.redhat.com/show_bug.cgi?id=1629096
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit aeea686734529869a3ed7b94a8da06da05933d85
Author: Madhu Kanoor <[email protected]>
Date:   Tue Sep 4 17:29:19 2018 -0400

    Merge pull request #413 from billfitzgerald0120/requested_rest_api_tweek
    
    Updated requested method to fix vm_reconfigure via rest-api.
    
    (cherry picked from commit 758b168e4520da52a310bb50666330fae697d2c6)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1629096

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.

6 participants