On Sept. 22, 2015, 3 nachm., Stephen Gallagher wrote:
config/roles/databaseserver/role.py, lines 51-89 http://reviewboard-fedoraserver.rhcloud.com/r/219/diff/2/?file=1121#file1121line51
This function is very difficult to follow. Please add many comments.
I don't like it very much myself ;), it's too unwieldy. It's basically a substitute for the functions of sed we used, you can match and `replace` or `append`, and optionally append at the end of the file if the regex never matched (for instance if future versions of the config file don't come with the matched directive so defaults get used). I thought about how to make this a bit more lean and easy to use so it's generally usable, but so far haven't come up with some concrete idea.
On Sept. 22, 2015, 3 nachm., Stephen Gallagher wrote:
config/roles/databaseserver/role.py, lines 66-71 http://reviewboard-fedoraserver.rhcloud.com/r/219/diff/2/?file=1121#file1121line66
How are we defining "global" regexes here? Per-file or per-line? That needs explanation. Right now, it looks like it's per-file.
The `global` option specifies that replacing/appending is done more than once, everytime the regex matches. Regex replacements are always done "globally" in a line.
On Sept. 22, 2015, 3 nachm., Stephen Gallagher wrote:
config/roles/databaseserver/role.py, lines 81-83 http://reviewboard-fedoraserver.rhcloud.com/r/219/diff/2/?file=1121#file1121line81
This whitespace usage is uncommon and hard to read.
Yeah. I think this is a place where backslash continuation is acceptable, see: https://www.python.org/dev/peps/pep-0008/#maximum-line-length
On Sept. 22, 2015, 3 nachm., Stephen Gallagher wrote:
config/roles/databaseserver/role.py, line 310 http://reviewboard-fedoraserver.rhcloud.com/r/219/diff/2/?file=1121#file1121line310
Can you add a comment about linkfile() here? Is this creating a hard-link? (And if so, is it also going to be overwritten by overwrite_safely()?)
Can you add a comment about linkfile() here? Is this creating a hard-link?
The `linkfile()` function is documented:
slip.util.files.linkfile = linkfile(srcpath, dstpath) Hardlink srcpath to dstpath.
Attempt to atomically replace dstpath if it exists.
I think documentation about a function should be with the function, not where it's used.
(And if so, is it also going to be overwritten by overwrite_safely()?)
It's not a function to "scrub" a file containing secret data, if you mean that. It's just something to overwrite a file safely with new content:
slip.util.files.overwrite_safely = overwrite_safely(path, content, preserve_mode=True, preserve_context=True, preserve_ownership=True) Safely overwrite a file by creating a temporary file in the same directory, writing it, moving it over the original file, eventually preserving file mode, SELinux context and ownership.
- Nils
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/219/#review535 -----------------------------------------------------------
On Sept. 19, 2015, 3:04 vorm., Nils Philippsen wrote:
This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/219/
(Updated Sept. 19, 2015, 3:04 vorm.)
Review request for RoleKit Mailing List, Miloslav Trmac, Nils Philippsen, Stephen Gallagher, and Thomas Woerner.
Repository: rolekit
Description
Use an internal implementation instead of calling sed to tweak configuration files. This has the neat side effect of overwriting the target file as the last step, side-stepping the need to copy over the backup file on errors.
This change requires python3-slip >= 0.6.4 because in previous versions slip.util.files.overwrite_safely() doesn't preserve file ownership, and the postgresql configuration files need to be owned by the postgres user.
https://github.com/libre-server/rolekit/issues/21
Diffs
config/roles/databaseserver/role.py 7443979ba7ff87ff6a018ac8a7a0a89e2b8ad6e7 rolekit.spec f3cf9f4b799909bc293f4e56eea78c71e7112e9c
Diff: http://reviewboard-fedoraserver.rhcloud.com/r/219/diff/
Testing
Deployed `databaseserver` role, compared contents of `postgresql.conf`, `pg_hba.conf` with what the original sed commands produced.
Thanks,
Nils Philippsen