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

Update cluster when modified #12927

Merged
merged 1 commit into from
Dec 5, 2016
Merged

Conversation

pkliczewski
Copy link
Contributor

@pkliczewski pkliczewski commented Nov 30, 2016

When we change cluster for a specific vm we were not able to
fetch proper cluster information. We were not able to get this
information from the event due to code limitation.

Now we fetch all the clusters and datacenters so we have enough
information to update the vm.

This patch fixes:
https://bugzilla.redhat.com/1397503

When we change cluster for a specific vm we were not able to
fetch proper cluster information. We were not able to get this
information from the event due to code limitation.

Now we fetch all the clusters and datacenters so we have enough
information to update the vm.

This patch fixes:
https://bugzilla.redhat.com/1397503
@miq-bot
Copy link
Member

miq-bot commented Nov 30, 2016

Checked commit pkliczewski@80f3bf9 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
2 files checked, 2 offenses detected

app/models/manageiq/providers/redhat/infra_manager/refresher.rb

spec/models/manageiq/providers/redhat/infra_manager/refresher_target_vm_spec.rb

@pkliczewski
Copy link
Contributor Author

@miq-bot assign @durandom

@pkliczewski
Copy link
Contributor Author

@borod108 please review

@miq-bot miq-bot assigned durandom and unassigned oourfali Dec 1, 2016
@pkliczewski
Copy link
Contributor Author

Copy link

@borod108 borod108 left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@durandom
Copy link
Member

durandom commented Dec 5, 2016

LGTM @agrare can you have a quick second quick look and merge?
adding to euwe as the BZ has 5.7.1 as target release

@miq-bot assign @agrare
@miq-bot add_label euwe/yes

@miq-bot miq-bot assigned agrare and unassigned durandom Dec 5, 2016
@agrare
Copy link
Member

agrare commented Dec 5, 2016

I'm good with this to fix the bug but we should try to fix the event limitation if we can to return the correct cluster. @pkliczewski what event gets raised when a VMs' cluster is changed?

I'm a little worried that we're reducing the effectiveness of RHEV targeted refresh if between this PR and #12959 we're pulling back all datacenters, hosts, and clusters

@agrare agrare merged commit 588b5f9 into ManageIQ:master Dec 5, 2016
@agrare agrare added this to the Sprint 50 Ending Dec 5, 2016 milestone Dec 5, 2016
simaishi pushed a commit that referenced this pull request Jan 10, 2017
@simaishi
Copy link
Contributor

Euwe backport details:

$ git log -1
commit 86641c87da02c6079b51a2033c99e7c553da8229
Author: Adam Grare <agrare@redhat.com>
Date:   Mon Dec 5 13:45:55 2016 -0500

    Merge pull request #12927 from pkliczewski/clutser_refresh
    
    Update cluster when modified
    (cherry picked from commit 588b5f9c38a58d30afafd02b52daa354bb2ea59c)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1411791

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.

9 participants