Yedidyah Bar David has posted comments on this change. Change subject: Clean the code to pass pep8 checks ......................................................................
Patch Set 3: (46 comments) Great job! Don't be overwhelmed by the number of comments... It might be better to make or find a beautifying tool to do this automatically - Sandro, did you ever use any? If 'find', it should be adaptable to our existing style. Generally we try to make changes do just one thing. We are not always strict about this, but this is a huge change, and I think it should contain no changes to logic at all, just styling/indentation. http://gerrit.ovirt.org/#/c/26176/3/src/__main__.py File src/__main__.py: Line 1471 Line 1472 Line 1473 Line 1474 Line 1475 same Line 274: try: Line 275: opts = ["--%s=%s" % (k, v) Line 276: for k, v in cp.items("ImageUploader")] Line 277: (new_options, args) = self.parser.parse_args(args=opts, Line 278: values=self.options) We usually do something like (new_options, args) = self.parser.parse_args( args=opts, values=self.options, ) Line 279: self.from_option_groups(new_options, self.parser) Line 280: self.from_options(new_options, self.parser) Line 281: except ConfigParser.NoSectionError: Line 282: pass Line 536: "the storage domain didn't have a status " Line 537: "element.") Line 538: else: Line 539: logging.debug( Line 540: _("DC(%s) does not have a storage domain.") % dcName) move ')' to next line? Line 541: if len(imageAry) > 0: Line 542: imageAry.sort(key=get_name) Line 543: fmt = "%-30s | %-25s | %s" Line 544: print fmt % (_("Export Storage Domain Name"), _("Datacenter"), Line 541: if len(imageAry) > 0: Line 542: imageAry.sort(key=get_name) Line 543: fmt = "%-30s | %-25s | %s" Line 544: print fmt % (_("Export Storage Domain Name"), _("Datacenter"), Line 545: _("Export Domain Status")) same here: something( item, item, ... ) Line 546: print "\n".join(fmt % (name, dcName, status) Line 547: for name, dcName, status in imageAry) Line 548: else: Line 549: ExitCodes.exit_code = ExitCodes.LIST_IMAGE_ERR Line 550: logging.error(_("There are no export storage domains.")) Line 551: else: Line 552: ExitCodes.exit_code = ExitCodes.LIST_IMAGE_ERR Line 553: logging.error( Line 554: _("There are no datacenters with Export storage domains.")) ')' to next line? Line 555: Line 556: def get_host_and_path_from_export_domain(self, exportdomain): Line 557: """ Line 558: Given a valid export storage domain, this method will return the Line 664: logging.debug( Line 665: "Size of %s:\t%s bytes\t%.1f 1K-blocks\t%.1f MB" % ( Line 666: ovf_file, size_in_bytes, size_in_bytes / 1024.0, Line 667: (size_in_bytes / 1024.0) / 1024.0) Line 668: ) Perhaps: logging.debug( "Size of %s:\t%s bytes\t%.1f 1K-blocks\t%.1f MB" % ( ovf_file, size_in_bytes, size_in_bytes / 1024.0, (size_in_bytes / 1024.0) / 1024.0 ) ) Line 669: logging.debug( Line 670: "Available space in %s:\t%s bytes\t%.1f 1K-blocks\t%.1f MB" % ( Line 671: dest_dir, dest_dir_size, dest_dir_size / 1024.0, Line 672: (dest_dir_size / 1024.0) / 1024.0) Line 669: logging.debug( Line 670: "Available space in %s:\t%s bytes\t%.1f 1K-blocks\t%.1f MB" % ( Line 671: dest_dir, dest_dir_size, dest_dir_size / 1024.0, Line 672: (dest_dir_size / 1024.0) / 1024.0) Line 673: ) same Line 674: Line 675: if dest_dir_size > size_in_bytes: Line 676: return (True, size_in_bytes) Line 677: else: Line 686: os.seteuid(uid) Line 687: dir_stat = os.statvfs(remote_dir) Line 688: except Exception: Line 689: raise Exception( Line 690: "unable to test the available space on %s" % remote_dir) ) to next? Line 691: finally: Line 692: os.seteuid(0) Line 693: os.setegid(0) Line 694: Line 842: Line 843: return retVal Line 844: Line 845: def update_meta_file(self, source_dir, old_image_id, new_image_id, Line 846: image_group_id): Also here: def something( param1, param2, ... ): Line 847: """ Line 848: Update the IMAGE attribute in the meta file with Line 849: the the given disk group ID and rename the META file Line 850: with the new disk ID Line 873: Line 874: old_image_dir = os.path.dirname(meta_file) Line 875: new_meta_file = os.path.join(old_image_dir, '%s.meta' % new_image_id) Line 876: logging.debug('old meta file(%s) new meta file(%s)' % Line 877: (meta_file, new_meta_file) large indent Line 878: ) Line 879: os.rename(meta_file, new_meta_file) Line 880: Line 881: return True Line 903: logging.debug("Image dictionary %s" % image_id_dict) Line 904: if ary[0] in image_id_dict: Line 905: logging.debug( Line 906: "Substituting old PUUID(%s) with new PUUID(%s)" % ( Line 907: ary[0], image_id_dict[ary[0]]) "Substituting..." % ( ary[0], ... ) Line 908: ) Line 909: text = re.sub(r'PUUID=.*', "PUUID=%s" Line 910: % image_id_dict[ary[0]], text Line 911: ) Line 906: "Substituting old PUUID(%s) with new PUUID(%s)" % ( Line 907: ary[0], image_id_dict[ary[0]]) Line 908: ) Line 909: text = re.sub(r'PUUID=.*', "PUUID=%s" Line 910: % image_id_dict[ary[0]], text text = re.sub( r'...', "PU..." % image, text ) Line 911: ) Line 912: logging.debug('Writing meta file\n%s' Line 913: % text Line 914: ) Line 909: text = re.sub(r'PUUID=.*', "PUUID=%s" Line 910: % image_id_dict[ary[0]], text Line 911: ) Line 912: logging.debug('Writing meta file\n%s' Line 913: % text large indent Line 914: ) Line 915: fp = open(meta_file, "w") Line 916: fp.write(text) Line 917: fp.close() Line 919: logging.error("Unable rewrite metafile. Message: %s" % ex) Line 920: return False Line 921: return True Line 922: Line 923: def __update_xml_item_puuid(self, ovf_file, tree, image_id_dict): Is this pep8? Not sure if needed, please separate to different change. There were a few other minor changes I ignored, but this one is imo too big for a 'pep8 change'. Line 924: """ Line 925: This method will update the Content/Section/Item/Parent element Line 926: UUIDs with correct replacement for images with snapshots. Line 927: """ Line 929: iterator = tree.findall('Content/Section') Line 930: for sec in iterator: Line 931: for attr in sec.attrib: Line 932: if str(sec.attrib[attr]).endswith( Line 933: 'VirtualHardwareSection_Type' large indent Line 934: ): Line 935: logging.debug("tag(%s) text(%s) attr(%s) class(%s)" Line 936: % (sec.tag, sec.text, sec.attrib, sec) Line 937: ) Line 932: if str(sec.attrib[attr]).endswith( Line 933: 'VirtualHardwareSection_Type' Line 934: ): Line 935: logging.debug("tag(%s) text(%s) attr(%s) class(%s)" Line 936: % (sec.tag, sec.text, sec.attrib, sec) logging.debug("stuff" % ( param, param, ... ) Line 937: ) Line 938: itemElement = sec.findall('Item') Line 939: for item in itemElement: Line 940: logging.debug("item tag(%s) item text(%s) item " Line 940: logging.debug("item tag(%s) item text(%s) item " Line 941: "attr(%s) class(%s)" Line 942: % (item.tag, item.text, Line 943: item.attrib, item) Line 944: ) same Line 945: instance_id_tag = None Line 946: host_resource_tag = None Line 947: resource_type = None Line 948: parent_tag = None Line 950: # Iterate through the child elements of an info Line 951: # and ensure that it has all of the requisite Line 952: # elements that describe a disk. Line 953: if str(elem.tag).endswith('ResourceType') \ Line 954: and elem.text == '17': We usually do something like: if ( str().endswith() and ... == '17' ): Line 955: resource_type = elem.text Line 956: elif str(elem.tag).endswith('HostResource'): Line 957: host_resource_tag = elem.tag Line 958: elif str(elem.tag).endswith('InstanceId'): Line 959: instance_id_tag = elem.tag Line 960: elif str(elem.tag).endswith('Parent'): Line 961: parent_tag = elem.tag Line 962: if (instance_id_tag and host_resource_tag \ Line 963: and resource_type see above. Also, you do not need the '\' because you already added () Line 964: ): Line 965: # Update the PUUID from old to new. Line 966: tmp = item.find(parent_tag) Line 967: if tmp.text in image_id_dict: Line 966: tmp = item.find(parent_tag) Line 967: if tmp.text in image_id_dict: Line 968: logging.debug( Line 969: "old puuid id(%s) new puuid (%s)" Line 970: % (tmp.text, large indent Line 971: image_id_dict[tmp.text]) Line 972: ) Line 973: tmp.text = image_id_dict[tmp.text] Line 974: return self.write_ovf_file(ovf_file, tree) Line 973: tmp.text = image_id_dict[tmp.text] Line 974: return self.write_ovf_file(ovf_file, tree) Line 975: except Exception, e: Line 976: logging.error("Content/Section/Item/Parent element. " Line 977: "Message: %s" % e large indent Line 978: ) Line 979: return False Line 980: Line 981: def __update_xml_disk_parentref(self, ovf_file, tree, parent_combined_id): Line 992: for elem in sec: Line 993: logging.debug("tag(%s) text(%s) attr(%s) class(%s)" Line 994: % (elem.tag, elem.text, elem.attrib, Line 995: elem) Line 996: ) same Line 997: for attr in elem.attrib: Line 998: if str(attr).endswith('parentRef') and \ Line 999: str(elem.attrib[attr]).strip() != '': Line 1000: logging.debug("old parentRef(%s) " Line 995: elem) Line 996: ) Line 997: for attr in elem.attrib: Line 998: if str(attr).endswith('parentRef') and \ Line 999: str(elem.attrib[attr]).strip() != '': see above. Even if you prefer just '\', we usually move the ':' to under the if: if cond part 1 and cond part 2 \ : stuff This way it's easier to see where the condition ended. Line 1000: logging.debug("old parentRef(%s) " Line 1001: "new parentRef(%s)" Line 1002: % (elem.attrib[attr], Line 1003: parent_combined_id) Line 1000: logging.debug("old parentRef(%s) " Line 1001: "new parentRef(%s)" Line 1002: % (elem.attrib[attr], Line 1003: parent_combined_id) Line 1004: ) same Line 1005: elem.attrib[attr] = parent_combined_id Line 1006: return self.write_ovf_file(ovf_file, tree) Line 1007: except Exception, e: Line 1008: logging.error("Section/Disk/parentRef element. Message: %s" % e) Line 1008: logging.error("Section/Disk/parentRef element. Message: %s" % e) Line 1009: return False Line 1010: Line 1011: def __chek_if_disk(self, item): Line 1012: """ imo should be in a different change Line 1013: Iterate through the child elements of an info and ensure Line 1014: that it has all of the requisite elements that describe Line 1015: a disk. Line 1016: """ Line 1200: for attr in sec.attrib: Line 1201: if str(sec.attrib[attr]).endswith( Line 1202: 'VirtualHardwareSection_Type'): Line 1203: logging.debug("tag(%s) text(%s) attr(%s) class(%s)" % Line 1204: (sec.tag, sec.text, sec.attrib, sec)) same Line 1205: itemElement = sec.findall('Item') Line 1206: for item in itemElement: Line 1207: logging.debug( Line 1208: "item tag(%s) item text(%s) item attr(%s) " Line 1212: host_resource_tag, instance_id_tag, \ Line 1213: resource_type = self.__chek_if_disk(item) Line 1214: Line 1215: if instance_id_tag and host_resource_tag and \ Line 1216: resource_type: same Line 1217: n_id_d = {'combined_ids': None, Line 1218: 'new_image_group_id': None, Line 1219: 'new_image_id': None, Line 1220: 'parent_combined_id': Line 1217: n_id_d = {'combined_ids': None, Line 1218: 'new_image_group_id': None, Line 1219: 'new_image_id': None, Line 1220: 'parent_combined_id': Line 1221: parent_combined_id n_id_d = { 'combined_ids': None, 'stuff': stuff, ... ) Line 1222: } Line 1223: Line 1224: ms, old_image_id = \ Line 1225: self.__remap_id(n_id_d, Line 1253: # Again it's odd that they're not daisy chained. Line 1254: if not self.__update_xml_disk_parentref(ovf_file, Line 1255: tree, Line 1256: n_id_d['parent_combined_id'] Line 1257: ): same Line 1258: return False Line 1259: except Exception, ex: Line 1260: logging.error("Unable to update the disk ID in the OVF XML. " Line 1261: "Message: %s" % ex) Line 1257: ): Line 1258: return False Line 1259: except Exception, ex: Line 1260: logging.error("Unable to update the disk ID in the OVF XML. " Line 1261: "Message: %s" % ex) same Line 1262: retVal = False Line 1263: Line 1264: return retVal Line 1265: Line 1273: iterator = tree.findall('Content/Name') Line 1274: elem_ary = list(iterator) Line 1275: if len(elem_ary) != 1: Line 1276: logging.error("There should only be one Name element in the " Line 1277: "OVF XML's Content section") same Line 1278: return False Line 1279: else: Line 1280: logging.debug("tag(%s) text(%s) attr(%s)" Line 1281: % (elem_ary[0].tag, elem_ary[0].text, Line 1285: if not self.write_ovf_file(ovf_file, tree): Line 1286: retVal = False Line 1287: except Exception, e: Line 1288: logging.error("Unable to update the Name element of the Content " Line 1289: "section in the OVF XML. Message: %s" % e) same Line 1290: retVal = False Line 1291: return retVal Line 1292: Line 1293: def remove_nics(self, ovf_file, tree): Line 1300: iterator = tree.findall('Content/Section') Line 1301: for sec in iterator: Line 1302: for attr in sec.attrib: Line 1303: if str(sec.attrib[attr]).\ Line 1304: endswith('VirtualHardwareSection_Type'): same Line 1305: logging.debug("tag(%s) text(%s) attr(%s) class(%s)" Line 1306: % (sec.tag, sec.text, sec.attrib, sec) Line 1307: ) Line 1308: itemElement = sec.findall('Item') Line 1302: for attr in sec.attrib: Line 1303: if str(sec.attrib[attr]).\ Line 1304: endswith('VirtualHardwareSection_Type'): Line 1305: logging.debug("tag(%s) text(%s) attr(%s) class(%s)" Line 1306: % (sec.tag, sec.text, sec.attrib, sec) same Line 1307: ) Line 1308: itemElement = sec.findall('Item') Line 1309: for item in itemElement: Line 1310: logging.debug("item tag(%s) item text(%s) " Line 1427: Line 1428: return retVal Line 1429: Line 1430: def copy_files_nfs(self, source_dir, remote_dir, address, Line 1431: ovf_size, ovf_file_name): same Line 1432: """ Line 1433: Copies all of the files in source_dir to remote_dir. Line 1434: """ Line 1435: files_to_copy = self.get_files_to_copy(source_dir) Line 1434: """ Line 1435: files_to_copy = self.get_files_to_copy(source_dir) Line 1436: if len(files_to_copy) < 1: Line 1437: logging.error("The internal directory structure " Line 1438: "of the OVF file is invalid") same Line 1439: return Line 1440: Line 1441: # Check for pre-existing files. We can't just overwrite Line 1442: # what is already there. Line 1451: logging.error(_('%s exists on %s.' Line 1452: ' Either remove it or supply' Line 1453: ' the --force option to ' Line 1454: 'overwrite it.') Line 1455: % (remote_file, address)) same Line 1456: return Line 1457: else: Line 1458: # Remove the file. Line 1459: self.remove_file_nfs(remote_file, Line 1457: else: Line 1458: # Remove the file. Line 1459: self.remove_file_nfs(remote_file, Line 1460: NUMERIC_VDSM_ID, Line 1461: NUMERIC_VDSM_ID) same Line 1462: Line 1463: # Is there enough room for what we want to copy now? Line 1464: retVal, remote_dir_size = self.space_test_nfs(remote_dir, ovf_size, Line 1465: NUMERIC_VDSM_ID, Line 1462: Line 1463: # Is there enough room for what we want to copy now? Line 1464: retVal, remote_dir_size = self.space_test_nfs(remote_dir, ovf_size, Line 1465: NUMERIC_VDSM_ID, Line 1466: NUMERIC_VDSM_ID) same Line 1467: if not retVal: Line 1468: logging.error(_('There is not enough space in %s (%s bytes) ' Line 1469: 'for the contents of %s (%s bytes)') % Line 1470: (address, remote_dir_size, ovf_file_name, ovf_size)) Line 1465: NUMERIC_VDSM_ID, Line 1466: NUMERIC_VDSM_ID) Line 1467: if not retVal: Line 1468: logging.error(_('There is not enough space in %s (%s bytes) ' Line 1469: 'for the contents of %s (%s bytes)') % same: if not retVal: logging.error( _( 'msg' 'msg' ).format( holder1=value1, ... ) ) Line 1470: (address, remote_dir_size, ovf_file_name, ovf_size)) Line 1471: return Line 1472: Line 1473: # Make the remote directories Line 1473: # Make the remote directories Line 1474: for valid_files in files_to_copy: Line 1475: tmp_dir = os.path.join(remote_dir, os.path.dirname(valid_files)) Line 1476: if not self.exists_nfs(tmp_dir, NUMERIC_VDSM_ID, Line 1477: NUMERIC_VDSM_ID): same Line 1478: self.make_dir_nfs(tmp_dir, NUMERIC_VDSM_ID, Line 1479: NUMERIC_VDSM_ID, Line 1480: 0770) Line 1481: Line 1476: if not self.exists_nfs(tmp_dir, NUMERIC_VDSM_ID, Line 1477: NUMERIC_VDSM_ID): Line 1478: self.make_dir_nfs(tmp_dir, NUMERIC_VDSM_ID, Line 1479: NUMERIC_VDSM_ID, Line 1480: 0770) same Line 1481: Line 1482: # Copy the files with the .ovf being last because Line 1483: # we don't want oVirt to find anything until Line 1484: # it is all there. Line 1499: return Line 1500: Line 1501: # Copy the .ovf *last* Line 1502: self.copy_file_nfs(ovf_file, remote_ovf_file, NUMERIC_VDSM_ID, Line 1503: NUMERIC_VDSM_ID) same Line 1504: Line 1505: def remove_file_nfs(self, file_name, uid, gid): Line 1506: """ Line 1507: Remove a file as the UID and GID provided. Line 1555: ) Line 1556: ) Line 1557: else: Line 1558: raise Exception(_("either export-domain or" Line 1559: " nfs-server must be provided")) same Line 1560: Line 1561: # NFS support. Line 1562: mount_dir = tempfile.mkdtemp() Line 1563: logging.debug('local NFS mount point is %s' % mount_dir) Line 1570: logging.debug('OVF data %s is a directory' % ovf_file) Line 1571: ovf_file_size = self.get_ovf_dir_space(ovf_file) Line 1572: if (ovf_file_size != -1 and self.update_ovf_xml(ovf_file)): Line 1573: self.copy_files_nfs(ovf_file, dest_dir, address, Line 1574: ovf_file_size, ovf_file) same Line 1575: else: Line 1576: try: Line 1577: ovf_extract_dir = tempfile.mkdtemp() Line 1578: logging.debug('local extract directory for OVF is %s' Line 1611: except KeyError: Line 1612: ExitCodes.exit_code = ExitCodes.CRITICAL Line 1613: logging.error(_("A user named %s with a UID and GID of %d must be " Line 1614: "defined on the system to mount the export " Line 1615: "storage domain on %s as Read/Write" same Line 1616: % (NFS_USER, Line 1617: NUMERIC_VDSM_ID, Line 1618: self.configuration.get('export_domain')))) Line 1619: except Exception, ex: -- To view, visit http://gerrit.ovirt.org/26176 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idb9f7416f0f81f0913d3c0145cd3c42e05377d71 Gerrit-PatchSet: 3 Gerrit-Project: ovirt-image-uploader Gerrit-Branch: master Gerrit-Owner: Simone Tiraboschi <[email protected]> Gerrit-Reviewer: Kiril Nesenko <[email protected]> Gerrit-Reviewer: Sandro Bonazzola <[email protected]> Gerrit-Reviewer: Simone Tiraboschi <[email protected]> Gerrit-Reviewer: Yedidyah Bar David <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
