Nir Soffer has posted comments on this change. Change subject: ImageManifest: introduce ImageManifest class ......................................................................
Patch Set 12: Code-Review-1 (3 comments) https://gerrit.ovirt.org/#/c/43551/12/tests/sdm_indirection_tests.py File tests/sdm_indirection_tests.py: Line 283: Line 284: Line 285: class FakeImageManifest(image.ImageManifest): Line 286: def __init__(self): Line 287: self.repoPath = '/rhev/data-center' We don't need this now - why not use the original signature and pass the repo path when you create the manifest? Line 288: Line 289: @recorded Line 290: def getImageDir(self, sdUUID, imgUUID): Line 291: pass https://gerrit.ovirt.org/#/c/43551/12/vdsm/storage/image.py File vdsm/storage/image.py: Line 88: Line 89: Line 90: class ImageManifest(object): Line 91: def __init__(self, repoPath): Line 92: self.repoPath = repoPath Why not readonly property? Line 93: Line 94: def getImageDir(self, sdUUID, imgUUID): Line 95: """ Line 96: Return image directory Line 118: cls.log.error("createImageRollback: Cannot remove dirty image " Line 119: "folder %s" % (imageDir)) Line 120: Line 121: def __init__(self, repoPath): Line 122: self.manifest = ImageManifest(repoPath) Why public? Are caller expected to be able to replace a manifest? Lets go with the default that everything is private. We will expose stuff using properties only when required. Line 123: Line 124: @property Line 125: def repoPath(self): Line 126: return self.manifest.repoPath -- To view, visit https://gerrit.ovirt.org/43551 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic7a78aef10902b9baf6f88d2e053e2232e2a1267 Gerrit-PatchSet: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
