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

Network Router: More generic use of SupportsFeatureMixin #13073

Closed
wants to merge 1 commit into from
Closed

Network Router: More generic use of SupportsFeatureMixin #13073

wants to merge 1 commit into from

Conversation

gildub
Copy link
Contributor

@gildub gildub commented Dec 9, 2016 •
edited
Loading

More generic use of SupportsFeatureMixin

@gildub gildub changed the title More generic support feature Network Manager: More generic support feature Dec 9, 2016
@gildub
Copy link
Contributor Author

gildub commented Dec 9, 2016

@miq-bot add_label ui

@miq-bot miq-bot added the ui label Dec 9, 2016
@gildub
Copy link
Contributor Author

gildub commented Dec 16, 2016

@dclarizio, would you please assign it?

durandom
durandom suggested changes Dec 19, 2016
View reviewed changes
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.

I'm fine with the changes to supports :network_router

@gildub 👍 for changing to the generic :create , :update features.

But could you open for the removal of floating_ip and security_group another PR? I also see eg. create_floatin_ip still used in https://github.com/ManageIQ/manageiq/blob/0184457040609a6a27062682c8371fb7c98ddc5b/app/models/manageiq/providers/openstack/network_manager/floating_ip.rb

WRT the changes in network_router_controller.rb I would defer to @martinpovolny - isnt there a more generic UI way to create tasks? I feel that this shouldnt be done by the Ems.

@miq-bot
Copy link
Member

miq-bot commented Dec 19, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

@gildub
Copy link
Contributor Author

gildub commented Dec 20, 2016

@durandom,

Thanks for reviewing,

Separating from the other network items makes sense and actually security group has already been taken care of in [1] and floating IP is covered with [2]

[1] #13165
[2] #12097

@miq-bot
Copy link
Member

miq-bot commented Dec 21, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

@gildub gildub changed the title Network Manager: More generic support feature Network Router: More generic support feature Dec 22, 2016
@gildub
Copy link
Contributor Author

gildub commented Dec 22, 2016 •
edited
Loading

@durandom,

Because of other network items merges and some conflicts, I rebased and forced push, sorry for the inconvenience, bottom line this now only impacting network router.

@gildub gildub changed the title Network Router: More generic support feature Network Router: supports feature and raw wrapping Dec 22, 2016
@durandom
Copy link
Member

to make this consistent with the rest of the codebase you'd have to move the delete_network_router and queueing methods to the base class, leave only the raw methods in the openstack class and queue the delete_network_router method - not the raw method.

@miq-bot
Copy link
Member

miq-bot commented Dec 22, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

@chessbyte chessbyte added wip and removed wip labels Jan 3, 2017
@gildub gildub changed the title Network Router: supports feature and raw wrapping Network Router: More generic use of SupportsFeatureMixin Jan 4, 2017
@miq-bot
Copy link
Member

miq-bot commented Jan 4, 2017

Checked commit https://github.com/gildub/manageiq/commit/e547e04799cfbe70e668abaf2b1e1ed64de73c3a with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
1 file checked, 0 offenses detected
Everything looks good. 🏆

durandom
durandom suggested changes Jan 5, 2017
View reviewed changes
app/models/manageiq/providers/openstack/network_manager/network_router.rb
@@ -2,10 +2,26 @@ class ManageIQ::Providers::Openstack::NetworkManager::NetworkRouter < ::NetworkR
include ProviderObjectMixin
include AsyncDeleteMixin

supports :create_network_router
supports :delete_network_router
supports :update_network_router
Copy link
Member

Choose a reason for hiding this comment

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

@gildub @tzumainn do you see a way of removing these features from the SupportsFeatureMixin and also from NetworkManager?

Copy link
Contributor Author

@gildub gildub Jan 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@durandom, are you referring about moving to the base class all related SupportsFeatureMixin code? If so then yes, I'll be looking into it meanwhile that would be part of a separate pull request.

Copy link
Member

Choose a reason for hiding this comment

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

supports :create_network_router is still used in ManageIQ::Providers::Openstack::NetworkManager can this be removed from the NetworkManager?

:delete_network_router and :update_network_router is not used anymore. So those can be removed from SupportsFeatureMixin

@chessbyte chessbyte removed the wip label Jan 5, 2017
durandom
durandom suggested changes Jan 11, 2017
View reviewed changes
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.

please at least remove delete and update from the Mixin.

It would be great to get rid of :create_ too

app/models/manageiq/providers/openstack/network_manager/network_router.rb
@@ -2,10 +2,26 @@ class ManageIQ::Providers::Openstack::NetworkManager::NetworkRouter < ::NetworkR
include ProviderObjectMixin
include AsyncDeleteMixin

supports :create_network_router
supports :delete_network_router
supports :update_network_router
Copy link
Member

Choose a reason for hiding this comment

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

supports :create_network_router is still used in ManageIQ::Providers::Openstack::NetworkManager can this be removed from the NetworkManager?

:delete_network_router and :update_network_router is not used anymore. So those can be removed from SupportsFeatureMixin

@miq-bot
Copy link
Member

miq-bot commented Apr 6, 2017

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

durandom
durandom suggested changes Apr 12, 2017
View reviewed changes
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.

@gildub can you move this PR to the new provider repo, close this and add a link to the new PR?

@miq-bot miq-bot added the ui label Apr 12, 2017
@gildub gildub mentioned this pull request Jun 19, 2017
Merged
@gildub
Copy link
Contributor Author

gildub commented Jun 19, 2017

@durandom,

Done, thanks

ManageIQ/manageiq-providers-openstack#56

@gildub gildub closed this Jun 19, 2017
@gildub gildub deleted the openstack_network_controller_network_router_fix_support_feature branch June 19, 2017 06:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@durandom durandom durandom requested changes

Assignees

@durandom durandom

@tzumainn tzumainn

Labels
providers/network refactoring ui unmergeable
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gildub @miq-bot @durandom @chessbyte @tzumainn