Fabian Deutsch has posted comments on this change. Change subject: Config.exists: fix exists method ......................................................................
Patch Set 2: (1 comment) https://gerrit.ovirt.org/#/c/44727/2/src/ovirt/node/utils/fs/__init__.py File src/ovirt/node/utils/fs/__init__.py: Line 676: if filename and self.is_enabled(): Line 677: from ovirtnode import ovirtfunctions Line 678: return ovirtfunctions.ovirt_safe_delete_config(filename) Line 679: Line 680: def exists(self, filename): > I believe we improving not changing semantics. There were cases where files The issue is that the exists() call is now behaving different than before. And that is considered an API breakage. Before it was returned True when the path was persisted. Now True is returned if the contents of the persisted and target file differ. Imagine this flow: f = "/my/file" c = Config() if not c.exists(f): c.persist(f) # Copy on /config is created and bind mounted change(f) if not c.exists(f): c.persist(f) # Nothing happens Without your change this works, because f will only be persisted if it is not on /config. With your change it will probably fail, because persist() (AFAIK) does only persist files, it does not "update" them. So with your change: f = "/my/file" c = Config() if not c.exists(f): c.persist(f) # Copy on /config is created and bind mounted change(f) if not c.exists(f): # Is now true, bvecause f is changed c.persist(f) # ERROR because f is already persisted I would be more open to this change if persist will re-persist ("sync") a file if it is already persisted. Line 681: """Check if the given file is persisted Line 682: """ Line 683: persisted_path = self._config_path(filename) Line 684: -- To view, visit https://gerrit.ovirt.org/44727 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If1ba23d67516f1690b223e3dd43985e5d9a502ef Gerrit-PatchSet: 2 Gerrit-Project: ovirt-node Gerrit-Branch: ovirt-3.5 Gerrit-Owner: Douglas Schilling Landgraf <[email protected]> Gerrit-Reviewer: Douglas Schilling Landgraf <[email protected]> Gerrit-Reviewer: Fabian Deutsch <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ node-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/node-patches
