[node-patches] Change in ovirt-node[master]: fix snmp to work

[dougsland at redhat.com]

Douglas Schilling Landgraf has posted comments on this change.

Change subject: fix snmp to work
......................................................................


Patch Set 3: Code-Review-1

(4 comments)

-1 since I have comments.

http://gerrit.ovirt.org/#/c/32167/3/src/ovirt/node/setup/snmp/snmp_model.py
File src/ovirt/node/setup/snmp/snmp_model.py:

Line 56:     fs.Config().persist("/etc/snmp/snmpd.conf")
Line 57: 
Line 58:     process.check_call("cat /dev/null >%s" % snmp_conf,
Line 59:                        shell=True)
Line 60:     f = open(snmp_conf, "a")
Hi hadong, thanks for the patch. I have question, isn't better to use more pythonic approach for this cat /dev/null and open?

Here example:
with open('/tmp/myfile', 'w') as f:
    f.write("writing inside file..")

So, no need to cat /dev/null to empty the file and no need to call .close() later.
Line 61:     # create user account
Line 62:     f.write("createUser root SHA %s AES\n" % password)
Line 63:     f.close()
Line 64:     system.service("snmpd", "start")


Line 64:     system.service("snmpd", "start")
Line 65:     fs.Config().persist(snmp_conf)
Line 66: 
Line 67:     if firewall.is_firewalld():
Line 68:         firewall.setup_firewalld("161", "udp")
do you mind to use:

firewall.setup_firewald(port="161", proto="udp") ?
Line 69:     else:
Line 70:         firewall.setup_iptables("161", "udp")
Line 71: 
Line 72: 


Line 66: 
Line 67:     if firewall.is_firewalld():
Line 68:         firewall.setup_firewalld("161", "udp")
Line 69:     else:
Line 70:         firewall.setup_iptables("161", "udp")
I would suggest the same here:
firewall.setup_iptables(port="161", proto="udp")
Line 71: 
Line 72: 
Line 73: def disable_snmpd():
Line 74:     system.service("snmpd", "stop")


Line 74:     system.service("snmpd", "stop")
Line 75:     # copy to /tmp for enable/disable toggles w/o reboot
Line 76:     process.check_call(["cp", "/etc/snmp/snmpd.conf", "/tmp"])
Line 77:     process.check_call("cat /dev/null >%s" % snmp_conf,
Line 78:                        shell=True)
Well, here I would suggest a different approach for such function to comment all conf files using a sed and do not unpersist the file. Maybe the others guys have different opinion.
sed -i 's/^/#/g' /etc/path/to/snmp_conf_file
Line 79:     fs.Config().unpersist(snmp_conf)
Line 80: 
Line 81: 
Line 82: class SNMP(NodeConfigFileSection):


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idc32bd53cffd2ef93a60a06f8937a9a9ff4539a5
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-node
Gerrit-Branch: master
Gerrit-Owner: hadong <hadong0720 at gmail.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes