Michal Skrivanek has posted comments on this change. Change subject: API: modernize VM methods ......................................................................
Patch Set 18: (3 comments) indeed we should avoid semantics change, but please do not split into hundred little patches, that's way more difficult to see the bigger picture. If there are only handful of places where we consider semantic change is acceptable it is better to keep it in a single patch https://gerrit.ovirt.org/#/c/61475/18/vdsm/API.py File vdsm/API.py: Line 346 Line 347 Line 348 Line 349 Line 350 > This changed the semantics - before we would raise before we run the hook, and since the hooks are typically quite stupid it may cause a problem indeed. We can likely live with it on master, but if it can be fixed easily it is a better option Line 125: Line 126: @property Line 127: def vm(self): Line 128: v = self._cif.vmContainer.get(self._UUID) Line 129: if not v: > if v is not None: ? isn't it the opposite? Line 130: raise exception.NoSuchVM() Line 131: return v Line 132: Line 133: @api.method Line 263: """ Line 264: Lock user session in guest operating system using guest agent. Line 265: """ Line 266: self.vm.guestAgent.desktopLock() Line 267: if self.vm.guestAgent.isResponsive(): > If you create a variable, I would do: +1 on current version -1 on needless extra indirection Line 268: return {'status': doneCode} Line 269: else: Line 270: return errCode['nonresp'] Line 271: -- To view, visit https://gerrit.ovirt.org/61475 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1e2e238fc632df97b63f7bb2a6293fe1c392a842 Gerrit-PatchSet: 18 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <[email protected]> Gerrit-Reviewer: Arik Hadas <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Irit Goihman <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Milan Zamazal <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list -- [email protected] To unsubscribe send an email to [email protected]
