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 <ybron...@redhat.com> Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Dima Kuznetsov <dkuzn...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: Yeela Kaplan <ykap...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches