Simone Tiraboschi has posted comments on this change. Change subject: Clean the code to pass pep8 checks ......................................................................
Patch Set 3: (41 comments) 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 Done 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 Done 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? Done 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: Done 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? Done 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: Done 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 Done 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? Done 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: Done 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 Done Line 878: ) Line 879: os.rename(meta_file, new_meta_file) Line 880: Line 881: return True 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( Done 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 Done Line 914: ) Line 915: fp = open(meta_file, "w") Line 916: fp.write(text) Line 917: fp.close() 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 Done 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 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 Done 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: Done 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 () Done 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 Done 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 Done 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 Done 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 th Done 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 Done 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 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 Done 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 Done 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 = { Done 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 Done 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 Done 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 Done 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 Done 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 Done 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 Done 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 Done 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 Done 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 Done 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 Done 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 Done 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: Done 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 Done Line 1478: self.make_dir_nfs(tmp_dir, NUMERIC_VDSM_ID, Line 1479: NUMERIC_VDSM_ID, Line 1480: 0770) Line 1481: 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 Done 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 Done 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 Done 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 Done 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 <stira...@redhat.com> Gerrit-Reviewer: Kiril Nesenko <knese...@redhat.com> Gerrit-Reviewer: Sandro Bonazzola <sbona...@redhat.com> Gerrit-Reviewer: Simone Tiraboschi <stira...@redhat.com> Gerrit-Reviewer: Yedidyah Bar David <d...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
