Dan Kenigsberg has posted comments on this change. Change subject: vdsm-tool: Add register verb ......................................................................
Patch Set 5: Code-Review-1 (7 comments) https://gerrit.ovirt.org/#/c/40966/5/lib/vdsm/tool/register.py File lib/vdsm/tool/register.py: Line 74: # The oVirt Node TUI currently only support registration Line 75: # via https://. However, If users decide to use Line 76: # 80 plain port, we can keep the legacy. Line 77: proto = "https://"; Line 78: if self.engine_port == "80": why do we want to support plaintext? it is insecure, and engine should better block it. Is it useful for debug? Beyond that, the protocol should not be determined by the port. if someone wants plaintext, he'd better say so explicitly e.g. --protocol=http. Line 79: proto = "http://"; Line 80: Line 81: self.engine_url = "{pt}{e}:{p}".format(pt=proto, Line 82: e=self.engine_fqdn, Line 88: Line 89: if check_fqdn == "False": Line 90: # User input False is str type, converting to bool Line 91: self.check_fqdn = False Line 92: else: this is too fragile, since "False" means False, while "FALSE" means True. I suggest using vdsm.utils.tobool(). Line 93: self.check_fqdn = True Line 94: Line 95: self.logger.debug("Check FQDN: {v}".format(v=self.check_fqdn)) Line 96: Line 149: Line 150: try: Line 151: res = requests.get(__GET_VERSION, verify=False) Line 152: except Exception as e: Line 153: self.logger.error(e, exc_info=True) that's what self.exception() is for; you don't need the e object at all. Line 154: raise e Line 155: Line 156: if res.status_code != 200: Line 157: self.reg_protocol = "legacy" Line 213: Line 214: ih = logging.StreamHandler(stream=sys.stdout) Line 215: ih.setLevel(logging.INFO) Line 216: Line 217: i_fmt = logging.Formatter("%(message)s", I'm confused. why do you need different handlers for ERROR and INFO, with different log format? this is bound to confuse debugging. The format should simple specify the log level of the message. Line 218: "%m/%d/%Y %I:%M:%S %p") Line 219: Line 220: fh.setFormatter(de_fmt) Line 221: ch.setFormatter(de_fmt) Line 247: if res.status_code != 200: Line 248: self.logger.error("http response was not OK", exc_info=True) Line 249: raise requests.RequestException Line 250: except requests.RequestException as e: Line 251: self.logger.error("Cannot connect to engine. {f} matches " again: exception() is nicer than error(). No need for e object. Line 252: "the FQDN of Engine?".format(f=self.engine_fqdn), Line 253: exc_info=True) Line 254: raise e Line 255: Line 289: return fp Line 290: Line 291: def host_uuid(self): Line 292: """ Line 293: Determine host UUID it does not only determine. it also *sets* it on /etc/vdsm/vdsm.id, which is quite surprsing, and must be documented here. Line 294: """ Line 295: if self.reg_protocol == "legacy": Line 296: # REQUIRED_FOR: Engine 3.3 Line 297: # The legacy version uses the format: UUID_MACADDRESS Line 434: '--engine-fqdn engine.mydomain' Line 435: ) Line 436: Line 437: # Needed by vdsm-tool. Otherwise it complains about register Line 438: # option doesn't exist that's very odd. I don't see it happening on other tool verbs. Can you ask Yaniv Bronheim why this can happen? Line 439: parser.add_argument( Line 440: 'register', Line 441: ) Line 442: -- To view, visit https://gerrit.ovirt.org/40966 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ica800027beec1e5a20165bb5e1e78baf9283c1da Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Douglas Schilling Landgraf <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Douglas Schilling Landgraf <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
