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

Add shared examples for EMS#pause! and #resume! #22716

Conversation

These tests should be run by every provider to ensure these are fully
functional.
@agrare
Copy link
Member Author

agrare commented Sep 26, 2023

@miq-bot cross-repo-tests ManageIQ/manageiq-providers-amazon#827

miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Sep 26, 2023
Comment on lines -531 to -539
emses = {
:amazon_cloud => FactoryBot.create(:ems_amazon, :zone => zone),
:azure_cloud => FactoryBot.create(:ems_azure, :zone => zone),
:google_cloud => FactoryBot.create(:ems_google, :zone => zone),
:openstack_cloud => FactoryBot.create(:ems_openstack, :zone => zone),
:openstack_infra => FactoryBot.create(:ems_openstack_infra, :zone => zone),
:vmware_cloud => FactoryBot.create(:ems_vmware_cloud, :zone => zone),
:vpc_cloud => FactoryBot.create(:ems_ibm_cloud_vpc, :zone => zone)
}
Copy link
Member Author

Choose a reason for hiding this comment

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

I always thought it was wrong that we looped through these providers in core versus having shared_examples that each include. This list doesn't even include all providers, with shared examples we can add a spec test to the provider generator.

@miq-bot
Copy link
Member

miq-bot commented Sep 26, 2023

Checked commit agrare@fca621e with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
2 files checked, 0 offenses detected
Everything looks fine. 🍰

Comment on lines +31 to +34
ems.pause!
ems.reload
ems.resume!
ems.reload
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a little weird but the existing tests do this (https://github.com/ManageIQ/manageiq/pull/22716/files#diff-f50b4b351bf43b590f4afa70fdf2534bb4291339af5d57f9c283b73913a95ab0L623-L627)

It seemed difficult to setup the ems paused with let(:ems). Maybe we could add an Rspec trait for :paused, more investigation needed.

@kbrock
Copy link
Member

kbrock commented Sep 26, 2023

think testing the propagation of network managers may overlap quite a bit with these tests.

Not sure how deep you want to go here
https://github.com/ManageIQ/manageiq/blob/master/app/models/manageiq/providers/base_manager.rb#L44 propagates these values for us, so I'm not sure why we have that code in pause!
https://github.com/ManageIQ/manageiq/blob/master/app/models/ext_management_system.rb#L342-L349
differences:
propagate_child_manager_attributes is using id and pause/resume is using objects. Objects tend to work better for specs/factories and ids tend to work better for performance but since we are probably loading zone into memory anyway
propagate_child_managers is hardcoded with callbacks/mixins and ems uses child_managers
yea, this looks fragile. which checks out to reality.
We have had a few PRs to clean up but it is amazing how many landmines live in code that looks so simple
Also, we never pass a zone into pause() so we could probably change it to pause=(value) and that would be more factory friendly
I don't know the state of our auto save children, but if it is working, this could be cut down a bit
Thoughts on how you would like to proceed?

@Fryguy Fryguy self-assigned this Sep 27, 2023
@Fryguy Fryguy merged commit 9e4b2e3 into ManageIQ:master Sep 28, 2023
@agrare agrare deleted the add_shared_examples_ext_management_system_pause_resume branch September 28, 2023 20:33
@Fryguy
Copy link
Member

Fryguy commented Sep 28, 2023

Backported to quinteros in commit cd4ba46.

commit cd4ba46eaa021c11d254b5c8049b16d10faa429f
Author: Jason Frey <[email protected]>
Date:   Thu Sep 28 16:14:00 2023 -0400

    Merge pull request #22716 from agrare/add_shared_examples_ext_management_system_pause_resume
    
    Add shared examples for EMS#pause! and #resume!
    
    (cherry picked from commit 9e4b2e3ccd914dbd61a85b9e238a33bca2fbf985)

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