Yedidyah Bar David has posted comments on this change.
Change subject: packaging: setup: revert nfs config on cleanup
......................................................................
Patch Set 3:
(5 comments)
Items marked "Done" are changed in my private repo, but I will push another
changeset only after I finish making it actually work (which it still does not).
....................................................
File packaging/setup/ovirt_engine_setup/util.py
Line 44: ^
Line 45: \s*
Line 46: (?P<param>\w+)
Line 47: \s*
Line 48: =
Do you ask just to be a bit more general? Or to immediately use elsewhere?
I now grepped for 'content=', and the following places seem to be able to be
adapted to use this function too:
./setup/plugins/ovirt-engine-setup/apache/ssl.py
./setup/plugins/ovirt-engine-setup/provisioning/postgres.py
./setup/plugins/ovirt-engine-setup/system/exportfs.py
And perhaps even:
./setup/plugins/ovirt-engine-setup/all-in-one/sshd.py
Do we want to? If so, we should make it general enough at least for these cases.
Adding a separator and quotes are probably required, but not enough. E.e. in
httpd conf files, we want to keep the indentation (first file above).
Line 49: \s*
Line 50: .*
Line 51: $
Line 52: """
Line 51: $
Line 52: """
Line 53: )
Line 54:
Line 55:
Done
Line 56: def editConfigContent(content, params, changed_lines=[]):
Line 57: newcontent = []
Line 58: for line in content:
Line 59: f = _RE_PARAM.match(line)
Line 52: """
Line 53: )
Line 54:
Line 55:
Line 56: def editConfigContent(content, params, changed_lines=[]):
You refer to changed_lines? It's an output parameter. If caller passes a
non-empty list, it's appended to. Do you see a problem with that? E.g.
changed_lines = []
content = editConfigContent(content, first_set_of_params, changed_lines)
content = editConfigContent(content, second_set_of_params, changed_lines)
Line 57: newcontent = []
Line 58: for line in content:
Line 59: f = _RE_PARAM.match(line)
Line 60: if f is not None and f.group('param') in params:
Line 53: )
Line 54:
Line 55:
Line 56: def editConfigContent(content, params, changed_lines=[]):
Line 57: newcontent = []
I do not think it makes it clearer what this function does. I agree that my
name is also not very good, but can't either find a really good one...
Line 58: for line in content:
Line 59: f = _RE_PARAM.match(line)
Line 60: if f is not None and f.group('param') in params:
Line 61: newline = '{param}={value}'.format(
Line 68: 'removed': line,
Line 69: }
Line 70: )
Line 71: line = newline
Line 72: del(params[f.group('param')])
Done
Line 73: newcontent.append(line)
Line 74: # Add remaining params at the end
Line 75: for name, value in params.items():
Line 76: newline = '{param}={value}'.format(
--
To view, visit http://gerrit.ovirt.org/19001
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I74dd4c1556bd6479fcf10f85fcbe083f215e0854
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Sandro Bonazzola <sbona...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Kiril Nesenko <knese...@redhat.com>
Gerrit-Reviewer: Ofer Schreiber <oschr...@redhat.com>
Gerrit-Reviewer: Ohad Basan <oba...@redhat.com>
Gerrit-Reviewer: Sandro Bonazzola <sbona...@redhat.com>
Gerrit-Reviewer: Yedidyah Bar David <d...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
