Dan Kenigsberg has posted comments on this change. Change subject: BZ#832227: vdsm-reg.conf: change vdc_host_port ......................................................................
Patch Set 6: I would prefer that you didn't submit this (5 inline comments) .................................................... File vdsm/netinfo.py Line 46: def isHostReachable(host, port=None, ssl=True, timeoutSec=5): is netinfo the correct place for this function? this module is about reporting host network configuration. this operation is something completely different. Line 49: _connect = httplib.HTTPSConnection no need for an underscore for a local variable. but this is a class, so better keep a leading uppercase. Line 53: conn = _connect(host, port=port, timeout=timeoutSec) It seems that you do not need to test port: passing port=None means "use default". I think it would be nicer to keep the default (and the name!) of timeout as in the Connection classes (timeout=socket._GLOBAL_DEFAULT_TIMEOUT) .................................................... File vdsm_reg/engine.py.in Line 178: rhevm_port = self.rhevm_server_port.value() engine_port is a better name, right? Line 184: if rhevm_port == "443": begs for a separate function. action() is too long already. -- To view, visit http://gerrit.ovirt.org/5367 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifb36d1ca0614741d24b512572f101f848b24e6d3 Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Douglas Schilling Landgraf <[email protected]> Gerrit-Reviewer: Bala.FA <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Douglas Schilling Landgraf <[email protected]> Gerrit-Reviewer: Michael Burns <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> _______________________________________________ vdsm-patches mailing list [email protected] https://fedorahosted.org/mailman/listinfo/vdsm-patches
