Allow selecting an optional volume type when creating an Openstack cloud volume #4536
Conversation
|
@mansam Could you please address the above codeclimate issues? Thanks. |
mansam
force-pushed
the
openstack-create-volume-with-volume-type
branch
from
bca34ab to
d39008e
Compare
|
This pull request is not mergeable. Please rebase and repush. |
mansam
force-pushed
the
openstack-create-volume-with-volume-type
branch
from
d39008e to
02fe265
Compare
|
@mzazrivec, can you review it... |
| = _("Required") | ||
|
|
||
| .form-group{"ng-class" => "{'has-error': angularForm.volume_type.$invalid}", | ||
| "ng-if" => "vm.cloudVolumeModel.emstype == 'ManageIQ::Providers::Openstack::StorageManager::CinderManager'"} |
There was a problem hiding this comment.
We want to avoid constructs like this as much as possible (i.e. stating specific model name in UI code).
Ideally, this test should happen on the ruby side, using our supports? functionality and just propagate a boolean of some sort to the angular code & the view.
There was a problem hiding this comment.
I'm not aware of supports? being accessible through the API, and the user can change the Storage Manager form field on the fly, so I can't pre-inject a boolean value into the form. With that in mind, what is the appropriate UI pattern for dynamically accessing the result of supports_whatever_feature? when the Storage Manager selection is changed?
There was a problem hiding this comment.
I guess we don't have a universal pattern for this really. But I guess results of the supports? call can come with the storage managers.
There was a problem hiding this comment.
@mzazrivec I don't think it can, since the storage managers are loaded via the API. I think in order to access it via the API it would have to be added as a virtual column like https://github.com/ManageIQ/manageiq-providers-openstack/pull/355/files, and that seems like it would be incredibly messy in the general case since you'd need a virtual column for every feature you wanted to check against.
There was a problem hiding this comment.
Nevermind, apparently there's already precedent for doing exactly that. Still seems incredibly messy, but I suppose that's how it has to be done.
mansam
force-pushed
the
openstack-create-volume-with-volume-type
branch
from
02fe265 to
d69a89e
Compare
|
Checked commits mansam/manageiq-ui-classic@f141f58~...d69a89e with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 **
|
|
Depends on ManageIQ/manageiq#18000 and ManageIQ/manageiq-providers-openstack#358 |
|
@mzazrivec Are these changes sufficient? |
|
@mzazrivec can you review this one today? |
|
Both dependent PRs were merged, could we get this in? |
|
@martinpovolny can you check this out, please? @mzazrivec is on PTO. |
|
@terezanovotna Seeing this PR has |
|
@miq-bot remove_label ux/review Welcome back from PTO @mzazrivec! I don't see any inconsistencies with the rest of the product, do you @mzazrivec? |
|
Nope, no UX inconsistencies found. |
|
@mansam Can this be |
|
@miq-bot add_label hammer/yes |
|
@simaishi Yes, and it looks like the other two components have already been included in Hammer. ManageIQ/manageiq-providers-openstack#358 and ManageIQ/manageiq#18000 |
…ume-type Allow selecting an optional volume type when creating an Openstack cloud volume (cherry picked from commit 7c4afa8) https://bugzilla.redhat.com/show_bug.cgi?id=1550008
|
Hammer backport details: |
Allows selecting an optional Volume Type when creating an Openstack cloud volume. Generalizes the existing AWS volume type code in the new volume form wherever possible.
Implements https://bugzilla.redhat.com/show_bug.cgi?id=1550008