Adam Litke has posted comments on this change. Change subject: Adding supervdsm uts ......................................................................
Patch Set 3: (3 inline comments) .................................................... 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: what is 'uts'? Perhaps you could state the rough nature of testing that you are adding. Mention how the tests work (ie. overriding the pid file used by the test server, etc). It would be nice if, when browsing the git log, I can tell a little bit about a patch from the commit message. This message tells me absolutely nothing. 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 Something like this: ## # These variables are overridden during testing. If you make changes to them # be careful to maintain the option of creating a test instance ## If you are opposed to a comment, then you should add a new class method to override these variables. By overriding these variables you have created a new interface to this class. What I am asking for is that you make that interface explicit so we don't accidentally break it in the future. 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) Ok, then keep the message verbosity as warn too. You can change it to debug in your later patch. 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: Saggi Mizrahi <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
