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

Changed disk_name to symbol in requested Quota method. #474

Conversation

billfitzgerald0120
Copy link
Contributor

@billfitzgerald0120 billfitzgerald0120 commented Nov 20, 2018

Disk_name changed to symbol.

Tested on VMware and Redhat machines for 5.8, 5.9. and 5.10.

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

@billfitzgerald0120
Copy link
Contributor Author

@miq-bot add_reviewer @tinaafitz
@miq-bot add_label bug, fine/yes, gaprindashvili/yes, blocker

@coveralls
Copy link

coveralls commented Nov 20, 2018

Pull Request Test Coverage Report for Build 2375

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 97.184%

Totals Coverage Status
Change from base Build 2357: 0.0%
Covered Lines: 2933
Relevant Lines: 3018

💛 - Coveralls

@billfitzgerald0120
Copy link
Contributor Author

@tinaafitz Please review

@tinaafitz
Copy link
Member

@mkanoor Please review.

Disk_name changed to symbol.

Tested on VMware and Redhat machines for 5.8, 5.9. and 5.10
Added         disk = HashWithIndifferentAccess.new(disk)
Updated spec
@miq-bot
Copy link
Member

miq-bot commented Nov 22, 2018

Checked commit billfitzgerald0120@c3d4b18 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. 🍰

@JPrause
Copy link
Member

JPrause commented Nov 26, 2018

@mkanoor was there anything else prior to merge. Also, who can get this merged in for a blocker.
@billfitzgerald0120 can you add the hammer/yes label.

@billfitzgerald0120
Copy link
Contributor Author

@miq-bot add_label hammer/yes

@gmcculloug
Copy link
Member

@billfitzgerald0120 Where did disk_name change to a symbol? Do you have a link or commit you can reference?

@mkanoor
Copy link
Contributor

mkanoor commented Nov 27, 2018

@gmcculloug
This was fixed in ManageIQ/manageiq-api@55fd438 by the REST API

Previously options would come in the way they were defined in the native language
Python, Java, Postman would send strings
Ruby could send strings or symbols

With the above PR everything coming into the API was converted to symbols.

@tinaafitz tinaafitz merged commit da43013 into ManageIQ:master Nov 27, 2018
@simaishi
Copy link
Contributor

Backporting this to Gaprindashvili conflicts unless #461 is also taken into Gaprindashvili. However #461 isn't approved to be included in G release right now.

Can/should this PR be done differently for G branch, or should we take #461 into G branch?

cc @dmetzger57

@gmcculloug
Copy link
Member

I would be in favor of including #461 in Gaprindashvili to incorporate the bug fix and refactoring changes.

@tinaafitz @mkanoor Do you see any reason not to back-port it?

simaishi pushed a commit that referenced this pull request Nov 28, 2018
…k_remove_fix

Changed disk_name to symbol in requested Quota method.

(cherry picked from commit da43013)

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

Hammer backport details:

$ git log -1
commit f42263cbd3a992fd103b6eb422f20400dc5f1014
Author: tina <tina.fitzgerald2@gmail.com>
Date:   Tue Nov 27 16:29:28 2018 -0500

    Merge pull request #474 from billfitzgerald0120/quota_vm_reconfig_disk_remove_fix
    
    Changed disk_name to symbol in requested Quota method.
    
    (cherry picked from commit da43013051edc76032eb483238ade483ea54b787)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1620161

@tinaafitz
Copy link
Member

@gmcculloug I am in favor of including #461 in Gaprindashvili to incorporate the bug fix and refactoring changes.

@simaishi
Copy link
Contributor

Thanks @gmcculloug @tinaafitz

@dmetzger57 - will wait for your ack to take #461 as https://bugzilla.redhat.com/show_bug.cgi?id=1644351 isn't approved for the next G release.

@dmetzger57
Copy link

The associated BZs were not scheduled for the G errata as there are no customer cases associated and the errata releases are heavily weighted to customer reported issues. So @simaishi at this time we will not be back porting to the G release.

@simaishi
Copy link
Contributor

In that case... marking as gaprindashvili/conflict as I'm not able to resolve the conflict.

@gmcculloug
Copy link
Member

@billfitzgerald0120 You will need to create a PR on the Gaprindashvili branch for this change. Let me know if you need help with this.

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.

None yet

9 participants