Change in vdsm[master]: properties: py3: properties.py and properties_test.py compli...

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: properties: py3: properties.py and properties_test.py compliance
..


Patch Set 3:

Your new patches replace this patch, you can abandon it now.

-- 
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 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Leon Goldberg 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: Tomas Golembiovsky 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: properties: py3: properties.py and properties_test.py compli...

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: properties: py3: properties.py and properties_test.py compliance
..


Patch Set 3:

(1 comment)

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:
This test is confusing the reader to think that password content is ascii. We 
should use binary data in the test:

data = b"\x80\x81\x82\x83"
obj.password = ProtectedPassword(base64.b64encode(data))
self.assertEqual(data, obj.password.value)

Using this test data, incorrect decoding attempt will fail, alerting the 
developer:

>>> b"\x80\x81\x82\x83".decode()
Traceback (most recent call last):
  File "", line 1, in 
UnicodeDecodeError: 'ascii' codec can't decode byte 0x80 in position 0: 
ordinal not in range(128)

>>> b"\x80\x81\x82\x83".decode('utf8')
Traceback (most recent call last):
  File "", line 1, in 
  File "/usr/lib64/python2.7/encodings/utf_8.py", line 16, in decode
return codecs.utf_8_decode(input, errors, True)
UnicodeDecodeError: 'utf8' codec can't decode byte 0x80 in position 0: 
invalid start byte


-- 
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 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Leon Goldberg 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: Tomas Golembiovsky 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: properties: py3: properties.py and properties_test.py compli...

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: properties: py3: properties.py and properties_test.py compliance
..


Patch Set 3:

You can remove the module from the blacklist in the last patch, or just send 
another one on top.

-- 
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 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Leon Goldberg 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: Tomas Golembiovsky 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: properties: py3: properties.py and properties_test.py compli...

[nsoffer]

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 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: Tomas Golembiovsky 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: properties: py3: properties.py and properties_test.py compli...

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: properties: py3: properties.py and properties_test.py compliance
..


Patch Set 3:

Looking in the failures when running this under python 3, we have 3 issues:

1. decoding passwords requires bytes instead of string

==
ERROR: test_decode (properties_test.PasswordDecodeTests)
--
Traceback (most recent call last):
  File "/home/nsoffer/src/vdsm/tests/properties_test.py", line 474, in 
test_decode
obj.password = ProtectedPassword(base64.b64encode("12345678"))
  File "/usr/lib64/python3.5/base64.py", line 62, in b64encode
encoded = binascii.b2a_base64(s)[:-1]
TypeError: a bytes-like object is required, not 'str'


2. basestring missing in python 3

==
ERROR: test_empty (properties_test.StringRequiredTests)
--
Traceback (most recent call last):
  File "/home/nsoffer/src/vdsm/tests/properties_test.py", line 201, in 
test_empty
obj = self.Cls("")
  File "/home/nsoffer/src/vdsm/tests/properties_test.py", line 195, in __init__
self.value = value
  File "/home/nsoffer/src/vdsm/lib/vdsm/properties.py", line 115, in __set__
value = self.validate(value)
  File "/home/nsoffer/src/vdsm/lib/vdsm/properties.py", line 154, in validate
if not isinstance(value, basestring):
NameError: name 'basestring' is not defined

3. metaclass verification not running

==
FAIL: test_required (properties_test.PropertyRequiredTests)
--
Traceback (most recent call last):
  File "/home/nsoffer/src/vdsm/tests/properties_test.py", line 65, in 
test_required
self.assertRaises(ValueError, self.Cls)
AssertionError: ValueError not raised by Cls

Can you break this patch to 3 smaller patches for each issue?

-- 
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 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: Tomas Golembiovsky 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: properties: py3: properties.py and properties_test.py compli...

[piotr . kliczewski]

Piotr Kliczewski has posted comments on this change.

Change subject: properties: py3: properties.py and properties_test.py compliance
..


Patch Set 3:

(2 comments)

https://gerrit.ovirt.org/#/c/63230/3//COMMIT_MSG
Commit Message:

PS3, Line 9:  
Please remove white space at the end of the line


PS3, Line 12: use six's with_metaclass to comply with both 2 and 3.
We could have 2 simple patches one for base64 and string_types and the other 
for metaclass change.


-- 
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 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: Tomas Golembiovsky 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: properties: py3: properties.py and properties_test.py compli...

[automation]

gerrit-hooks has posted comments on this change.

Change subject: properties: py3: properties.py and properties_test.py compliance
..


Patch Set 3:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-4.0'])

-- 
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 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: properties: py3: properties.py and properties_test.py compli...

[Jenkins CI]

Jenkins CI has posted comments on this change.

Change subject: properties: py3: properties.py and properties_test.py compliance
..


Patch Set 2: Continuous-Integration+1

Propagate review hook: Continuous Integration value inherited from patch 1

-- 
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: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Leon Goldberg 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: properties: py3: properties.py and properties_test.py compli...

[automation]

gerrit-hooks has posted comments on this change.

Change subject: properties: py3: properties.py and properties_test.py compliance
..


Patch Set 2:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-4.0'])

-- 
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: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Leon Goldberg 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: properties: py3: properties.py and properties_test.py compli...

[automation]

gerrit-hooks has posted comments on this change.

Change subject: properties: py3: properties.py and properties_test.py compliance
..


Patch Set 1:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-4.0'])

-- 
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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Leon Goldberg 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org