Adam Litke has posted comments on this change. Change subject: Adding supervdsm uts ......................................................................
Patch Set 3: I would prefer that you didn't submit this (3 inline comments) Looks good, just a few small comments to address. .................................................... Commit Message Line 4: Commit: Yaniv Bronhaim <[email protected]> Line 5: CommitDate: 2012-10-17 18:33:58 +0200 Line 6: Line 7: Adding supervdsm uts Line 8: Better commit message next time if you would please... Line 9: Change-Id: I08880b12db8a6ca9cc57d74d2ef2a86980a5b097 .................................................... File vdsm/supervdsm.py Line 92: def __init__(self): Line 93: self._firstLaunch = True Line 94: self.pidfile = PIDFILE Line 95: self.timestamp = TIMESTAMP Line 96: self.address = ADDRESS Since these are used by the tests, they have become a de-facto API. Could you add a comment to say this so that someone knows they need to update the tests if they change these. Line 97: Line 98: def open(self, *args, **kwargs): Line 99: return self._manager.open(*args, **kwargs) Line 100: Line 165: self._log.debug("Trying to connect to Super Vdsm") Line 166: try: Line 167: self._manager.connect() Line 168: except Exception, ex: Line 169: self._log.debug("Connect to %s failed %s", self.address, ex) Why is this being downgraded to a warning? As long as you're changing this line, use exc_info=True to print exception rather than what is there. Line 170: raise Line 171: self._svdsm = self._manager.instance() Line 172: Line 173: def launch(self): -- To view, visit http://gerrit.ovirt.org/8600 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I08880b12db8a6ca9cc57d74d2ef2a86980a5b097 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
