Alon Bar-Lev has posted comments on this change.
Change subject: ovirt-live: migrate ovirt live plugin to otopi
......................................................................
Patch Set 2: (26 inline comments)
....................................................
File fedora/kickstart/ovirt.ks
Line 129
Line 130
Line 131
Line 132
Line 133
why are these not in root?
Line 141: sed -i 's/\#WDMDOPTS/WDMDOPTS/g' /etc/sysconfig/wdmd
Line 142: yum localinstall -y /home/oVirtuser/oVirtLiveFiles/rpms/*.rpm
Line 143:
Line 144: # Updating patched files
Line 145: cp -r /home/oVirtuser/root/* /
good.
To work with non standard umask and user that checks out you need at least:
tmproot="$(mktemp -d)"
cp -r ~oVirtuser/root/* "${tmproot}"
chown -R root "${tmproot}"
chmod -R a+rX "${tmproot}"
find "${tmproot}" -type f -executable | while read f; do chmod a+x "$f"; done
or better to create tarball with right permissions and extract it using -p.
Line 146:
Line 147:
Line 148: # Manipulate fqdn validation, so that it is possible to setup with
answer file
Line 149: sed -i 's/raise Exception(output_messages.ERR_EXP_VALIDATE_PARAM %
param.getKey("CONF_NAME"))/logging.debug("Failed to validate %s with value
%s",param,paramValue)/g' /usr/share/ovirt-engine/scripts/engine-setup.py
Line 141
Line 142
Line 143
Line 144
Line 145
ln -s?
Line 154
Line 155
Line 156
Line 157
Line 158
why do you need the '\'?
....................................................
File
fedora/oVirtLiveFiles/root/usr/share/ovirt-engine/setup/plugins/ovirt-engine-setup/olive/core.py
Line 31: from otopi import plugin
Line 32: from otopi import filetransaction
Line 33: from otopi import constants as otopicons
Line 34:
Line 35: import oliveconst
after ovirt_engine_setup section, please add two spaces
Line 36:
Line 37: from ovirt_engine_setup import constants as osetupcons
Line 38: from ovirt_engine_setup import dialog
Line 39:
Line 36:
Line 37: from ovirt_engine_setup import constants as osetupcons
Line 38: from ovirt_engine_setup import dialog
Line 39:
Line 40:
two spaces, please run pep8
Line 41:
Line 42: @util.export
Line 43: class Plugin(plugin.PluginBase):
Line 44: """
Line 70: @plugin.event(
Line 71: stage=plugin.Stages.CLOSEUP,
Line 72: condition=lambda self: self._enabled,
Line 73: name=oliveconst.Stages.OL_CONFIG_STORAGE,
Line 74: before=[
please convert to tupple:
before=(
oliveconst.Stages.OL_COPY_ISO,
),
Line 75: oliveconst.Stages.OL_COPY_ISO
Line 76: ],
Line 77: after=[osetupcons.Stages.AIO_CONFIG_VDSM]
Line 78: )
Line 73: name=oliveconst.Stages.OL_CONFIG_STORAGE,
Line 74: before=[
Line 75: oliveconst.Stages.OL_COPY_ISO
Line 76: ],
Line 77: after=[osetupcons.Stages.AIO_CONFIG_VDSM]
please add comma at end, please convert tupple, please use the above syntax of:
aaa=(
xxx,
yyy,
),
Line 78: )
Line 79: def _createstorage(self):
Line 80: import ovirtsdk.api
Line 81: from ovirtsdk.xml import params
Line 84: self._ovirtsdk_xml = ovirtsdk.xml
Line 85: engine_api = self._ovirtsdk_api.API(
Line 86: url='https://{fqdn}:{port}/api'.format(
Line 87: fqdn=self.environment[oliveconst.ConfigEnv.FQDN],
Line 88: port=self.environment[oliveconst.ConfigEnv.HTTPS_PORT],
PUBLIC_HTTPS_PORT
Line 89: ),
Line 90: username='{user}@{domain}'.format(
Line 91: user=oliveconst.Const.USER_ADMIN,
Line 92: domain=oliveconst.Const.DOMAIN_INTERNAL,
Line 100: )
Line 101: self.logger.debug('Creating the local data storage domain')
Line 102:
Line 103: stParams = params.Storage(path='/localdata')
Line 104: stParams.set_type('localfs')
can't the type be in Storage constructor?
Line 105:
Line 106: sdParams = params.StorageDomain(name='local_storage',
Line 107:
data_center=engine_api.datacenters.get(self.environment[oliveconst.ConfigEnv.DEFAULT_LOCAL_DATA_CENTER]),
Line 108: storage_format='v3',
Line 107:
data_center=engine_api.datacenters.get(self.environment[oliveconst.ConfigEnv.DEFAULT_LOCAL_DATA_CENTER]),
Line 108: storage_format='v3',
Line 109:
host=engine_api.hosts.get('local_host'),
Line 110: storage=stParams,
Line 111: type='data')
please use one indent:
xxxx(
name='aaa',
dddd='ddd,
)
Line 112:
Line 113: engine_api.storagedomains.add(sdParams)
Line 114:
Line 115:
engine_api.datacenters.get(self.environment[oliveconst.ConfigEnv.DEFAULT_LOCAL_DATA_CENTER]).storagedomains.add(engine_api.self.environment[oliveconst.ConfigEnv.DEFAULT_ISO_NAME])
Line 111: type='data')
Line 112:
Line 113: engine_api.storagedomains.add(sdParams)
Line 114:
Line 115:
engine_api.datacenters.get(self.environment[oliveconst.ConfigEnv.DEFAULT_LOCAL_DATA_CENTER]).storagedomains.add(engine_api.self.environment[oliveconst.ConfigEnv.DEFAULT_ISO_NAME])
please make sure pep8 is passing.
xxx.get(
bla
).storagedomains.add(
bla
)
Line 116:
Line 117: @plugin.event(
Line 118: stage=plugin.Stages.CLOSEUP,
Line 119: condition=lambda self: self._enabled,
Line 117: @plugin.event(
Line 118: stage=plugin.Stages.CLOSEUP,
Line 119: condition=lambda self: self._enabled,
Line 120: name=oliveconst.Stages.OL_COPY_ISO,
Line 121: before=[
tuple
Line 122: oliveconst.Stages.OL_CREATE_VM,
Line 123: ],
Line 124: )
Line 125:
Line 125:
Line 126: def _copyiso(self):
Line 127: self.logger.debug('Copying Iso Files')
Line 128: isoPattern = "/home/oVirtuser/oVirtLiveFiles/iso/*.iso"
Line 129: fileList = glob.glob(isoPattern)
isoPattern used once thus can be eliminated
Line 130: targetPath =
os.path.join(self.environment[osetupcons.ConfigEnv.ISO_DOMAIN_DEFAULT_NFS_MOUNT_POINT],
self.environment[osetupcons.ConfigEnv.ISO_DOMAIN_SD_UUID], "images",
"11111111-1111-1111-1111-111111111111")
Line 131: for filename in fileList:
Line 132: shutil.move(filename, targetPath)
Line 133: file = os.path.join(targetPath,filename)
Line 126: def _copyiso(self):
Line 127: self.logger.debug('Copying Iso Files')
Line 128: isoPattern = "/home/oVirtuser/oVirtLiveFiles/iso/*.iso"
Line 129: fileList = glob.glob(isoPattern)
Line 130: targetPath =
os.path.join(self.environment[osetupcons.ConfigEnv.ISO_DOMAIN_DEFAULT_NFS_MOUNT_POINT],
self.environment[osetupcons.ConfigEnv.ISO_DOMAIN_SD_UUID], "images",
"11111111-1111-1111-1111-111111111111")
pep8
Line 131: for filename in fileList:
Line 132: shutil.move(filename, targetPath)
Line 133: file = os.path.join(targetPath,filename)
Line 134: os.chown(file,36,36)
Line 129: fileList = glob.glob(isoPattern)
Line 130: targetPath =
os.path.join(self.environment[osetupcons.ConfigEnv.ISO_DOMAIN_DEFAULT_NFS_MOUNT_POINT],
self.environment[osetupcons.ConfigEnv.ISO_DOMAIN_SD_UUID], "images",
"11111111-1111-1111-1111-111111111111")
Line 131: for filename in fileList:
Line 132: shutil.move(filename, targetPath)
Line 133: file = os.path.join(targetPath,filename)
pep8 space after comma
Line 134: os.chown(file,36,36)
Line 135:
Line 136: @plugin.event(
Line 137: stage=plugin.Stages.CLOSEUP,
Line 130: targetPath =
os.path.join(self.environment[osetupcons.ConfigEnv.ISO_DOMAIN_DEFAULT_NFS_MOUNT_POINT],
self.environment[osetupcons.ConfigEnv.ISO_DOMAIN_SD_UUID], "images",
"11111111-1111-1111-1111-111111111111")
Line 131: for filename in fileList:
Line 132: shutil.move(filename, targetPath)
Line 133: file = os.path.join(targetPath,filename)
Line 134: os.chown(file,36,36)
same, and what is 36? please get ovirt user from environment and use utils to
resolve it.
Line 135:
Line 136: @plugin.event(
Line 137: stage=plugin.Stages.CLOSEUP,
Line 138: condition=lambda self: self._enabled,
Line 136: @plugin.event(
Line 137: stage=plugin.Stages.CLOSEUP,
Line 138: condition=lambda self: self._enabled,
Line 139: name=oliveconst.Stages.OL_CREATE_VM,
Line 140: after=[
tuple
Line 141: oliveconst.Stages.OL_COPY_ISO,
Line 142: ],
Line 143: )
Line 144:
Line 148:
Line 149: engine_api = self._ovirtsdk_api.API(
Line 150: url='https://{fqdn}:{port}/api'.format(
Line 151: fqdn=self.environment[osetupcons.ConfigEnv.FQDN],
Line 152:
port=self.environment[osetupcons.ConfigEnv.DEFAULT_NETWORK_HTTPS_PORT],
why default?
Line 153: ),
Line 154: username='{user}@{domain}'.format(
Line 155: user=osetupcons.Const.USER_ADMIN,
Line 156: domain=osetupcons.Const.DOMAIN_INTERNAL,
Line 164: )
Line 165:
Line 166:
Line 167: # Defins OS param for the boot option
Line 168: os=params.OperatingSystem(type_='unassigned',
boot=[params.Boot(dev='cdrom'), params.Boot(dev='hd')])
pep8
Line 169:
Line 170: # Create VM
Line 171: engine_api.vms.add(params.VM(name='local_vm',
memory='1024MB', os=os, cluster=engine_api.clusters.get('local_cluster'),
template=engine_api.templates.get('Blank')))
Line 172:
Line 173: # Create NIC
Line 174:
engine_api.vms.get('local_vm').nics.add(params.NIC(name='eth0',
network=params.Network(name='ovirtmgmt'), interface='virtio'))
Line 175:
Line 176: sd = engine_api.storagedomains.get('local_storage')
Line 177: diskParam =
params.Disk(storage_domains=params.StorageDomains(storage_domain=[sd]),
align one indent
Line 178: size='6000MB',
Line 179: type_='data',
Line 180: interface='virtio',
Line 181: format='cow',
....................................................
File
fedora/oVirtLiveFiles/root/usr/share/ovirt-engine/setup/plugins/ovirt-engine-setup/olive/__init__.py
Line 28:
Line 29: @util.export
Line 30: def createPlugins(context):
Line 31: core.Plugin(context=context)
Line 32:
two spaces
....................................................
File
fedora/oVirtLiveFiles/root/usr/share/ovirt-engine/setup/plugins/ovirt-engine-setup/olive/oliveconst.py
Line 1: #!/bin/python
you ignored my comments... please rename OL to OVIRTLIVE or OLIVE. OL prefix
means nothing.
you can install this constants in ovirt-engine-setup, you don't have to use the
extra directory. But it doe not heart.
Line 2:
Line 3: #OVIRT LIVE
Line 4:
Line 5: @util.export
Line 15: class OLEnv(object):
Line 16: ENABLE = 'OVESETUP_OL/enable'
Line 17:
Line 18: def CONFIGURE(self):
Line 19: return 'OVESETUP_OL/configure'
no need...
CONFIGURE = 'OVESETUP_OL/configure'
unless you want to ask if user wants live configuration. and probably you want
to ask... as the fact that the plugin is install should not automatically
activate it.
Line 20:
Line 21: @osetupattrs(
Line 22: answerfile=True,
Line 23: summary=False,
Line 22: answerfile=True,
Line 23: summary=False,
Line 24: )
Line 25: def ROOT_PASSWORD(self):
Line 26: return 'OVESETUP_AIO/rootPassword'
why do you need root password?
Line 27:
Line 28: LOCAL_DATA_CENTER = 'OVESETUP_AIO/localDataCenter'
Line 29: LOCAL_CLUSTER = 'OVESETUP_AIO/localCluster'
Line 30: LOCAL_HOST = 'OVESETUP_AIO/localHost'
Line 57: @util.codegen
Line 58: class OLConst(object):
Line 59: MINIMUM_SPACE_STORAGEDOMAIN_MB = 10240
Line 60:
Line 61:
missing vim mode line
--
To view, visit http://gerrit.ovirt.org/17518
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I703f64dc1183a6fe176d9d0352f93de381d906bb
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-live
Gerrit-Branch: master
Gerrit-Owner: Ohad Basan <oba...@redhat.com>
Gerrit-Reviewer: Alex Lourie <alou...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Eyal Edri <ee...@redhat.com>
Gerrit-Reviewer: Itamar Heim <ih...@redhat.com>
Gerrit-Reviewer: Moran Goldboim <mgold...@redhat.com>
Gerrit-Reviewer: Sandro Bonazzola <sbona...@redhat.com>
Gerrit-Reviewer: Yedidyah Bar David <d...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
