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.

Sign up for GitHub

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

Jump to bottom

Dont return Storage Services if They arent present #240

Merged
merged 2 commits into from
Mar 22, 2018
Merged

Dont return Storage Services if They arent present #240

merged 2 commits into from
Mar 22, 2018

Conversation

jerryk55
Copy link
Member

Check if the cinder and swift services are actually present before
checking the names and returning them.

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

along with ManageIQ/manageiq#17067.

Order of merging is irrelevant. Related PR was ManageIQ/manageiq#16922

@roliveri @hsong-rh please review and merge when appropriate.

@jerryk55 jerryk55 mentioned this pull request Feb 28, 2018
Merged
@jerryk55
Dont return Storage Services if They arent present
44b23b6 
Check if the cinder and swift services are actually present before
checking the names and returning them.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1458959
@jerryk55
Make sure proper Cinder Mixin is Picked Up
5aaa513 
Use the top-level cinder mixin.
@jerryk55 jerryk55 force-pushed the dont_add_uninitialized_storage_managers branch from 17e4678 to 5aaa513 Compare March 20, 2018 14:34
@miq-bot
Copy link
Member

miq-bot commented Mar 20, 2018

Checked commits jerryk55/manageiq-providers-openstack@44b23b6~...5aaa513 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🏆

@jerryk55
Copy link
Member Author

@roliveri since the dependent PR has been merged this can be also when you are ready. Thanks.

roliveri
roliveri reviewed Mar 22, 2018
View reviewed changes
app/models/manageiq/providers/openstack/cloud_manager.rb
@@ -30,7 +30,7 @@ class ManageIQ::Providers::Openstack::CloudManager < ManageIQ::Providers::CloudM
:class_name => "ManageIQ::Providers::StorageManager",
:autosave => true

include ManageIQ::Providers::Openstack::CinderManagerMixin
Copy link
Member

Choose a reason for hiding this comment

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

@jerryk55
Was this changed from CinderManagerMixin and you're changing it back?

Copy link
Member Author

@jerryk55 jerryk55 Mar 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Yes it was changed by #194 and I was of the impression it was breaking my tests. Which of course means I'm probably breaking something in that PR now. Um...

Copy link
Contributor

Choose a reason for hiding this comment

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

how does this help? The codes are identical..

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh good. Then we can delete the identical code that was added under the Openstack Provider. No need for duplicated code, right?

@roliveri roliveri merged commit 97fb7ee into ManageIQ:master Mar 22, 2018
@roliveri roliveri added this to the Sprint 82 Ending Mar 26, 2018 milestone Mar 22, 2018
@Ladas
Copy link
Contributor

Ladas commented Mar 23, 2018

@jerryk55 same should be applied to graph refresh https://github.com/Ladas/manageiq-providers-openstack/blob/e263101b27a892a7e0e24f1d80cc705e775ccd0e/app/models/manageiq/providers/openstack/inventory/collector.rb#L77

https://github.com/Ladas/manageiq-providers-openstack/blob/1e060626baa0241490a8729332012eb45f72bbcf/app/models/manageiq/providers/openstack/inventory/collector/storage_manager/cinder_manager.rb#L4

And targeted refresh collector for cinder:
https://github.com/Ladas/manageiq-providers-openstack/blob/1e060626baa0241490a8729332012eb45f72bbcf/app/models/manageiq/providers/openstack/inventory/collector/target_collection.rb#L194

there is no graph refresh for swift for now

@Ladas Ladas mentioned this pull request Mar 23, 2018
Merged
@Ladas
Copy link
Contributor

Ladas commented Mar 23, 2018

@jerryk55 also please add specs, so we are sure both old refresh and graph refresh work with storage services not defined.

simaishi pushed a commit that referenced this pull request Mar 26, 2018
@roliveri @simaishi
Merge pull request #240 from jerryk55/dont_add_uninitialized_storage_…
899f155 
…managers

Dont return Storage Services if They arent present
(cherry picked from commit 97fb7ee)

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

Gaprindashvili backport details:

$ git log -1
commit 899f15548608ebe274ed4a99e6f425177e332082
Author: Richard Oliveri <oliveri.richard.github@gmail.com>
Date:   Thu Mar 22 17:45:49 2018 -0400

    Merge pull request #240 from jerryk55/dont_add_uninitialized_storage_managers
    
    Dont return Storage Services if They arent present
    (cherry picked from commit 97fb7eeecc225409e5be604ae8e9e5f103f579d2)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1560692

@simaishi
Copy link
Contributor

simaishi commented Apr 9, 2018 •
edited
Loading

Fine backport (to manageiq repo) details:

$ git log -1
commit dbfabf573d3019143d6d1c3c6c55fc7007496a0e
Author: Richard Oliveri <oliveri.richard.github@gmail.com>
Date:   Thu Mar 22 17:45:49 2018 -0400

    Merge pull request #240 from jerryk55/dont_add_uninitialized_storage_managers
    
    Dont return Storage Services if They arent present
    (cherry picked from commit 97fb7eeecc225409e5be604ae8e9e5f103f579d2)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1560693

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ladas Ladas Ladas left review comments

@roliveri roliveri roliveri approved these changes

Assignees
No one assigned
Labels
bug fine/backported gaprindashvili/backported smart state
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jerryk55 @miq-bot @Ladas @simaishi @roliveri