Nir Soffer has posted comments on this change.
Change subject: tests: prevent hook validation decorator from leaving scripts installed
......................................................................
Patch Set 7:
(1 comment)
....................................................
File lib/vdsm/utils.py
Line 130: logging.warning("Directory: %s already removed", directoryToRemove)
Line 131: else:
Line 132: logging.error("Removing directory: %s failed", directoryToRemove,
Line 133: exc_info=True)
Line 134: raise
If you raise, why not let the caller handle this and log its own error?
I would write this as:
try:
shutil.rmtree(path)
except OSError as e:
if e.errno != errno.ENOENT:
raise
Line 135:
Line 136: IPXMLRPCRequestHandler = SimpleXMLRPCRequestHandler
Line 137:
Line 138:
--
To view, visit http://gerrit.ovirt.org/20310
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I138c3eab77ea6d35d6a997049940923026e06d96
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Miguel Angel Ajo Pelayo <miguelangel(a)ajo.es>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Miguel Angel Ajo Pelayo <miguelangel(a)ajo.es>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
Nir Soffer has posted comments on this change.
Change subject: tests: prevent hook validation decorator from leaving scripts installed
......................................................................
Patch Set 6:
(1 comment)
....................................................
File lib/vdsm/utils.py
Line 129: if e.errno == errno.ENOENT:
Line 130: logging.warning("Directory: %s already removed", directoryToRemove)
Line 131: else:
Line 132: logging.error("Removing directory: %s failed", directoryToRemove,
Line 133: exc_info=True)
It also apply, but it is out of the scope of this patch.
Line 134:
Line 135:
Line 136: IPXMLRPCRequestHandler = SimpleXMLRPCRequestHandler
Line 137:
--
To view, visit http://gerrit.ovirt.org/20310
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I138c3eab77ea6d35d6a997049940923026e06d96
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Miguel Angel Ajo Pelayo <miguelangel(a)ajo.es>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Miguel Angel Ajo Pelayo <miguelangel(a)ajo.es>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
oVirt Jenkins CI Server has posted comments on this change.
Change subject: tests: prevent hook validation decorator from leaving scripts installed
......................................................................
Patch Set 7:
Build Successful
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4408/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5212/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5288/ : SUCCESS
--
To view, visit http://gerrit.ovirt.org/20310
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I138c3eab77ea6d35d6a997049940923026e06d96
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Miguel Angel Ajo Pelayo <miguelangel(a)ajo.es>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Miguel Angel Ajo Pelayo <miguelangel(a)ajo.es>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
Nir Soffer has posted comments on this change.
Change subject: tests: prevent hook validation decorator from leaving scripts installed
......................................................................
Patch Set 6:
(1 comment)
....................................................
File lib/vdsm/utils.py
Line 129: if e.errno == errno.ENOENT:
Line 130: logging.warning("Directory: %s already removed", directoryToRemove)
Line 131: else:
Line 132: logging.error("Removing directory: %s failed", directoryToRemove,
Line 133: exc_info=True)
Also if we assume that the directory was removed, why log a warning about it? This just fill the logs with bogus warnings. A debug message would be better here.
Line 134:
Line 135:
Line 136: IPXMLRPCRequestHandler = SimpleXMLRPCRequestHandler
Line 137:
--
To view, visit http://gerrit.ovirt.org/20310
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I138c3eab77ea6d35d6a997049940923026e06d96
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Miguel Angel Ajo Pelayo <miguelangel(a)ajo.es>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Miguel Angel Ajo Pelayo <miguelangel(a)ajo.es>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
Nir Soffer has posted comments on this change.
Change subject: tests: prevent hook validation decorator from leaving scripts installed
......................................................................
Patch Set 6:
(1 comment)
....................................................
File lib/vdsm/utils.py
Line 129: if e.errno == errno.ENOENT:
Line 130: logging.warning("Directory: %s already removed", directoryToRemove)
Line 131: else:
Line 132: logging.error("Removing directory: %s failed", directoryToRemove,
Line 133: exc_info=True)
Is it common that we want to ignore errors when removing directories?
I think it will be safer to ignore only the ENOENT error by default, and let the caller handle other errors.
Line 134:
Line 135:
Line 136: IPXMLRPCRequestHandler = SimpleXMLRPCRequestHandler
Line 137:
--
To view, visit http://gerrit.ovirt.org/20310
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I138c3eab77ea6d35d6a997049940923026e06d96
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Miguel Angel Ajo Pelayo <miguelangel(a)ajo.es>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Miguel Angel Ajo Pelayo <miguelangel(a)ajo.es>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
oVirt Jenkins CI Server has posted comments on this change.
Change subject: tests: prevent hook validation decorator from leaving scripts installed
......................................................................
Patch Set 6:
Build Successful
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4401/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5205/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5281/ : SUCCESS
--
To view, visit http://gerrit.ovirt.org/20310
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I138c3eab77ea6d35d6a997049940923026e06d96
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Miguel Angel Ajo Pelayo <miguelangel(a)ajo.es>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Miguel Angel Ajo Pelayo <miguelangel(a)ajo.es>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No