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

Remove the Eventcatcher from CinderManager #14962

Merged

Conversation

hsong-rh
Copy link
Contributor

@hsong-rh hsong-rh commented May 2, 2017

There is no specific events need to be catch for cinder storage manager. Remove it from CinderManager and event catcher list.

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1465395
#14961

@hsong-rh
Copy link
Contributor Author

hsong-rh commented May 2, 2017

@roliveri Please review.

@roliveri roliveri requested review from roliveri and jerryk55 May 2, 2017 15:27
@roliveri roliveri self-assigned this May 2, 2017
@@ -2,8 +2,6 @@
#

class ManageIQ::Providers::StorageManager::CinderManager < ManageIQ::Providers::StorageManager
require_nested :EventCatcher
require_nested :EventParser
require_nested :RefreshParser
require_nested :RefreshWorker
require_nested :Refresher
Copy link
Member

Choose a reason for hiding this comment

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

@hsong-rh - shouldn't the event_monitor_class at the end of this file be removed as well? (as well as the one from the swift_manager.rb - not sure why I added that)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, changed to remove it.

@chessbyte chessbyte requested a review from blomquisg May 3, 2017 00:27
Copy link
Member

@blomquisg blomquisg left a comment

Choose a reason for hiding this comment

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

See Jerry's comment. Otherwise, I'm fine with this change.

Copy link
Member

@roliveri roliveri left a comment

Choose a reason for hiding this comment

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

Echoing @blomquisg comment echoing @jerryk55 comment.

@roliveri
Copy link
Member

@hsong-rh Are you going to address the comment made by @jerryk55 ?

@chessbyte
Copy link
Member

@hsong-rh bump

@miq-bot
Copy link
Member

miq-bot commented Jun 8, 2017

This pull request is not mergeable. Please rebase and repush.

@hsong-rh
Copy link
Contributor Author

@roliveri @blomquisg Made the change according to Jerry's comments. Please review again, thanks.

@miq-bot
Copy link
Member

miq-bot commented Aug 17, 2017

This pull request is not mergeable. Please rebase and repush.

@hsong-rh hsong-rh force-pushed the remove_cindermanager_eventcatcher branch from dfbd06b to 772bdc2 Compare August 18, 2017 12:47
@miq-bot
Copy link
Member

miq-bot commented Aug 18, 2017

Checked commit hsong-rh@772bdc2 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 🍪

@blomquisg blomquisg merged commit e928b59 into ManageIQ:master Aug 18, 2017
@blomquisg blomquisg added this to the Sprint 67 Ending Aug 21, 2017 milestone Aug 18, 2017
juliancheal pushed a commit to juliancheal/manageiq-schema that referenced this pull request Jan 8, 2019
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.

6 participants