Bala.FA has posted comments on this change. Change subject: gluster: volume snapshot list verb. ......................................................................
Patch Set 12: Code-Review-1 (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 volume[volumeName] = {'snapRemaining': tree.find('snapInfo/originVolume/snapRemaining').text, 'snapshots': []} 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(volume[volumeName]['snapshots']) 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' if snapCount is 0? If so this is valid, you need to write test case to cover this. 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? el.find('description').text or '' 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 something to consistent with activate/deactivate verbs 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 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 you like it should be changed all over in this patch 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
