Darshan N has posted comments on this change. Change subject: gluster: volume snapshot list verb. ......................................................................
Patch Set 12: (7 comments) http://gerrit.ovirt.org/#/c/36087/12/vdsm/gluster/cli.py File vdsm/gluster/cli.py: Line 1232: volume[volumeName] = {} Line 1233: volume[volumeName]['snapshots'] = [] Line 1234: volume[volumeName]['snapRemaining'] = tree.find( Line 1235: 'snapInfo/originVolume/snapRemaining' Line 1236: ).text > how about Done Line 1237: volume[volumeName]['snapCount'] = tree.find( Line 1238: 'snapInfo/originVolume/snapCount' Line 1239: ).text Line 1240: if volume[volumeName]['snapCount'] == 0: Line 1235: 'snapInfo/originVolume/snapRemaining' Line 1236: ).text Line 1237: volume[volumeName]['snapCount'] = tree.find( Line 1238: 'snapInfo/originVolume/snapCount' Line 1239: ).text > Please remove snapCount which is a duplicate and it can be get by len(volum Done Line 1240: if volume[volumeName]['snapCount'] == 0: Line 1241: return {} Line 1242: for el in tree.findall('snapInfo/snapshots/snapshot'): Line 1243: snapshot = {} Line 1236: ).text Line 1237: volume[volumeName]['snapCount'] = tree.find( Line 1238: 'snapInfo/originVolume/snapCount' Line 1239: ).text Line 1240: if volume[volumeName]['snapCount'] == 0: > Does the output have missing 'snapInfo/snapshots' or 'snapInfo/snapshots' i Done Line 1241: return {} Line 1242: for el in tree.findall('snapInfo/snapshots/snapshot'): Line 1243: snapshot = {} Line 1244: snapshot['snapshotUuid'] = el.find('uuid').text Line 1242: for el in tree.findall('snapInfo/snapshots/snapshot'): Line 1243: snapshot = {} Line 1244: snapshot['snapshotUuid'] = el.find('uuid').text Line 1245: snapshot['snapDescription'] = "" if el.find('description') is None\ Line 1246: else el.find('description').text > cannot the below work? if el.find('description') is None then the above statement will raise an exception Line 1247: snapshot['createTime'] = el.find('createTime').text Line 1248: snapshot['snapVolume'] = el.find('snapVolume/name').text Line 1249: snapshot['snapVolumeStatus'] = el.find('snapVolume/status').text Line 1250: snapshot['snapshotName'] = el.find('name').text Line 1245: snapshot['snapDescription'] = "" if el.find('description') is None\ Line 1246: else el.find('description').text Line 1247: snapshot['createTime'] = el.find('createTime').text Line 1248: snapshot['snapVolume'] = el.find('snapVolume/name').text Line 1249: snapshot['snapVolumeStatus'] = el.find('snapVolume/status').text > you should send status as 'ACTIVATED' or 'DEACTIVATED' than 'Started' or so Done Line 1250: snapshot['snapshotName'] = el.find('name').text Line 1251: volume[volumeName]['snapshots'].append(snapshot) Line 1252: return volume Line 1253: Line 1293: volumes[volumeName]['snapRemaining'] = el.find( Line 1294: 'snapVolume/originVolume/snapRemaining').text Line 1295: volumes[volumeName]['snapCount'] = el.find( Line 1296: 'snapVolume/originVolume/snapCount').text Line 1297: volumes[volumeName]['snapshots'].append(snapshot) > all above comments apply here Done Line 1298: return volumes Line 1299: Line 1300: Line 1301: @makePublic Line 1311: try: Line 1312: if volumeName: Line 1313: return _parseVolumeSnapshotList(xmltree) Line 1314: else: Line 1315: return _parseSnapshotList(xmltree) > How about having this function name as _parseAllVolumeSnapshotList()? If y Done Line 1316: except _etreeExceptions: -- To view, visit http://gerrit.ovirt.org/36087 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I06945d6781432b7fb40e417dd21c0ecf107de132 Gerrit-PatchSet: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Darshan N <[email protected]> Gerrit-Reviewer: Bala.FA <[email protected]> Gerrit-Reviewer: Darshan N <[email protected]> Gerrit-Reviewer: Sahina Bose <[email protected]> Gerrit-Reviewer: Shubhendu Tripathi <[email protected]> Gerrit-Reviewer: Timothy Asir <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
