Change in vdsm[master]: cleanup: Improve networking imports (PEP328)
Antoni Segura Puimedon has posted comments on this change. Change subject: cleanup: Improve networking imports (PEP328) .. Patch Set 2: Code-Review-1 :-( This is because the vdsm daemon directory (vdsm dir in repo or /usr/share/vdsm once installed) is not a package, so no relative imports can be done. The only way that I can see to do what I'd like to do would be to have it be a package. The issue is that the current naming would create a conflict between lib vdsm and daemon vdsm package names. -- To view, visit http://gerrit.ovirt.org/20555 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I249cfa0ad734ea45ecbbecbade9daeed6c3adc12 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Assaf Muller amul...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: cleanup: Improve networking imports (PEP328)
Dan Kenigsberg has posted comments on this change. Change subject: cleanup: Improve networking imports (PEP328) .. Patch Set 2: Code-Review-1 from ..netmodels import Bond, Bridge ValueError: Attempted relative import beyond toplevel package -- To view, visit http://gerrit.ovirt.org/20555 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I249cfa0ad734ea45ecbbecbade9daeed6c3adc12 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Assaf Muller amul...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: cleanup: Improve networking imports (PEP328)
oVirt Jenkins CI Server has posted comments on this change. Change subject: cleanup: Improve networking imports (PEP328) .. Patch Set 2: Verified-1 Build Failed http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5119/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4315/ : FAILURE http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5193/ : ABORTED http://jenkins.ovirt.org/job/vdsm_network_functional_tests/751/ : FAILURE -- To view, visit http://gerrit.ovirt.org/20555 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I249cfa0ad734ea45ecbbecbade9daeed6c3adc12 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Assaf Muller amul...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: cleanup: Improve networking imports (PEP328)
Antoni Segura Puimedon has uploaded a new change for review.
Change subject: cleanup: Improve networking imports (PEP328)
..
cleanup: Improve networking imports (PEP328)
PEP328 ( http://www.python.org/dev/peps/pep-0328/ ) does away with
the ambiguity of where the modules that are being imported reside
making the codebase much more browsable.
With a codebase of:
package/
__init__.py
subpackage1/
__init__.py
moduleX.py
moduleY.py
subpackage2/
__init__.py
moduleZ.py
moduleA.py
this (moduleY.py):
import moduleX
import moduleZ
becomes
from . import moduleX
from ..subpackage2 import moduleZ
Which makes it much more evident where things are and avoids possible
shadowing of packages in site-packages.
Change-Id: I249cfa0ad734ea45ecbbecbade9daeed6c3adc12
Signed-off-by: Antoni S. Puimedon asegu...@redhat.com
---
M lib/vdsm/ipwrapper.py
M lib/vdsm/netconfpersistence.py
M lib/vdsm/netinfo.py
M vdsm/configNetwork.py
M vdsm/netconf/__init__.py
M vdsm/netconf/ifcfg.py
M vdsm/netconf/iproute2.py
M vdsm/netconf/libvirtCfg.py
M vdsm/netmodels.py
M vdsm/sourceRoute.py
M vdsm/sourceRouteThread.py
M vdsm/tc.py
M vdsm/vdsm-restore-net-config
13 files changed, 77 insertions(+), 71 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/55/20555/1
diff --git a/lib/vdsm/ipwrapper.py b/lib/vdsm/ipwrapper.py
index cf0c069..a3455de 100644
--- a/lib/vdsm/ipwrapper.py
+++ b/lib/vdsm/ipwrapper.py
@@ -16,6 +16,7 @@
#
# Refer to the README and COPYING files for full details of the license
#
+from __future__ import absolute_import
from collections import namedtuple
@@ -23,8 +24,8 @@
from netaddr import IPAddress
from netaddr import IPNetwork
-from utils import CommandPath
-from utils import execCmd
+from .utils import CommandPath
+from .utils import execCmd
_IP_BINARY = CommandPath('ip', '/sbin/ip')
diff --git a/lib/vdsm/netconfpersistence.py b/lib/vdsm/netconfpersistence.py
index 633cb94..34c9ea8 100644
--- a/lib/vdsm/netconfpersistence.py
+++ b/lib/vdsm/netconfpersistence.py
@@ -17,16 +17,17 @@
#
# Refer to the README and COPYING files for full details of the license
#
+from __future__ import absolute_import
import errno
import json
import logging
import os
-from config import config
-from tool.restore_nets import restore
-import constants
-import utils
+from .config import config
+from .tool.restore_nets import restore
+from . import constants
+from . import utils
CONF_RUN_DIR = constants.P_VDSM_RUN + 'netconf/'
diff --git a/lib/vdsm/netinfo.py b/lib/vdsm/netinfo.py
index 60088d4..1c41802 100644
--- a/lib/vdsm/netinfo.py
+++ b/lib/vdsm/netinfo.py
@@ -17,27 +17,26 @@
#
# Refer to the README and COPYING files for full details of the license
#
+from __future__ import absolute_import
from collections import namedtuple
-import errno
from fnmatch import fnmatch
from glob import iglob
from itertools import chain
+from xml.dom import minidom
+import errno
import logging
import os
import shlex
import socket
import struct
-from xml.dom import minidom
import ethtool
-from config import config
-import constants
-from ipwrapper import Route
-from ipwrapper import routeShowAllDefaultGateways
-import libvirtconnection
-from ipwrapper import linkShowDev
+from .config import config
+from . import constants
+from . import libvirtconnection
+from .ipwrapper import linkShowDev, Route, routeShowAllDefaultGateways
NET_CONF_DIR = '/etc/sysconfig/network-scripts/'
# ifcfg persistence directories
diff --git a/vdsm/configNetwork.py b/vdsm/configNetwork.py
index 3894691..06a7e9e 100755
--- a/vdsm/configNetwork.py
+++ b/vdsm/configNetwork.py
@@ -16,32 +16,24 @@
#
# Refer to the README and COPYING files for full details of the license
#
+from __future__ import absolute_import
from functools import wraps
import inspect
-import sys
-import os
-import traceback
-import time
import logging
+import os
+import sys
+import time
+import traceback
+from . import hooks
+from . import neterrors as ne
+from .netconf.ifcfg import ConfigWriter, Ifcfg
+from .neterrors import ConfigNetworkError
+from .netmodels import Bond, Bridge, IPv4, IPv6, IpConfig, Nic, Vlan
from vdsm.config import config
-from vdsm import constants
-from vdsm import utils
-from storage.misc import execCmd
-import neterrors as ne
-from neterrors import ConfigNetworkError
-from vdsm import netinfo
-from netconf.ifcfg import ConfigWriter
-from netconf.ifcfg import Ifcfg
-from netmodels import Bond
-from netmodels import Bridge
-from netmodels import IPv4
-from netmodels import IPv6
-from netmodels import IpConfig
-from netmodels import Nic
-from netmodels import Vlan
-import hooks
+from vdsm import constants, netinfo, utils
+from vdsm.utils import execCmd
CONNECTIVITY_TIMEOUT_DEFAULT = 4
diff --git a/vdsm/netconf/__init__.py
Change in vdsm[master]: cleanup: Improve networking imports (PEP328)
Nir Soffer has posted comments on this change. Change subject: cleanup: Improve networking imports (PEP328) .. Patch Set 1: Code-Review-1 (16 comments) Important cleanup - but mixes many unrelated cleanups. This patch does much more than promised in the commit message: - Merge multiple imports into one line - this change is wrong, as it makes reviewing import changes harder. If you want to push this evil practice, do this in another patch. - Change import order - not sure why this was done - Add missing copyright - Fix the years in another copyright Please remove anything which is not related to absolute imports. Commit Message Line 29: from . import moduleX Line 30: from ..subpackage2 import moduleZ Line 31: Line 32: Which makes it much more evident where things are and avoids possible Line 33: shadowing of packages in site-packages. Great commit message! I wish we have more like this. Line 34: Line 35: Change-Id: I249cfa0ad734ea45ecbbecbade9daeed6c3adc12 File lib/vdsm/netinfo.py Line 28: import logging Line 29: import os Line 30: import shlex Line 31: import socket Line 32: import struct Why did you change the order of imports? How is this related to absolute imports? Line 33: Line 34: import ethtool Line 35: Line 36: from .config import config Line 35: Line 36: from .config import config Line 37: from . import constants Line 38: from . import libvirtconnection Line 39: from .ipwrapper import linkShowDev, Route, routeShowAllDefaultGateways How is merging multiple imports to one line import related to the purpose of this patch? Line 40: Line 41: NET_CONF_DIR = '/etc/sysconfig/network-scripts/' Line 42: # ifcfg persistence directories Line 43: NET_CONF_BACK_DIR = constants.P_VDSM_LIB + 'netconfback/' File vdsm/configNetwork.py Line 29: from . import hooks Line 30: from . import neterrors as ne Line 31: from .netconf.ifcfg import ConfigWriter, Ifcfg Line 32: from .neterrors import ConfigNetworkError Line 33: from .netmodels import Bond, Bridge, IPv4, IPv6, IpConfig, Nic, Vlan How is merging multiple import lines to one import line related to the purpose of this patch? Line 34: from vdsm.config import config Line 35: from vdsm import constants, netinfo, utils Line 36: from vdsm.utils import execCmd Line 37: Line 31: from .netconf.ifcfg import ConfigWriter, Ifcfg Line 32: from .neterrors import ConfigNetworkError Line 33: from .netmodels import Bond, Bridge, IPv4, IPv6, IpConfig, Nic, Vlan Line 34: from vdsm.config import config Line 35: from vdsm import constants, netinfo, utils How is merging multiple import lines to one import line related to the purpose of this patch? Line 36: from vdsm.utils import execCmd Line 37: Line 38: CONNECTIVITY_TIMEOUT_DEFAULT = 4 Line 39: Line 32: from .neterrors import ConfigNetworkError Line 33: from .netmodels import Bond, Bridge, IPv4, IPv6, IpConfig, Nic, Vlan Line 34: from vdsm.config import config Line 35: from vdsm import constants, netinfo, utils Line 36: from vdsm.utils import execCmd vdsm.utils.execCmd is *not* storage.misc.execCmd! Line 37: Line 38: CONNECTIVITY_TIMEOUT_DEFAULT = 4 Line 39: Line 40: File vdsm/netconf/__init__.py Line 20: Line 21: import logging Line 22: Line 23: from ..netmodels import Bond, Bridge Line 24: from ..sourceRoute import DynamicSourceRoute, StaticSourceRoute How is merging multiple import lines to one import line related to the purpose of this patch? Line 25: from vdsm.config import config Line 26: from vdsm import netinfo Line 27: from vdsm.netconfpersistence import RunningConfig Line 28: Line 22: Line 23: from ..netmodels import Bond, Bridge Line 24: from ..sourceRoute import DynamicSourceRoute, StaticSourceRoute Line 25: from vdsm.config import config Line 26: from vdsm import netinfo Why did you change the order of imports? Line 27: from vdsm.netconfpersistence import RunningConfig Line 28: Line 29: Line 30: class Configurator(object): File vdsm/netconf/iproute2.py Line 17: # Refer to the README and COPYING files for full details of the license Line 18: # Line 19: Line 20: Line 21: from vdsm.ipwrapper import routeAdd, routeDel, ruleAdd, ruleDel This style makes it harder to review when import are changed. One import per line is a better strategy. Line 22: Line 23: Line 24: class Iproute2(object): Line 25: @staticmethod File vdsm/netconf/libvirtCfg.py Line 19: import libvirt Line 20: from xml.dom.minidom import Document Line 21: from xml.sax.saxutils import escape Line 22: Line 23: from vdsm import libvirtconnection, netinfo How is merging multiple import lines to one import line
Change in vdsm[master]: cleanup: Improve networking imports (PEP328)
oVirt Jenkins CI Server has posted comments on this change. Change subject: cleanup: Improve networking imports (PEP328) .. Patch Set 1: Verified-1 Build Failed http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5084/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4280/ : FAILURE http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5158/ : FAILURE http://jenkins.ovirt.org/job/vdsm_network_functional_tests/747/ : FAILURE -- To view, visit http://gerrit.ovirt.org/20555 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I249cfa0ad734ea45ecbbecbade9daeed6c3adc12 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Assaf Muller amul...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: cleanup: Improve networking imports (PEP328)
Antoni Segura Puimedon has posted comments on this change. Change subject: cleanup: Improve networking imports (PEP328) .. Patch Set 1: (4 comments) Thanks a lot for the review Nir! Important cleanup - but mixes many unrelated cleanups. This patch does much more than promised in the commit message: - Merge multiple imports into one line - this change is wrong, as it makes reviewing import changes harder. If you want to push this evil practice, do this in another patch. I can very well move it to a separate patch so devs can vote on this specific issue alone. - Change import order - not sure why this was done I'll update the commit message to reflect that I'm fixing the imports in general, and part of that is to keep them properly sorted. - Add missing copyright Totally right, will move that into a separate patch. - Fix the years in another copyright This is necessary for the patch. When you touch a file you are supposed to make sure the the copyright is updated to include the current year in the range. Please remove anything which is not related to absolute imports. File vdsm/configNetwork.py Line 32: from .neterrors import ConfigNetworkError Line 33: from .netmodels import Bond, Bridge, IPv4, IPv6, IpConfig, Nic, Vlan Line 34: from vdsm.config import config Line 35: from vdsm import constants, netinfo, utils Line 36: from vdsm.utils import execCmd storage.misc.execCmd only puts a storage specific logger on top of utils execCmd (we moved the original there). Line 37: Line 38: CONNECTIVITY_TIMEOUT_DEFAULT = 4 Line 39: Line 40: File vdsm/sourceRouteThread.py Line 16: # Line 17: # Refer to the README and COPYING files for full details of the license Line 18: # Line 19: from __future__ import absolute_import Line 20: Yeah. This one is unrelated, just my laziness to make an extra patch for that :P Line 21: import logging Line 22: import os Line 23: Line 24: import pyinotify File vdsm/tc.py Line 26: Line 27: import ethtool Line 28: Line 29: from vdsm.constants import EXT_TC Line 30: from vdsm.utils import execCmd answered in another file. Line 31: Line 32: ERR_DEV_NOEXIST = 2 Line 33: Line 34: QDISC_INGRESS = ':' File vdsm/vdsm-restore-net-config Line 1: #! /usr/bin/python Line 2: # Line 3: # Copyright 2011-2013 Red Hat, Inc. If I modify the file in 2013 I should update the year in the header. Line 4: # Line 5: # This program is free software; you can redistribute it and/or modify Line 6: # it under the terms of the GNU General Public License as published by Line 7: # the Free Software Foundation; either version 2 of the License, or -- To view, visit http://gerrit.ovirt.org/20555 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I249cfa0ad734ea45ecbbecbade9daeed6c3adc12 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Assaf Muller amul...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: cleanup: Improve networking imports (PEP328)
Antoni Segura Puimedon has posted comments on this change. Change subject: cleanup: Improve networking imports (PEP328) .. Patch Set 1: Thanks a lot for the review Nir! Important cleanup - but mixes many unrelated cleanups. This patch does much more than promised in the commit message: - Merge multiple imports into one line - this change is wrong, as it makes reviewing import changes harder. If you want to push this evil practice, do this in another patch. I can very well move it to a separate patch so devs can vote on this specific issue alone. - Change import order - not sure why this was done I'll update the commit message to reflect that I'm fixing the imports in general, and part of that is to keep them properly sorted. - Add missing copyright Totally right, will move that into a separate patch. - Fix the years in another copyright This is necessary for the patch. When you touch a file you are supposed to make sure the the copyright is updated to include the current year in the range. Please remove anything which is not related to absolute imports. -- To view, visit http://gerrit.ovirt.org/20555 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I249cfa0ad734ea45ecbbecbade9daeed6c3adc12 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Assaf Muller amul...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: cleanup: Improve networking imports (PEP328)
Nir Soffer has posted comments on this change. Change subject: cleanup: Improve networking imports (PEP328) .. Patch Set 1: Please do not change import order in this patch - it is not related to absolute imports and just make it harder to review. -- To view, visit http://gerrit.ovirt.org/20555 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I249cfa0ad734ea45ecbbecbade9daeed6c3adc12 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Assaf Muller amul...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
