[node-patches] Change in ovirt-node[master]: Fix clear /etc/puppet/puppet.conf issue

[fabiand at fedoraproject.org]

Fabian Deutsch has posted comments on this change.

Change subject: Fix clear /etc/puppet/puppet.conf issue
......................................................................


Patch Set 2: Code-Review-1

(2 comments)

In general ok, just some style nitpicks.

....................................................
File src/ovirt/node/setup/puppet/puppet_page.py
Line 142: 
Line 143:         lines = File("/etc/puppet/puppet.conf").read().split("\n")
Line 144:         conf = File("/etc/puppet/puppet.conf")
Line 145:         conf.write("")
Line 146:         for line in lines:
Please use:

    for line in File(...):

that will use a generator - which uses less mem.
Line 147:             try:
Line 148:                 item = re.match(r'^\s+(\w+) =', line).group(1)
Line 149:                 if item in cfg:
Line 150:                     conf.write("\n" + re.sub(r'(^.*?' + item + ' =).*', r'\1 "'


Line 146:         for line in lines:
Line 147:             try:
Line 148:                 item = re.match(r'^\s+(\w+) =', line).group(1)
Line 149:                 if item in cfg:
Line 150:                     conf.write("\n" + re.sub(r'(^.*?' + item + ' =).*', r'\1 "'
Please make this function call more clear by introducing one or two carefully named variables
Line 151:                                              + cfg[item] + '"', line), "a")
Line 152:                 else:
Line 153:                     conf.write("\n" + line, "a")
Line 154:             except:


-- 
To view, visit http://gerrit.ovirt.org/17522
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idffce430477f9d44d2a8bff12ad518b479e86648
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-node
Gerrit-Branch: master
Gerrit-Owner: hadong <hadong0720 at gmail.com>
Gerrit-Reviewer: Fabian Deutsch <fabiand at fedoraproject.org>
Gerrit-Reviewer: Ryan Barry <rbarry at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes