Francesco Romani has posted comments on this change. Change subject: ceph: Support ephemeral Libvirt secrets ......................................................................
Patch Set 8: (6 comments) Looks good, a few questions inside, mostly about how to organize tests https://gerrit.ovirt.org/#/c/40712/8/tests/vmSecretTests.py File tests/vmSecretTests.py: Line 1: # the tests looks very nice and comprehensive. I only have a concern about having per-test Fakes, which I try to avoid to reduce the proliferation of them. I know there are cases on which is just more practical to have some localized test Fakes (I exploited one of those cases myself recently). But still, there is any chance to move or merge some of those fakes into vmfakelib? Line 2: # Copyright 2015 Red Hat, Inc. Line 3: # Line 4: # This program is free software; you can redistribute it and/or modify Line 5: # it under the terms of the GNU General Public License as published by Line 55: raise FakeLibvirtError(libvirt.VIR_ERR_NO_SECRET) Line 56: return self.secrets[sec_uuid] Line 57: Line 58: Line 59: class FakeLibvitSecret(object): typo: should be libvirt Line 60: Line 61: def __init__(self, con, uuid, xml): Line 62: self.con = con Line 63: self.uuid = uuid https://gerrit.ovirt.org/#/c/40712/8/vdsm/rpc/vdsmapi-schema.json File vdsm/rpc/vdsmapi-schema.json: Line 3896: # @key: The authentication key for this secret Line 3897: # Line 3898: # @ephemeral: #optional This secret must only be kept in memory, never Line 3899: # stored persistently Line 3900: # (True if not specfied) Do we have cases on which we DON'T want an ephemeral secret? Line 3901: # Line 3902: # @private: #optional The value of the secret must not be revealed to any Line 3903: # caller of libvirt, nor to any other node Line 3904: # (True if not specified) Line 3900: # (True if not specfied) Line 3901: # Line 3902: # @private: #optional The value of the secret must not be revealed to any Line 3903: # caller of libvirt, nor to any other node Line 3904: # (True if not specified) Same, do we have some clients which DON'T want a private secret? Line 3905: # Line 3906: # @description: #optional A human-readable description of this secret Line 3907: # Line 3908: # Since: 4.17.0 https://gerrit.ovirt.org/#/c/40712/8/vdsm/virt/secret.py File vdsm/virt/secret.py: Line 45: logging.error("Could not register secret %s: %s", secret, e) Line 46: return response.error("secretRegisterErr") Line 47: Line 48: # TODO: Use response.success when it is available Line 49: return {'status': doneCode} response.success() was recently (earlier this day!) merged Line 50: Line 51: Line 52: def unregister(uuids): Line 53: try: Line 144: Line 145: def _get_required(params, name): Line 146: value = params.get(name) Line 147: if not value: Line 148: raise ValueError("Missing or empty required property %r" % name) I'm under the impression that KeyError could be equally good and simpler. What do you think about this? -- To view, visit https://gerrit.ovirt.org/40712 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2e9ee33a7447ee07b0c82cf5a80d1f9b470663bb Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Vinzenz Feenstra <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
