[node-patches] Change in ovirt-node[master]: puppet: add disable_puppet function into puppet_page

[rbarry at redhat.com]

Ryan Barry has posted comments on this change.

Change subject: puppet: add disable_puppet function into puppet_page
......................................................................


Patch Set 4: Code-Review-1

(4 comments)

....................................................
File src/ovirt/node/setup/puppet/puppet_page.py
Line 128:         (valid.Empty() | valid.FQDNOrIPAddress())(server)
Line 129:         (valid.Empty() | valid.URL())(certname)
Line 130:         return {"OVIRT_PUPPET_ENABLED": "yes" if enabled else None}
Line 131: 
Line 132: 
Why are these moved out instead of being ActivatePuppet.__enable_puppet() or ActivatePuppet.__disable_puppet() ?

Keeping logic related to it in the page has the potential to create autoinstall problems if we don't want to import the entire page (and all of its requisite imports) just for the autoinstaller.
Line 133: def enable_puppet():
Line 134:     cfg = Puppet().retrieve()
Line 135: 
Line 136:     conf = File("/etc/puppet/puppet.conf")


Line 157:                              shell=True)
Line 158:     system.service("puppet", "start")
Line 159: 
Line 160: 
Line 161: def disable_puppet():
This is a lot of code duplication.
Line 162:     item_args = ["server", "certname"]
Line 163: 
Line 164:     conf = File("/etc/puppet/puppet.conf")
Line 165:     conf_builder = ""


Line 162:     item_args = ["server", "certname"]
Line 163: 
Line 164:     conf = File("/etc/puppet/puppet.conf")
Line 165:     conf_builder = ""
Line 166:     for line in conf:
for item in item_args:    
        line = re.sub(r'(^\s+\w+ =).*', r'#\1', line) if item in line else line
        conf_builder += line

This lets you scrap the duplicated code.
Line 167:         try:
Line 168:             item = re.match(r'^\s+(\w+) =', line).group(1)
Line 169:             if item in item_args:
Line 170:                 conf_builder += re.sub(r'(^.*?' + item + ' =).*',


Line 177: 
Line 178:     conf.write(conf_builder, "w")
Line 179:     fs.Config().persist("/etc/puppet/puppet.conf")
Line 180: 
Line 181:     system.service("puppet", "stop")
This is probably the only line which is ultimately necessary, though resetting puppet.conf is fine. 

The regex to enable Puppet is written in such a way that we don't need to reset puppet.conf in order to reconfigure/reenable it.
Line 182: 
Line 183: 
Line 184: class ActivatePuppet(utils.Transaction.Element):
Line 185: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I942252e228e9475c4c41e893aae6507b9505e245
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-node
Gerrit-Branch: master
Gerrit-Owner: hadong <hadong0720 at gmail.com>
Gerrit-Reviewer: Ryan Barry <rbarry at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes