On Aug. 25, 2015, 1:58 p.m., Nils Philippsen wrote:
> src/rolekit/server/io/systemd.py, line 257
>
<
http://reviewboard-fedoraserver.rhcloud.com/r/178/diff/1/?file=791#file79...
>
> Is there a reason why this is not a normal (or class, or static) method? I.e.:
>
> ...
> @staticmethod
> def optionxform(option):
> "This method optionally transforms an option."
> return option
> ...
No, it was just saving some typing, but it probably needed a comment. All it really needs
to do is become a no-op. If you want me to make it a staticmethod, I will.
On Aug. 25, 2015, 1:58 p.m., Nils Philippsen wrote:
> src/rolekit/server/io/systemd.py, lines 358-366
>
<
http://reviewboard-fedoraserver.rhcloud.com/r/178/diff/1/?file=791#file79...
>
> Call this more of a gut-feeling, but I'm not sure I like the value of a
section being a tuple only if it exists multiple times. I don't think this makes it
easier on the user of the class, they need to check if the section exists more than once
and then handle it differently than if it doesn't. Or do I misunderstand something?
First, this is actually an option inside a section; it's not handling
identically-named sections.
So, the first time an option name appears in the file, it's value is added directly.
The second time the parser encounters that entry, it becomes a tuple. Anyone that
*consumes* this configparser would need to detect whether the value is a tuple or not and
handle it differently. The alternative would be to simply *always* have this turn into a
tuple (rather than only on subsequent additions), but this would be a significant change
from existing configparser behavior. I don't have a great answer for it. In our
rolekit use-case, we are basically "write-only". The values are handled by the
.write() function and get put into the destination unit files properly.
If you have a strong opinion one way or another, please voice it.
On Aug. 25, 2015, 1:58 p.m., Nils Philippsen wrote:
> src/rolekit/server/io/systemd.py, line 388
>
<
http://reviewboard-fedoraserver.rhcloud.com/r/178/diff/1/?file=791#file79...
>
> This looks like a copy-paste error:
>
> `is not None**configparser.RawConfigParser**`
Yeah, probably a stray middle-click. I'll fix that once we sort out the other
questions.
- Stephen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard-fedoraserver.rhcloud.com/r/178/#review475
-----------------------------------------------------------
On Aug. 4, 2015, 10:28 p.m., Stephen Gallagher wrote:
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard-fedoraserver.rhcloud.com/r/178/
-----------------------------------------------------------
(Updated Aug. 4, 2015, 10:28 p.m.)
Review request for RoleKit Mailing List, Miloslav Trmac, Nils Philippsen, Stephen
Gallagher, and Thomas Woerner.
Repository: rolekit
Description
-------
This is a temporary measure; this code will be going upstream into python-systemd in the
near future, but for now we will carry it in rolekit.
There is one known limitation to this implementation; It does not support reading unit
files with multiline values. This won't be an issue in rolekit, but it's
preventing it from going upstream to python-systemd at the moment.
Diffs
-----
src/rolekit/server/io/systemd.py 3b6bca678f70a608e2d54bb3615523dec7ab9271
Diff:
http://reviewboard-fedoraserver.rhcloud.com/r/178/diff/
Testing
-------
Testing performed manually by loading the python module and writing out some sample unit
files. Also tested with the kickstart patch.
Thanks,
Stephen Gallagher