No revert for "active" snapshot

[masayag]

In the snapshot screen when the "active" snapshot is selected
the revert button should be disabled for RHV.

If VM is up, the 'revert' button is disabled due to VM status not 'DOWN'.
If VM is down and the active snapshot is selected, the 'revert' button
is disabled due to selection of 'active' snapshot.

http://bugzilla.redhat.com/1396068

[masayag]

@miq-bot assign @borod108

[masayag]

Screenshots were added to ManageIQ/manageiq-ui-classic#845

[miq-bot]

Checked commit masayag@fd30285 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks good. 👍

[borod108]

LGTM (just like before the split :))
@miq-bot assign @durandom

[masayag]

Bug 1402992 - VM snapshot: revert option is enabled, for Active VM

[masayag]

@miq-bot add_label bug

[durandom]

where is the active param used? I only see the call to this method in the supports block above

[masayag]

@durandom the 'active' is passed from the UI call, when the information about the snapshot is available. See:

https://github.com/ManageIQ/manageiq-ui-classic/pull/845/files#diff-d424af3c1ddd0fc71914bd5b85559c9fR10

[durandom]

and I guess fine/yes according to the BZ?

[masayag]

In addition, the patch was originally targeted to Euwe. Should we add a label for it as well ? I've prepared a dedicated PR for it, once this will be merged, I'll apply to comment on it:
ManageIQ/manageiq#14326

[durandom]

In addition, the patch was originally targeted to Euwe. Should we add a label for it as well ?

nope. Satoe is using the labels to backport. If you have a dedicated euwe PR (because you know the cherry picking will fail) you should not label this PR

[simaishi]

Fine backport (to manageiq repo) details:

$ git log -1
commit 3a4b4992ece416c3c9c4a9030ff56b17aabc92ba
Author: Marcel Hild <hild@b4mad.net>
Date:   Wed Apr 12 10:07:05 2017 +0200

    Merge pull request #5 from masayag/block_revert_of_active_snapshot
    
    No revert for "active" snapshot
    (cherry picked from commit 27d1f9d5afa0da7f8112346eb22220fc10be8b31)

[simaishi]

In addition, the patch was originally targeted to Euwe. Should we add a label for it as well ?

nope. Satoe is using the labels to backport. If you have a dedicated euwe PR (because you know the cherry picking will fail) you should not label this PR

Actually, what I'd prefer is to still mark the PR with euwe/yes, but also mention where the Euwe PR is in the comment. Thanks!

[masayag]

@miq-bot add_label euwe/yes

[masayag]

Euwe PR is here:

ManageIQ/manageiq#14326

[agrare]

@masayag is there a way to tell from the provider which snapshot represents the Active VM? It seems wrong that this relies on the UI telling the provider which snapshot is the "current" one which is all that @active was meant to do.

[simaishi]

Backported to Euwe via ManageIQ/manageiq#14326

[masayag]

@agrare In order to find the active snapshot we should select the snapshot which complies to snapshot.current?.

The validation 'revert_to_snapshot' is defined for the VM class, therefore the information about the selected snapshot is not available in that context. To enable that support, @durandom opened ManageIQ/manageiq#14740

If there was an option to pass the selected snapshot as an argument to the validation method, we could have check for that snapshot if it is the active one and report the proper reason via the unsupported_reason_add method of the supported_feature_mixin.

Alternately, we can expose the selected snapshot as an instance variable and require it instead of the @active in https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/helpers/application_helper/button/vm_snapshot_revert.rb#L2
And with the existing code it requires to change https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/helpers/application_helper/button/vm_snapshot_revert.rb#L10 to something like try(:revert_to_snapshot_denied_message, @selected_snapshot) which isn't ideal as well.
So not sure what is the preferred way - exposing another instance variable to represent the selected_snapshot or to reuse the existing @active

I assume that declaring selected_snapshot as instance variable would be needed even with ManageIQ/manageiq#14740