Yedidyah Bar David has posted comments on this change.

Change subject: packaging: setup: firewall configuration
......................................................................


Patch Set 1: (2 inline comments)

That's only a partial review as I still do not understand many parts of otopi - 
file transactions etc.

....................................................
File src/plugins/ovirt-hosted-engine-setup/network/firewall_manager.py
Line 149:                 if response == _('yes'):
Line 150:                     self.environment[
Line 151:                         ohostedcons.NetworkEnv.FIREWALL_MANAGER
Line 152:                     ] = manager
Line 153:                     break
why loop and ask about all of them? I'd prefer to show the user all of the 
options and let them choose in one question. At the beginning of the loop I 
thought that you let them configure more than one, which might make sense, but 
here you break. Even if you let them configure more than one, I'd first show 
all of them.
Line 154: 
Line 155:         self.environment[otopicons.NetEnv.IPTABLES_ENABLE] = (
Line 156:             self.environment[
Line 157:                 ohostedcons.NetworkEnv.FIREWALL_MANAGER


....................................................
File templates/hosted-console.xml.in
Line 2: <service>
Line 3:     <short>hosted-console</short>
Line 4:     <description>oVirt Hosted Engine console service</description>
Line 5:     <port protocol="tcp" port="5900"/>
Line 6:     <port protocol="udp" port="5900"/>
Is 5900 enough? We have a range of possible ports. I understand that this will 
currently be the first vm and so will get the first port, not sure we should 
not open a range, e.g. to prepare for HA when a vm can migrate to another host.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6c3ac2ae97416f4a9b82f9a7c404350345a0c765
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-hosted-engine-setup
Gerrit-Branch: master
Gerrit-Owner: Sandro Bonazzola <[email protected]>
Gerrit-Reviewer: Alex Lourie <[email protected]>
Gerrit-Reviewer: Lev Veyde <[email protected]>
Gerrit-Reviewer: Moran Goldboim <[email protected]>
Gerrit-Reviewer: Ofer Schreiber <[email protected]>
Gerrit-Reviewer: Sandro Bonazzola <[email protected]>
Gerrit-Reviewer: Yedidyah Bar David <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to