-
Notifications
You must be signed in to change notification settings - Fork 62
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
Reconnect host on provider add #189
Conversation
@pkliczewski 'Ladas agrare' is an invalid assignee, ignoring... |
@agrare please review |
@miq-bot add_label gaprindashvili/yes |
@@ -76,6 +76,39 @@ def root_folders(extra_attributes = {}) | |||
attributes.merge!(extra_attributes) | |||
end | |||
|
|||
def hosts(extra_attributes = {}) | |||
reconnect_block = lambda do |inventory_collection, inventory_objects_index, attributes_index| |
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.
we can use the same code for Vms, so we are compatible with old refresh, @agrare what else do we reconnect?
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.
Great idea, just hosts and vms currently.
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.
Lets apply this also to Vms and add specs please :-)
@Ladas @pkliczewski this probably belongs in the main |
@agrare will move it to the main repo. |
@agrare right, so @pkliczewski is adding it here because e.g. VmWare reconnect is different. In here we will reconnect using only ems_ref. |
@Ladas you mean for hosts? That is different behavior between normal and graph refresh then. |
@agrare right @pkliczewski says they need just reconnecting by |
@Ladas that's not the only reason we lookup by hostname, if you drop a host from a provider and connect it to another this would also pick that up. |
This PR requires ManageIQ/manageiq#16750 |
EmsRefresh.refresh(@ems) | ||
@ems.reload | ||
|
||
vm = VmOrTemplate.find_by(:uid_ems => "3a9401a0-bf3d-4496-8acf-edd3e903511f") |
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.
probably also assert that
expect(VmOrTemplate.where(:uid_ems => "3a9401a0-bf3d-4496-8acf-edd3e903511f").count).to eq 1
To make sure the Vm is not duplicated ,same for the Host
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.
OK
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.
Done, @Ladas are you ok with the specs now?
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.
yes, awesome
We need to make sure that when we remove a provider and add it again we will keep using the same host entities. Without this change add and remove provider cause to duplicate hosts. Fixes https://bugzilla.redhat.com/1528859
Checked commit pkliczewski@90fcaed with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0 |
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.
Looks great now 👍
Reconnect host on provider add (cherry picked from commit 057e7d0) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1532332
Gaprindashvili backport details:
|
We need to make sure that when we remove a provider and add it again we
will keep using the same host entities. Without this change add and remove
provider cause to duplicate hosts.
Fixes https://bugzilla.redhat.com/1528859