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

Move VIM_SELECTOR_SPEC out of the base EmsRefresh #20

Merged
merged 1 commit into from Mar 7, 2017

Conversation

agrare
Copy link
Member

@agrare agrare commented Mar 6, 2017

The VIM_SELECTOR_SPEC is used to filter properties that we care about from the VMware provider to reduce memory usage. It is a very VMware specific construct but was in EmsRefresh::VcUpdates in the main manageiq repository.

This moves the selection spec out of EmsRefresh under ManageIQ::Providers::Vmware::InfraManager::VimSelectorSpec

@agrare
Copy link
Member Author

agrare commented Mar 6, 2017

@durandom this is pluggable-providers/provider-extraction territory, can you review?

@agrare agrare force-pushed the refactor_vim_selector_spec branch from 601074a to f2f56be Compare March 6, 2017 21:25
@durandom
Copy link
Member

durandom commented Mar 6, 2017

Looks like a move in the right direction :)

How about moving the whole EmsRefresh::VcUpdates to the Vmware namespace?
But it's also a good opportunity for refactoring, so breaking it up makes sense too.

Isnt this whole VimBroker construct really Vmware specific? Maybe you can chat with @jrafanie how we can move MiqVimBrokerWorker over here?

@agrare
Copy link
Member Author

agrare commented Mar 6, 2017

@durandom that's exactly what I'm planning on 😄 right now VcUpdates still needs to be in EmsRefresh so I can't just move it out (yet) but that's coming soon
Also plan on moving the broker worker and the core refresh worker here as well as they both are VMware only.

Copy link
Member

@durandom durandom left a comment

Choose a reason for hiding this comment

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

👍 should I merge?

@@ -15,7 +15,7 @@ def self.init_console(use_vim_broker = false)
def provider.use_vim_broker?; @__use_vim_broker; end
klass = use_vim_broker ? MiqVimBroker : MiqVimInventory
klass.cacheScope = :cache_scope_ems_refresh
klass.setSelector(EmsRefresh::VcUpdates::VIM_SELECTOR_SPEC)
klass.setSelector(ManageIQ::Providers::Vmware::InfraManager::SelectorSpec::VIM_SELECTOR_SPEC)
Copy link
Member

Choose a reason for hiding this comment

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

you could use parent to avoid the FQN

klass.setSelector(parent::SelectorSpec::VIM_SELECTOR_SPEC)

but it looks like we seldom use this...

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, yeah this is just used when running EmsRefresh on the rails console

@agrare
Copy link
Member Author

agrare commented Mar 7, 2017

👍 should I merge?

If ManageIQ/manageiq#14205 looks good to you too then go for it :)

@durandom
Copy link
Member

durandom commented Mar 7, 2017

Yeah, the corresponding PR looks great too

@durandom durandom merged commit 43474ba into ManageIQ:master Mar 7, 2017
@agrare agrare deleted the refactor_vim_selector_spec branch March 7, 2017 14:52
@agrare
Copy link
Member Author

agrare commented Mar 7, 2017

Thanks @durandom ! The rest of VcUpdates is up next :)

@agrare agrare added this to the Sprint 56 Ending Mar 13, 2017 milestone Mar 8, 2017
@agrare agrare self-assigned this Jun 5, 2018
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.

None yet

2 participants