-
Notifications
You must be signed in to change notification settings - Fork 356
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
[WIP] Attach detach cloud volume form #8205
[WIP] Attach detach cloud volume form #8205
Conversation
miq_bootstrap('#form_div'); | ||
= render :partial => "layouts/flash_msg" | ||
.col-md-12 | ||
- deviceMountpointRequiered = @volume.ext_management_system.type == 'ManageIQ::Providers::Amazon::StorageManager::Ebs' ? true : false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A. Typo? Requiered
-> Required
B. We shouldn't be doing this type of provider-specific check in the common UI code. These options should be coming from DDF params in the providers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely a typo and I included Device Mountpoint
as that because in the original code it is verified like that (se code here) and i'm still trying to understand what the point of that text field is. When you do the attach_volume_queue this Device Mountpoint
isn't needed or used, we only really need userid and server_ems_red
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def attach_volume_queue(userid, server_ems_ref, device = nil)
takes an optional device
parameter, it looks like on amazon this isn't optional and is required (https://docs.aws.amazon.com/sdk-for-ruby/v3/api/Aws/EC2/Volume.html#attach_to_instance-instance_method)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahhh i see, alrighty ill need to update my api as well then
35ac4db
to
37edd14
Compare
setState((state) => ({ | ||
...state, | ||
isLoading: false, | ||
deviceMountpointRequired: (initialValues.type === 'ManageIQ::Providers::Amazon::StorageManager::Ebs') ? true : false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kavyanekkalapu this is probably out of scope of this changeset but if we have provider specific logic like this we'll need to convert the form to DDF and load this logic from the providers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@agrare Sure Melody can add these to provider. @MelsHyrule Can you check providers supporting this feature and add schema in provider side? @GilbertCherrie converted multiple forms like this. Please reach out to him for some samples and help. Thanks.
9f95cbd
to
cfe995c
Compare
@miq-bot add-reviewer @kavyanekkalapu |
cfe995c
to
d5fa5ff
Compare
Checked commits MelsHyrule/manageiq-ui-classic@5f5ab3c~...ed0238d with ruby 2.6.9, rubocop 1.19.1, haml-lint 0.35.0, and yamllint app/views/cloud_volume/attach.html.haml
app/views/cloud_volume/detach.html.haml
|
This pull request is not mergeable. Please rebase and repush. |
@MelsHyrule Do you want to close this pr? |
@kavyanekkalapu If we close it we'll end up reopening it later when we finish work on the API side ManageIQ/manageiq-api#1147 |
@MelsHyrule Your form conversion pr is merged. once api is merged, you can open a new pr from master to change schema and get schema from api call. |
Depends on 1141 and 8205.
Part of fixing attach / detach from 7603.