This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/219/

On September 22nd, 2015, 3 nachm. CEST, Stephen Gallagher wrote:

config/roles/databaseserver/role.py (Diff revision 2)
51
def tweak_lines(lines_iterable, tweaking_rules, append_if_missing=False):
52
    for rule in tweaking_rules:
53
        regex = rule['regex']
54
        if isinstance(regex, str):
55
            rule['regex'] = re.compile(regex)
56
        rule.setdefault('global', True)
57
        rule.setdefault('append_if_missing', append_if_missing)
58
59
    found_regexes = set()
60
    ignore_regexes = set()
61
62
    for line in lines_iterable:
63
        lines_to_append = []
64
        for rule in tweaking_rules:
65
            regex = rule['regex']
66
            if regex not in ignore_regexes:
67
                m = regex.search(line)
68
                if m:
69
                    found_regexes.add(regex)
70
                    if not rule['global']:
71
                        ignore_regexes.add(regex)
72
                    if 'replace' in rule:
73
                        line = regex.sub(rule['replace'], line)
74
                    if 'append' in rule:
75
                        lines_to_append.append(rule['append'])
76
        yield line
77
        for l in lines_to_append:
78
            yield l + "\n"
79
80
    for rule in tweaking_rules:
81
        if (
82
                rule['append_if_missing'] and
83
                rule['regex'] not in found_regexes):
84
            if 'replace' in rule:
85
                yield rule['replace'] + "\n"
86
            if 'append' in rule:
87
                yield rule['append'] + "\n"
88
89

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 September 22nd, 2015, 3 nachm. CEST, Stephen Gallagher wrote:

config/roles/databaseserver/role.py (Diff revision 2)
66
            if regex not in ignore_regexes:
67
                m = regex.search(line)
68
                if m:
69
                    found_regexes.add(regex)
70
                    if not rule['global']:
71
                        ignore_regexes.add(regex)

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 September 22nd, 2015, 3 nachm. CEST, Stephen Gallagher wrote:

config/roles/databaseserver/role.py (Diff revision 2)
81
        if (
82
                rule['append_if_missing'] and
83
                rule['regex'] not in found_regexes):

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 September 22nd, 2015, 3 nachm. CEST, Stephen Gallagher wrote:

config/roles/databaseserver/role.py (Diff revision 2)
305
                linkfile(conffile, bakfile)

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


On September 19th, 2015, 3:04 vorm. CEST, Nils Philippsen wrote:

Review request for RoleKit Mailing List, Miloslav Trmac, Nils Philippsen, Stephen Gallagher, and Thomas Woerner.
By Nils Philippsen.

Updated Sept. 19, 2015, 3:04 vorm.

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

Testing

Deployed databaseserver role, compared contents of postgresql.conf, pg_hba.conf with what the original sed commands produced.

Diffs

  • config/roles/databaseserver/role.py (7443979ba7ff87ff6a018ac8a7a0a89e2b8ad6e7)
  • rolekit.spec (f3cf9f4b799909bc293f4e56eea78c71e7112e9c)

View Diff