Nir Soffer has posted comments on this change. Change subject: properties: py3: properties.py and properties_test.py compliance ......................................................................
Patch Set 3: (5 comments) https://gerrit.ovirt.org/#/c/63230/3/lib/vdsm/properties.py File lib/vdsm/properties.py: Line 151: Line 152: class String(Property): Line 153: Line 154: def validate(self, value): Line 155: if not isinstance(value, six.string_types): This looks ok. Line 156: raise ValueError("Invalid value %r for string property %s" % Line 157: (value, self.name)) Line 158: return value Line 159: Line 230: Line 231: Line 232: def decode_base64(value): Line 233: try: Line 234: return base64.b64decode(value).decode() decode assume default encoding - we cannot use such assumptions. Fortunately, we don't have to deal with encodings here. The contents of a secret is binary data - this is why we encode this in base64. The result of decoding base64 must be bytes on both python 2 and 3, this is *not* a unicode string. Line 235: except TypeError as e: Line 236: raise ValueError("Unable to decode base64 value %s" % e) Line 237: Line 238: Line 253: obj.check(instance) Line 254: return instance Line 255: Line 256: Line 257: class Owner(six.with_metaclass(OwnerType, object)): This syntax is ugly, hopefully there is another way. Line 258: """ Line 259: Base class for classes using properties Line 260: Line 261: Inheriting from this class, all properties on this class and subclasses https://gerrit.ovirt.org/#/c/63230/3/tests/properties_test.py File tests/properties_test.py: Line 471 Line 472 Line 473 Line 474 Line 475 I think the correct fix is: self.assertEqual(b"12345678", obj.password.value) We need to check that users of this code are not assuming by mistake that password.value is unicode value, and are not trying to encode it somehow. Francesco, what do you think? Line 470: password = properties.Password(decode=properties.decode_base64) Line 471: Line 472: def test_decode(self): Line 473: obj = self.Cls() Line 474: obj.password = ProtectedPassword(base64.b64encode(b"12345678")) Looks good Line 475: self.assertEqual("12345678", obj.password.value) Line 476: Line 477: def test_invalid(self): Line 478: obj = self.Cls() -- To view, visit https://gerrit.ovirt.org/63230 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id3f49dd718d786c4a98b06c7b8757ce5a9eb6d2a Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Leon Goldberg <leon.ot...@gmail.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
