Alon Bar-Lev has posted comments on this change. Change subject: node: Determine node version ......................................................................
Patch Set 2: (6 comments) http://gerrit.ovirt.org/#/c/29503/2/src/plugins/ovirt-host-deploy/node/node_ver.py File src/plugins/ovirt-host-deploy/node/node_ver.py: this can go into the detect plugin Line 1: # ovirt-host-deploy -- ovirt host deployer Line 2: # Copyright (C) 2012-2014 Red Hat, Inc. Line 3: # Line 4: # This library is free software; you can redistribute it and/or Line 47: def _get_node_version(self, version_file): Line 48: version = None Line 49: try: Line 50: with open(version_file) as f: Line 51: version_str = f.read().strip('\n') read().splitlines() please Line 52: for part in version_str.split(' '): Line 53: if ( Line 54: len(part) > 0 and Line 55: part[0].isdigit() Line 54: len(part) > 0 and Line 55: part[0].isdigit() Line 56: ): Line 57: # 1st part starting with number should be version Line 58: version = part this is bad. we need to file rfe for node for proper version token, we should not guess. Line 59: break Line 60: except (IOError, OSError, ValueError): Line 61: self.logger.debug( Line 62: 'Error getting node version', Line 56: ): Line 57: # 1st part starting with number should be version Line 58: version = part Line 59: break Line 60: except (IOError, OSError, ValueError): please drop these... we already discuss that. Line 61: self.logger.debug( Line 62: 'Error getting node version', Line 63: exc_info=True, Line 64: ) Line 82: for file in version_files: Line 83: version = self._get_node_version(file) Line 84: if version is not None: Line 85: node_version = version Line 86: break why so complex? version = None f = ( glob.glob('/etc/rhev-hypervisor-release') + glob.glob('/etc/ovirt-node-*-release') + [None,] )[0] if f is not None: logic Line 87: Line 88: self.logger.debug( Line 89: "Node version: '%s'", Line 90: node_version, Line 88: self.logger.debug( Line 89: "Node version: '%s'", Line 90: node_version, Line 91: ) Line 92: self.environment.setdefault( it is not default, it is hard detect here Line 93: odeploycons.VdsmEnv.OVIRT_NODE_VERSION, Line 94: node_version -- To view, visit http://gerrit.ovirt.org/29503 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Icfa4fb729cb2db3fba9076a1184d2906cee06868 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-host-deploy Gerrit-Branch: master Gerrit-Owner: Martin Peřina <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Martin Peřina <[email protected]> Gerrit-Reviewer: Oved Ourfali <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
