On Nov. 16, 2015, 3:19 p.m., Nils Philippsen wrote:
src/rolekit/server/rolebase.py, lines 130-132 http://reviewboard-fedoraserver.rhcloud.com/r/230/diff/2/?file=1209#file1209line130
I'm not sure I'm too happy with overloading the state this way. Just from looking at the code, I think it should work but I still think it would be cleaner if we kept the state as `SYSTEMD` so it's explicit that one needs to consult systemd (whether by listening to signals or polling) to find out the state of the target. AIUI, this state gets saved to the JSON file in `/etc/rolekit/roles`, and I'd like it better if none of "must consult systemd" states are ever persisted there.
OK, so it turns out that there was actually only one case where it would get written out to disk: in the deferred role processing. I've modified that code so that it will always explicitly set the state to NASCENT before writing out the deferred settings JSON. I then modified rolesettings.py to only set the SYSTEMD state if something other than NASCENT or ERROR is present in the settings data.
I've examined the rest of the code and with the exception of deferred processing, settings.write() is never called anywhere before change_state(), so SYSTEMD should never appear in the file.
On Nov. 16, 2015, 3:19 p.m., Nils Philippsen wrote:
src/rolekit/server/io/systemd.py, lines 63-69 http://reviewboard-fedoraserver.rhcloud.com/r/230/diff/2/?file=1208#file1208line63
That's pretty minimalistic. Are you sure you don't want to put this in a later commit that adds a little code? ;)
This was leftover from a first implementation that I ultimately scrapped but forgot to remove. New version of the patch corrects this.
On Nov. 16, 2015, 3:19 p.m., Nils Philippsen wrote:
src/rolekit/server/io/rolesettings.py, line 30 http://reviewboard-fedoraserver.rhcloud.com/r/230/diff/2/?file=1207#file1207line30
This bogus import comes from review #226. Please remove it in that commit.
Done.
- Stephen
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/230/#review555 -----------------------------------------------------------
On Nov. 12, 2015, 9:21 p.m., Stephen Gallagher wrote:
This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/230/
(Updated Nov. 12, 2015, 9:21 p.m.)
Review request for RoleKit Mailing List, Miloslav Trmac, Nils Philippsen, Stephen Gallagher, and Thomas Woerner.
Repository: rolekit
Description
Get role state from systemd
Diffs
src/rolekit/config/__init__.py.in 05d9f7b2bda59cf0b4ed6e1f277880ff2092b3c7 src/rolekit/dbus_utils.py a5f5a1d5ee4f32865fca6459615ea3359f3ca11d src/rolekit/server/io/rolesettings.py c2424f1b166eb9eec5c59e3b79047cb482ca9081 src/rolekit/server/io/systemd.py b105483c7fa689a43a960ef78aa1105f7aa58b42 src/rolekit/server/rolebase.py 510190a5fbd66858417e9f75f73ded63e6236b85
Diff: http://reviewboard-fedoraserver.rhcloud.com/r/230/diff/
Testing
Thanks,
Stephen Gallagher
rolekit-commits@lists.fedorahosted.org