Fabian Deutsch has posted comments on this change. Change subject: Add initramfs re-generation during installation ......................................................................
Patch Set 4: (6 comments) https://gerrit.ovirt.org/#/c/42912/4/scripts/ovirt-node-update-initramfs File scripts/ovirt-node-update-initramfs: Line 32: try: Line 33: initramfs.rebuild() Line 34: except: Line 35: log.error("Initramfs regeneration failed") Line 36: # Raise to exit with != 0 > This is not a valid reason to raise. To exit with non-zero code, use sys.e Done Line 37: raise Line 38: https://gerrit.ovirt.org/#/c/42912/4/src/ovirt/node/utils/system.py File src/ovirt/node/utils/system.py: Line 923: Line 924: fs = Filesystem(device) Line 925: Line 926: except process.CalledProcessError as e: Line 927: LOGGER.debug("Failed to resolve disks: %s" % e.cmd, exc_info=True) > This try except is relevant only around check_output. This will also simpli I partially agree, but I'd rather do this in a spearate patch. Line 928: return fs Line 929: Line 930: def _tokens(self): Line 931: tokens = process.check_output(["blkid", "-o", "export", self.device]) Line 977: LOGGER.exception("Can't mount without a device specified") Line 978: raise RuntimeError("No device was specified when Mount() " Line 979: "was initialized") Line 980: Line 981: args = ["-t", self.fstype if self.fstype else "auto"] > Why not add "mount" here? Done Line 982: if options: Line 983: args += ["-o" + options] Line 984: args += [self.device, self.path] Line 985: Line 983: args += ["-o" + options] Line 984: args += [self.device, self.path] Line 985: Line 986: try: Line 987: utils.process.check_call(["mount"] + args) > Why add "mount" only at the end? Done Line 988: except: Line 989: LOGGER.exception("Can't mount %s on %s" % (self.device, Line 990: self.path)) Line 991: Line 1265: """ Line 1266: def try_unlink(self, path, _log=lambda a: None): Line 1267: try: Line 1268: os.unlink(path) Line 1269: except: > You are hiding the actual error - this is almost bad as "pass". Done. And yes, a good rule of thumb for when only except is allowed. Line 1270: _log("Failed to remove " + path) Line 1271: Line 1272: def _generate_new_initramfs(self, new_initrd): Line 1273: LOGGER.info("Generating new initramfs " Line 1270: _log("Failed to remove " + path) Line 1271: Line 1272: def _generate_new_initramfs(self, new_initrd): Line 1273: LOGGER.info("Generating new initramfs " Line 1274: "'%s' (this can take a while)" % new_initrd) > You can use %r instead of '%s' Done Line 1275: Line 1276: rd_stdout = "" Line 1277: try: Line 1278: rd_stdout = check_output(["dracut", new_initrd], -- To view, visit https://gerrit.ovirt.org/42912 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I209a82ff6bf10edf0857e362584bc6370081c320 Gerrit-PatchSet: 4 Gerrit-Project: ovirt-node Gerrit-Branch: master Gerrit-Owner: Fabian Deutsch <[email protected]> Gerrit-Reviewer: Fabian Deutsch <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ node-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/node-patches
