[node-patches] Change in ovirt-node[master]: snmp auto：fix snmp page still shown disabled issue

[fabiand at fedoraproject.org]

Fabian Deutsch has posted comments on this change.

Change subject: snmp auto：fix snmp page still shown disabled issue
......................................................................


Patch Set 11: Code-Review-1

(3 comments)

Hey,

you addressed the previous comments, but also added so many new changes that i can not merge it.

Please pull out i.e. the kdump changes into a separate patch. Also please remove the unrelated RHN changes.

Also see my inline comments. :)

http://gerrit.ovirt.org/#/c/22727/11/scripts/ovirt-auto-install.py
File scripts/ovirt-auto-install.py:

Line 161: class ConfigureKdump(Transaction.Element):
Line 162:     title = "Configuring KDump"
Line 163: 
Line 164:     def commit(self):
Line 165:         try:
why do you change kdump stuff?
Line 166:             model = defaults.KDump()
Line 167: 
Line 168:             if "OVIRT_KDUMP_SSH" in OVIRT_VARS and \
Line 169:                     "OVIRT_KDUMP_SSH_KEY" in OVIRT_VARS:


http://gerrit.ovirt.org/#/c/22727/11/src/Makefile.am
File src/Makefile.am:

Line 202:   ovirt/node/setup/puppet/puppet_page.py
Line 203: 
Line 204: 
Line 205: # Setup RHN Plugin
Line 206: pyovirt_node_setup_rhn_PYTHON = \
Why is the rhn stuff in here?

Where is the hooks.py file?
Line 207:    ovirt/node/setup/rhn/__init__.py \
Line 208:    ovirt/node/setup/rhn/rhn_model.py \
Line 209:    ovirt/node/setup/rhn/rhn_page.py
Line 210: 


http://gerrit.ovirt.org/#/c/22727/11/src/ovirtnode/ovirtfunctions.py
File src/ovirtnode/ovirtfunctions.py:

Line 1200:             hookscript = os.path.join(hookdir, hook)
Line 1201:             if hook.endswith(".py"):
Line 1202:                 pass
Line 1203:             else:
Line 1204:                 system_closefds(hookscript + " &> /dev/null")
You should be using the hook class you created elsewhere.

That prevents code duplication. And introduces a single point of responsibility.
Line 1205:     for f in ["/etc/ssh/ssh_host%s_key" % t for t in ["", "_dsa", "_rsa"]]:
Line 1206:         ovirt_store_config(f)
Line 1207:         ovirt_store_config("%s.pub" % f)
Line 1208:     for f in OVIRT_CONFIG_FILES:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I113f9835d9ac76deaab50bd9e8dfcb034328e441
Gerrit-PatchSet: 11
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: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes