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

Remove the policy checking for request_host_vmotion_enabled. #14429

Merged

Conversation

lfu
Copy link
Member

@lfu lfu commented Mar 21, 2017

There are events request_host_enable_vmotion and request_host_disable_vmotion defined in db/fixtures/miq_event_definitions.csv.
But request_host_vmotion_enabled is not a defined event.

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

@miq-bot assign @gmcculloug
@miq-bot add_label bug, control, darga/yes, euwe/yes

There are events request_host_enable_vmotion and request_host_disable_vmotion defined in db/fixtures/miq_event_definitions.csv.
But request_host_vmotion_enabled is not a defined event.
@miq-bot
Copy link
Member

miq-bot commented Mar 21, 2017

Checked commit lfu@62d0ffe with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks good. ⭐

@@ -487,7 +487,7 @@ def disable_vmotion
def vmotion_enabled?
msg = validate_vmotion_enabled?
if msg[:available] && respond_to?(:vim_vmotion_enabled?)
check_policy_prevent("request_host_vmotion_enabled", "vim_vmotion_enabled?")
vim_vmotion_enabled?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lfu I think removing this check makes sense, but for a slightly different reason. This looks like it was a typo more than an intentional change in event name. However, I do not believe we should be doing the check here for vmotion_enabled?. This method should return if it is supported from the provider and we are doing the policy check if it is requested.

On a side note, these methods should be refactored to move into the VMware specific layer since they all require vim_* methods. Probably along the lines of how we do the raw_* methods for other provider methods.

@gmcculloug gmcculloug merged commit 0d957ec into ManageIQ:master May 31, 2017
@gmcculloug gmcculloug added this to the Sprint 62 Ending Jun 5, 2017 milestone May 31, 2017
@simaishi
Copy link
Contributor

simaishi commented Jun 1, 2017

@lfu is there a BZ? Can you please create if it doesn't exist?

@lfu
Copy link
Member Author

lfu commented Jun 1, 2017

simaishi pushed a commit that referenced this pull request Jun 1, 2017
…on_enabled

Remove the policy checking for request_host_vmotion_enabled.
(cherry picked from commit 0d957ec)

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

simaishi commented Jun 1, 2017

Euwe backport details:

$ git log -1
commit bc5d96070685d095603e15e79defcc7bdf34971f
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Wed May 31 18:08:27 2017 -0400

    Merge pull request #14429 from lfu/delete_checking_request_host_vmotion_enabled
    
    Remove the policy checking for request_host_vmotion_enabled.
    (cherry picked from commit 0d957ec71748cc04d9e7767b92cb397bb64c89e6)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1457924

simaishi pushed a commit that referenced this pull request Jun 9, 2017
…on_enabled

Remove the policy checking for request_host_vmotion_enabled.
(cherry picked from commit 0d957ec)

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

simaishi commented Jun 9, 2017

Fine backport details:

$ git log -1
commit 08fe667bddbd83d087c970d9d57a1aaf000f60a9
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Wed May 31 18:08:27 2017 -0400

    Merge pull request #14429 from lfu/delete_checking_request_host_vmotion_enabled
    
    Remove the policy checking for request_host_vmotion_enabled.
    (cherry picked from commit 0d957ec71748cc04d9e7767b92cb397bb64c89e6)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1460359

@lfu lfu deleted the delete_checking_request_host_vmotion_enabled branch October 16, 2017 20:14
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.

4 participants