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

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.


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.


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.


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()?)


- Stephen Gallagher


On September 19th, 2015, 1:04 a.m. UTC, 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, 1:04 a.m.

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