Ofer Schreiber has posted comments on this change.
Change subject: packaging: add the all-in-one plugin - DO NOT MERGE
......................................................................
Patch Set 7: Fails
(21 inline comments)
1. Missing cosnsts
2. Add host should use real host name
....................................................
Commit Message
Line 7: packaging: add the all-in-one plugin - DO NOT MERGE
Please remove the DO NOT MERGE
....................................................
File packaging/fedora/setup/plugins/all_in_one_100.py
Line 96: aioSteps = [ { 'title' : "Creating storage directory",
Just an idea - why not prefix plugin titles with "AIO:"
Line 103: # This is waiting for the resolution of #799111, which is lock on
logging file
Please add #FixME
Line 113: time.sleep(25)
Const?
Line 118: URL = "https://127.0.0.1:%s/api"; % controller.CONF["HTTPS_PORT"]
api path - const?
Line 127: if not
controller.CONF["API_OBJECT"].datacenters.add(params.DataCenter(name='local_datacenter',
const? (local_datacenter? maybe consult with some QE guys)
Line 131: raise Exception("Could not create the local datacenter,
please consult the logs for details")
No need to mention logs here.
iirc the setup already refers the user to logs
Line 137: if not
controller.CONF["API_OBJECT"].clusters.add(params.Cluster(name='local_cluster',
const? better name?
Line 139:
data_center=controller.CONF["API_OBJECT"].datacenters.get('local_datacenter'),
another good reason to use const
Line 140:
version=params.Version(major='3', minor='0'))):
3.0? why?
Line 147: if not
controller.CONF["API_OBJECT"].hosts.add(params.Host(name='local_host',
const
Line 148:
address='127.0.0.1',
We should use the real hostname (we already have it)
Line 150:
cluster=controller.CONF["API_OBJECT"].clusters.get('local_cluster'),
const?
Line 152: logging.error("Could not create local host")
could not install?
Line 158: vdsmService.start()
do we really need it?
Line 163: timeout = 33 # (5.5 (minutes) * 60 )/ 10, since we sleep 10
seconds after each iteration
onst?
Line 177: logging.error("Host was found in Failed state. Please check
log file for details.")
Better be "Host installation failed, please check engine and bootstrap logs" or
somethig similar
Line 205: with open('/etc/shadow', "r") as f:
const
Line 225: nfsutils.setSELinuxContextForDir(controller.CONF["STORAGE_PATH"],
nfsutils.SELINUX_RW_LABEL)
what will happen if selinux is disabled?
Line 234: sys.path.append('/usr/share/vdsm')
const?
....................................................
File packaging/fedora/setup/plugins/example_plugin_000.py
Line 10: #import engine_validators as validate
why did you put this as comments?
--
To view, visit http://gerrit.ovirt.org/2221
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I13d29c3642862d99abe39c1b9458cdb23eca30dd
Gerrit-PatchSet: 7
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ronen Angluster <rangl...@redhat.com>
Gerrit-Reviewer: Alex Lourie <alou...@redhat.com>
Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com>
Gerrit-Reviewer: Eyal Edri <ee...@redhat.com>
Gerrit-Reviewer: Michael Burns <mbu...@redhat.com>
Gerrit-Reviewer: Ofer Schreiber <oschr...@redhat.com>
Gerrit-Reviewer: Ronen Angluster <rangl...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
