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