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

Rename network module to not overrun network class #16994

Merged
merged 1 commit into from
Feb 13, 2018

Conversation

d-m-u
Copy link
Contributor

@d-m-u d-m-u commented Feb 13, 2018

This addresses the concern here wherein ManageIQ::Network is conflicting with ::Network...in particular on the has_many :networks relationship on ExtManagementSystem (or possibly ManageIQ::Providers::InfraManager, which strangely redefines has_many :networks), an issue which was created here

@miq-bot miq-bot added the wip label Feb 13, 2018
@d-m-u d-m-u force-pushed the fixing_network_module_naming branch from 383d304 to 91c8472 Compare February 13, 2018 16:17
@d-m-u d-m-u changed the title [WIP] Rename network module to not overrun network class Rename network module to not overrun network class Feb 13, 2018
@miq-bot miq-bot removed the wip label Feb 13, 2018
@d-m-u
Copy link
Contributor Author

d-m-u commented Feb 13, 2018

@d-m-u
Copy link
Contributor Author

d-m-u commented Feb 13, 2018

@miq-bot add_label bug, core

@d-m-u d-m-u force-pushed the fixing_network_module_naming branch from 91c8472 to a01857a Compare February 13, 2018 16:54
@miq-bot
Copy link
Member

miq-bot commented Feb 13, 2018

Checked commit d-m-u@a01857a with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
5 files checked, 0 offenses detected
Everything looks fine. ⭐

Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

👍 LGTM - will need to change the other PRs

@jrafanie jrafanie self-assigned this Feb 13, 2018
Copy link
Member

@gtanzillo gtanzillo left a comment

Choose a reason for hiding this comment

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

@d-m-u This looks good 👍 Are there additional PRs to handle the users of this module?

@jrafanie jrafanie merged commit ca48813 into ManageIQ:master Feb 13, 2018
@jrafanie jrafanie modified the milestone: Sprint 79 Ending Feb 12, 2018 Feb 13, 2018
@d-m-u d-m-u deleted the fixing_network_module_naming branch February 13, 2018 18:31
@jrafanie
Copy link
Member

@d-m-u This looks good 👍 Are there additional PRs to handle the users of this module?

Yeah, I see a few. I'll open PRs to fix them.

@jrafanie
Copy link
Member

Ok, I think I updated the clients of the old module name. @gildub Please confirm if you think there are others.

Note, I believe we have two modules with the same name now.

https://github.com/ManageIQ/manageiq-network_discovery/blob/master/lib/manageiq/network_discovery.rb

The PROVIDERS_BY_TYPE constant could be a problem since it looks like it's the same name constant now in ManageIQ::NetworkDiscovery and also in ManageIQ::NetworkDiscovery::Discovery. I'm not sure why we have both sets of code now. @gildub any ideas?

https://github.com/ManageIQ/manageiq-network_discovery/blob/3290c1e1c6bf1855eee2d3dbf8a8bd8da83940ba/lib/manageiq/network_discovery.rb#L72-L83

PROVIDERS_BY_TYPE = {
:ipmi => "ManageIQ::NetworkDiscovery::IPMI::Discovery",
:msvirtualserver => "ManageIQ::Providers::Microsoft::Discovery",
:mswin => "ManageIQ::Providers::Microsoft::Discovery",
:scvmm => "ManageIQ::Providers::Microsoft::Discovery",
:openstack_infra => "ManageIQ::Providers::Openstack::Discovery",
:rhevm => "ManageIQ::Providers::Redhat::Discovery",
:esx => "ManageIQ::Providers::Vmware::Discovery",
:virtualcenter => "ManageIQ::Providers::Vmware::Discovery",
:vmwareserver => "ManageIQ::Providers::Vmware::Discovery"
}.freeze

@gildub
Copy link
Contributor

gildub commented Feb 14, 2018

@jrafanie, the name space change is to allow a smooth transition to the new discovery modules location, details in ManageIQ/manageiq-network_discovery#8.

There are now effectively 2 modules with the same name and this is going to create an issue with discovery.

The best way to address this is use a different name like ManageIQ::Discovery instead, to avoid conflicts, I'll submit patches consequently.

I believe ultimately having Models STI not allowing to use proper name spaces is something to be addressed because this is a bad structural constraint.

Thank you for your time on this one and sorry for the trouble.

@gildub
Copy link
Contributor

gildub commented Feb 14, 2018

@jrafanie, BTW IPMI is bit of a black sheep because it doesn't have its own space, maybe one day it will ;)

@gildub
Copy link
Contributor

gildub commented Feb 15, 2018

@jrafanie, ManageIQ/manageiq-providers-vmware#183 is the last one

jrafanie added a commit to jrafanie/manageiq-providers-ovirt that referenced this pull request Feb 15, 2018
We had a regression and tests didn't detect it:
ManageIQ#208
ManageIQ/manageiq#16994
jrafanie added a commit to jrafanie/manageiq-providers-ovirt that referenced this pull request Feb 15, 2018
Note, Ovirt namespace exists in the client gem and also in the
ManageIQ::Providers namespace so we need to clarify which Ovirt we want,
namely, the ovirt gem's one.

We had a regression and tests didn't detect it:
ManageIQ#208
ManageIQ/manageiq#16994
jrafanie added a commit to jrafanie/manageiq-providers-openstack that referenced this pull request Feb 15, 2018
jrafanie added a commit to jrafanie/manageiq-providers-scvmm that referenced this pull request Feb 20, 2018
@gildub
Copy link
Contributor

gildub commented Mar 7, 2018

@miq-bot add_label gaprindashvili/yes

simaishi pushed a commit that referenced this pull request Mar 8, 2018
Rename network module to not overrun network class
(cherry picked from commit ca48813)

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

simaishi commented Mar 8, 2018

Gaprindashvili backport details:

$ git log -1
commit 1a2bf02ba3c467a1a506cef8e6d5348631f1b0fe
Author: Joe Rafaniello <jrafanie@users.noreply.github.com>
Date:   Tue Feb 13 13:28:26 2018 -0500

    Merge pull request #16994 from d-m-u/fixing_network_module_naming
    
    Rename network module to not overrun network class
    (cherry picked from commit ca488135596eb353985bbfe9496f3506d0411fb6)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1552665

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.

7 participants