Douglas Schilling Landgraf has posted comments on this change. Change subject: vdsm-tool: Add register verb ......................................................................
Patch Set 5: (8 comments) https://gerrit.ovirt.org/#/c/40966/5/lib/vdsm/tool/register.py File lib/vdsm/tool/register.py: Line 51: from Engine Line 52: ssh_port - Port of ssh daemon is running Line 53: check_fqdn - Validate Engine FQDN against CA (True or False) Line 54: Default is TRUE Line 55: vdc_port - The communication port between VDSM and Engine > this is a very vague documentation for a very odd name. Done Line 56: node_fqdn - Specify node FQDN Line 57: node_name - Specify node name Line 58: """ Line 59: self.logger = self._set_logger() 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 bett Totally agreed, let's cut it. 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 Done 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. Done 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 d Done 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. Done 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 i Done 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 As I can see most of others verbs don't use the option schema, like vdsm-tool foo <option> But I might be missing something, if Yaniv has a suggestion, that's would be nice. 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
