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

Deleting Code For Rename VM #7916

Merged
merged 2 commits into from
Nov 1, 2021
Merged

Conversation

MelsHyrule
Copy link
Member

@MelsHyrule MelsHyrule commented Oct 18, 2021

Fixes #7912

Renaming a VM from certain pages can cause errors.

VM Controllers to consider are: vm_controller, vm_infra_controller, vm_or_template_controller and vm_cloud_controller. However vm_or_template_controller and vm_cloud_controller handled pages do not have the option to rename their VMs so no changes needed here.

After some discussion with @agrare @Fryguy @kavyanekkalapu @chessbyte and @jrafanie we concluded that for this PR we will be removing the rename button from pages that allow for multi select, i.e. if the user wants to rename a VM they have to click into the VM's details page.

BEFORE

vm_controller page

rename.from.vm.controller.mov

Note: This is a multi selectable table. If a user selects multiple rows / VMs the rename button will be disabled.

vm_infra_controller

renaming.vm.infra.mov

Note: this page is working as. intended so we want to ensure we preserve this functionality in this PR.

Screenshots

Error after trying to rename from multi-select table
Screen Shot 2021-10-22 at 12 25 51 PM

AFTER

Screen.Recording.2021-10-19.at.10.27.52.AM.mov

Screenshots

No renaming option from multi-select table
Screen Shot 2021-10-22 at 12 05 01 PM

Renaming can only be done from details view
Screen Shot 2021-10-22 at 12 05 30 PM

The renaming page UI
Screen Shot 2021-10-22 at 12 05 35 PM

@miq-bot miq-bot added the wip label Oct 18, 2021
@MelsHyrule MelsHyrule force-pushed the rename_vm branch 2 times, most recently from a4031a0 to 95d1099 Compare October 18, 2021 20:27
@Fryguy
Copy link
Member

Fryguy commented Oct 19, 2021

However vm_or_template_controller and vm_cloud_controller handled pages do not have the option to rename their VMs so no changes needed here.

@agrare Was this an oversight? Not saying we should add for this PR, but should this be added eventually?

@Fryguy
Copy link
Member

Fryguy commented Oct 19, 2021

Going Compute > Infra > providers > pick a provider > VMs > pick a VM > Rename this VM the Rename this VM Button is greyed out for some VMs. Is this intended behavior?

It's possible that VMs from Cloud providers are greyed out because they don't support rename. Are all of the VMs with this problem Cloud VMs?

@MelsHyrule
Copy link
Member Author

Going Compute > Infra > providers > pick a provider > VMs > pick a VM > Rename this VM the Rename this VM Button is greyed out for some VMs. Is this intended behavior?

It's possible that VMs from Cloud providers are greyed out because they don't support rename. Are all of the VMs with this problem Cloud VMs?

@Fryguy From what i've seen all VMs in providers are not rename-able so i'm inclined to say yes

@Fryguy
Copy link
Member

Fryguy commented Oct 19, 2021

You can also hover greyed out items and it should say why in a tooltip.

@agrare
Copy link
Member

agrare commented Oct 19, 2021

@Fryguy it looks like VMware is the only provider type that currently implements this feature

@Fryguy
Copy link
Member

Fryguy commented Oct 19, 2021

@agrare Should we open an issue to support this across the board? Seems like something we'd want consistently.

@agrare
Copy link
Member

agrare commented Oct 19, 2021

Yeah definitely, I don't see why the UI/API shouldn't have this be possible for all VM/Template types since it is guarded by supports :rename

@Fryguy
Copy link
Member

Fryguy commented Oct 19, 2021

Created ManageIQ/manageiq#21510

@MelsHyrule MelsHyrule force-pushed the rename_vm branch 4 times, most recently from 703979a to ec29410 Compare October 22, 2021 15:13
@MelsHyrule MelsHyrule changed the title [WIP] Rename VM Deleting Code For Rename VM Oct 22, 2021
@miq-bot miq-bot removed the wip label Oct 22, 2021
@MelsHyrule
Copy link
Member Author

@miq-bot add-reviewer @Fryguy
@miq-bot add-reviewer @kavyanekkalapu
@miq-bot assign @kavyanekkalapu

@Fryguy
Copy link
Member

Fryguy commented Oct 25, 2021

Even though this PR is correct, I feel like we should keep the mixin and only mix it into vminfra and vmcloud. In this PR the method is moved directly into vm_infra_controller.rb, but once we implement ManageIQ/manageiq#21510, then we're gonna have to update this again to add it to vm_cloud_controller.rb (and perhaps other places like template controllers). Would like @agrare's thoughts here.

@agrare
Copy link
Member

agrare commented Oct 27, 2021

@Fryguy yes agreed, rename can be a valid operation for both infra and cloud type VMs

@miq-bot
Copy link
Member

miq-bot commented Oct 28, 2021

Checked commits MelsHyrule/manageiq-ui-classic@4f5b0b4~...12b43c1 with ruby 2.6.3, rubocop 1.13.0, haml-lint 0.35.0, and yamllint
5 files checked, 0 offenses detected
Everything looks fine. 👍

@Fryguy
Copy link
Member

Fryguy commented Nov 1, 2021

LGTM - @kavyanekkalapu please review.

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.

Renaming a VM from the hosts or ems_infra page fails
5 participants