Change in vdsm[master]: bootstrap: use yum API

danken at redhat.com danken at redhat.com
Thu Sep 27 08:52:20 UTC 2012


Dan Kenigsberg has posted comments on this change.

Change subject: bootstrap: use yum API
......................................................................


Patch Set 9: I would prefer that you didn't submit this

(9 inline comments)

....................................................
File vds_bootstrap/miniyum.py
Line 348:                         if (
Line 349:                             transactionCurrent.altered_lt_rpmdb or
Line 350:                             transactionCurrent.altered_gt_rpmdb
Line 351:                         ):
Line 352:                             # safe guard?
I'd rather have the comment say:

  # I don't understand the above condition, I copied this from yum...

so that reviewers would understand what you say.
Line 353:                             pass
Line 354:                         else:
Line 355:                             try:
Line 356:                                 self._yb.repos.populateSack(


Line 393:                     ret = False
Line 394:                     msg = ""
Line 395:                     if type(e.value) is list:
Line 396:                         for s in e.value:
Line 397:                             msg += str(s) + "\n"
My recommendation is not to worry about the formatting of an error, and use the defaults. It does not seem related to the core function of MiniYum.

If the default is too ugly for you to bare, please use unicode() instead of str(), so that we do not explode on str('世界שלום').
Line 398:                     else:
Line 399:                         msg = str(e.value)
Line 400: 
Line 401:                     self._sink.error(


Line 406:                         raise
Line 407: 
Line 408:                 except Exception as e:
Line 409:                     self._sink.error(
Line 410:                         "cannot queue package %s: %s" % (package, e)
understood. no need to log traceback here.

'%s' calls The Right Thing, which is most probably unicode(x).
Line 411:                     )
Line 412:                     raise
Line 413: 
Line 414:         return ret


Line 446:             self._yb.repos.setProgressBar(
Line 447:                 MiniYum._downloadcallback(self._sink)
Line 448:             )
Line 449: 
Line 450:         except YumBaseError as e:
pardon for not noting this before - do we really want to return an object - which may be empty - instead of letting the exception pass through?

p.s. theoretically, self._sink may be uninitialized .
Line 451:             self._sink.error(str(e.value))
Line 452:         except Exception as e:
Line 453:             self._sink.error(str(e))
Line 454: 


Line 459:         to adjust proper roles for selinux.
Line 460:         it will re-execute the process with same arguments.
Line 461: 
Line 462:         This has similar effect of:
Line 463:         # chcon -t rpm_exec_t miniyum.py
sorry, I did not understand your reply here. At the least, this part of the comment is misleading
Line 464: 
Line 465:         We must do this dynamic as this class is to be
Line 466:         used at bootstrap stage, so we cannot put any
Line 467:         persistent selinux policy changes.


Line 513:             self._yb.cleanMetadata()
Line 514:             self._yb.cleanPackages()
Line 515:             self._yb.cleanSqlite()
Line 516: 
Line 517:     def install(self, packages, **kwargs):
ok, you want extensibility. I prefer stating all known args. Maybe I'm missing something.
Line 518:         """Install packages.
Line 519: 
Line 520:         Keyword arguments:
Line 521:         packages -- packages to install.


Line 797:                 if miniyum.buildTransaction():
Line 798:                     miniyum.processTransaction()
Line 799:                 raise Exception("Fail please")
Line 800:         except Exception as e:
Line 801:             if str(e) != "Fail please":
testing e.args[0] would be slightly better.

defining a local

  class RequestTransactionRollback(Exception)

and catching only it would have been clearer to me.
Line 802:                 raise
Line 803: 
Line 804: if __name__ == "__main__":


....................................................
File vds_bootstrap/vds_bootstrap.py
Line 82:         self._stream = os.dup(sys.stdout.fileno())
Line 83:         self._touch()
Line 84: 
Line 85:     def __del__(self):
Line 86:         if self._stream is not None:
if os.dup raises an exception, self_stream would not be set, and this test would fail on AtributeError.

You may want to replace the code with

 if hasattr(self, '_stream'):
    os.close()
Line 87:             os.close(self._stream)
Line 88: 
Line 89:     def _touch(self):
Line 90:         self._last = time.time()


Line 126:             hexkeyid
Line 127:         )
Line 128:         logging.warning("MiniYum: WARN:  %s", msg)
Line 129:         self._status('WARN', msg)
Line 130:         return True
do we currently auto-approve gpg keys?
Line 131: 
Line 132: 
Line 133: rhel6based = deployUtil.versionCompare(deployUtil.getOSVersion(), "6.0") >= 0
Line 134: 


--
To view, visit http://gerrit.ovirt.org/8039
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I65796801bc2db7c5abf71c1e9e4ad8ca308138b9
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev <alonbl at redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alonbl at redhat.com>
Gerrit-Reviewer: Barak Azulay <bazulay at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck at redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland at redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky at redhat.com>
Gerrit-Reviewer: Juan Hernandez <juan.hernandez at redhat.com>
Gerrit-Reviewer: Pradipta Banerjee <bpradip at in.ibm.com>
Gerrit-Reviewer: Rodrigo Trujillo <trujillo.unicamp at gmail.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Shireesh Anjal <sanjal at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>


More information about the vdsm-patches mailing list