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

use ancestry in replace_children #20990

Merged
merged 1 commit into from
Jan 28, 2021

Conversation

d-m-u
Copy link
Contributor

@d-m-u d-m-u commented Jan 28, 2021

the api uses replace_children (https://github.com/ManageIQ/manageiq-api/blob/01bb977042d6b75e73a0e568b4b3fd455b9d0ec7/app/controllers/api/vms_controller.rb#L107) which, as it isn't an ancestry method, wasn't extended to use the gem functionality.

currently broken: any api call that's updating vm ancestry

thanks @agrare
this fixes the failing api specs

@miq-bot assign @agrare
@miq-bot add_label bug

broken in #20274

related: ManageIQ/manageiq-api#994

@d-m-u
Copy link
Contributor Author

d-m-u commented Jan 28, 2021

rspec ./spec/requests/vms_spec.rb:364 # Vms API Vm edit can edit a VM with an appropriate role has two failing expectations:

     1.3) Failure/Error: expect(vm.reload.children).to match_array(new_vms)

            expected collection contained:  [#<ManageIQ::Providers::Openstack::CloudManager::Vm id: 71000000000124, vendor: "openstack", format: ...emory_hot_add_increment: nil, hostname: nil, ems_ref_type: nil, restart_needed: nil, ancestry: nil>]
            actual collection contained:    [#<ManageIQ::Providers::Openstack::CloudManager::Vm id: 71000000000122, vendor: "openstack", format: ..._increment: nil, hostname: nil, ems_ref_type: nil, restart_needed: nil, ancestry: "71000000000121">]
            the missing elements were:      [#<ManageIQ::Providers::Openstack::CloudManager::Vm id: 71000000000124, vendor: "openstack", format: ...emory_hot_add_increment: nil, hostname: nil, ems_ref_type: nil, restart_needed: nil, ancestry: nil>]
            the extra elements were:        [#<ManageIQ::Providers::Openstack::CloudManager::Vm id: 71000000000122, vendor: "openstack", format: ..._increment: nil, hostname: nil, ems_ref_type: nil, restart_needed: nil, ancestry: "71000000000121">]
          # ./spec/requests/vms_spec.rb:389:in `block (3 levels) in <top (required)>'
          # /Users/drewmu/.rvm/gems/ruby-2.5.5/gems/webmock-3.11.1/lib/webmock/rspec.rb:37:in `block (2 levels) in <top (required)>'

should be fixed by the replace_children addition here

     1.4) Failure/Error: expect(vm.parent).to eq(vm_openstack2)

            expected: #<ManageIQ::Providers::Openstack::CloudManager::Vm id: 71000000000126, vendor: "openstack", format: n...memory_hot_add_increment: nil, hostname: nil, ems_ref_type: nil, restart_needed: nil, ancestry: nil>
                 got: nil

should be fixed by the save!, which i think is the responsibility of the caller, so the accompanying pr adds that single line

@d-m-u d-m-u force-pushed the fix_api_replace_children_vms_call branch 2 times, most recently from 7bc16d0 to 7841db9 Compare January 28, 2021 08:49
d-m-u added a commit to d-m-u/manageiq-api that referenced this pull request Jan 28, 2021
it should be the responsibility of the caller to save this, not the mixin code
see the related ManageIQ/manageiq#20990
@d-m-u d-m-u force-pushed the fix_api_replace_children_vms_call branch 2 times, most recently from a2293e1 to cd35a2c Compare January 28, 2021 09:53
@d-m-u d-m-u requested a review from gtanzillo as a code owner January 28, 2021 09:53
@agrare agrare assigned kbrock and agrare and unassigned agrare Jan 28, 2021
d-m-u added a commit to d-m-u/manageiq-api that referenced this pull request Jan 28, 2021
it should be the responsibility of the caller to save this, not the mixin code
see the related ManageIQ/manageiq#20990
@d-m-u d-m-u force-pushed the fix_api_replace_children_vms_call branch 2 times, most recently from 91574b0 to 430f52a Compare January 28, 2021 14:26
@d-m-u d-m-u force-pushed the fix_api_replace_children_vms_call branch from 430f52a to fb77dbf Compare January 28, 2021 14:57
@agrare
Copy link
Member

agrare commented Jan 28, 2021

LGTM, @kbrock can you eyeball this as well?

@miq-bot
Copy link
Member

miq-bot commented Jan 28, 2021

Checked commit d-m-u@fb77dbf with ruby 2.6.3, rubocop 0.82.0, haml-lint 0.35.0, and yamllint
2 files checked, 0 offenses detected
Everything looks fine. 🏆

@agrare agrare merged commit f22e653 into ManageIQ:master Jan 28, 2021
@d-m-u d-m-u deleted the fix_api_replace_children_vms_call branch January 28, 2021 16:17
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