Change in vdsm[master]: Adding Vdsm cron job check for available packages update

danken at redhat.com danken at redhat.com
Mon Mar 30 12:58:26 UTC 2015


Dan Kenigsberg has posted comments on this change.

Change subject: Adding Vdsm cron job check for available packages update
......................................................................


Patch Set 10: Code-Review-1

(8 comments)

https://gerrit.ovirt.org/#/c/38401/10/vdsm/Makefile.am
File vdsm/Makefile.am:

Line 73: dist_vdsm_SCRIPTS = \
Line 74: 	addNetwork \
Line 75: 	delNetwork \
Line 76: 	get-conf-item \
Line 77: 	updater-tool \
the name is very generic; yet what it does is just poll for updates. The problem goes away if the script moves to vdsm-tool.
Line 78: 	set-conf-item \
Line 79: 	supervdsmServer \
Line 80: 	vdsm \
Line 81: 	vdsm-restore-net-config \


https://gerrit.ovirt.org/#/c/38401/10/vdsm/updater-tool
File vdsm/updater-tool:

Line 22: import sys
Line 23: import syslog
Line 24: import stat
Line 25: import os
Line 26: import argparse
please sort alphabetically
Line 27: import rpm
Line 28: import itertools
Line 29: import json
Line 30: 


Line 30: 
Line 31: 
Line 32: try:
Line 33:     import yum
Line 34:     yumSupport = True
please use pep8-comliant variable names (and method names, too!) in new modules.
Line 35: except ImportError:
Line 36:     yumSupport = False
Line 37: 
Line 38: 


Line 58:     def __init__(self, pkgProvider):
Line 59:         self.provider = pkgProvider()
Line 60:         # ups holds dict of name, version, prev, repo for each
Line 61:         # available update
Line 62:         self.ups = None
"ups" is a short for "updates"? can we use the full and clearer name?
Line 63: 
Line 64:     def refreshUpdates(self):
Line 65:         self._cleanCache()
Line 66:         return self.getCacheUpdates()


Line 84:             ups = self.update()
Line 85:             for trns in ups:
Line 86:                 if pkgs is None or trns.name in pkgs:
Line 87:                     pkgInfo = {}
Line 88:                     pkgInfo['fullname'] = "%s.%s %s-%s" % (trns.name,
why do we need to report duplicate info? we have name, we have version, why report also the combination?
Line 89:                                                            trns.arch,
Line 90:                                                            trns.version,
Line 91:                                                            trns.release,
Line 92:                                                            )


Line 144:                             'will check all available updates')
Line 145:                         )
Line 146:     parser.add_argument('--provider',
Line 147:                         default='yum',
Line 148:                         choices=('yum', 'dnf'),
what are our plans regarding apt-get ?
Line 149:                         help=(
Line 150:                             'Specify package-management utility to use. \n'
Line 151:                             'Current available options - %s' %
Line 152:                             ('yum, dnf (default: yum)'))


Line 155:     return parser.parse_args(sys.argv[1:])
Line 156: 
Line 157: 
Line 158: if __name__ == '__main__':
Line 159:     if os.geteuid() != 0:
don't you want to make it a vdsm-tool command? at least for proper placement under the source and rpm trees.
Line 160:         outputPrint('updater-tool must run as root')
Line 161:         sys.exit(1)
Line 162: 
Line 163:     args = _parse_args()


https://gerrit.ovirt.org/#/c/38401/10/vdsm/vdsm-packages-update.in
File vdsm/vdsm-packages-update.in:

Line 1: 0 2 * * * root @VDSMDIR@/updater-tool -o @VDSMLIBDIR@/updates.list
unless you move the script to vdsm-tool, please place it under /usr/libexec/vdsm


-- 
To view, visit https://gerrit.ovirt.org/38401
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1f6b83e1c18c05142da095b0b537fa6d9aecb59e
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Barak Azulay <bazulay at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Dima Kuznetsov <dkuznets at redhat.com>
Gerrit-Reviewer: Moti Asayag <masayag at redhat.com>
Gerrit-Reviewer: Oved Ourfali <oourfali at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list