Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix typeerror unicode #49199

Merged
merged 3 commits into from Aug 23, 2018

Conversation

Projects
None yet
5 participants
@jacobweinstock
Copy link
Contributor

commented Aug 20, 2018

fix TypeError: 'unicode' does not have the buffer interface and updates for python 3 support

from __future__ import unicode_literals - breaks this module in python 2. I found python 3 to not be supported as well. It appears that the pypureomapi library leverages sockets that are designed to only take byte strings. In python 2, strings are already byte strings and using unicode_literals causes the TypeError. In python 3 strings are unicode by default and thus need to be encoded to byte strings. This update encodes string literals into byte strings and provides support for both python 2 and 3.

What does this PR do?

fix TypeError: 'unicode' does not have the buffer interface introduced by from __future__ import unicode_literals and handles python 3 support

What issues does this PR fix or reference?

n/a

Tests written?

No

Commits signed with GPG?

Yes

@cachedout cachedout requested a review from terminalmage Aug 20, 2018

@@ -13,7 +13,7 @@
'''

# Import python libs
from __future__ import absolute_import, print_function, unicode_literals
from __future__ import absolute_import, print_function

This comment has been minimized.

Copy link
@terminalmage

terminalmage Aug 20, 2018

Contributor

This needs a comment as to why we don't have unicode_literals imported, so that someone doesn't add it back later.

if key:
key = key.encode('UTF-8')
if username:
username = username.encode('UTF-8')

This comment has been minimized.

Copy link
@terminalmage

terminalmage Aug 20, 2018

Contributor

Instead of using encode here, you should be using salt.utils.stringutils.to_bytes(). This will avoid an AttributeError if this value happens to already be a bytestring (though this is very unlikely).

@jacobweinstock jacobweinstock requested a review from saltstack/team-core as a code owner Aug 21, 2018

@jacobweinstock jacobweinstock force-pushed the jacobweinstock:fix-typeerror-unicode branch from d27c48b to 6944baf Aug 21, 2018

@@ -13,10 +13,14 @@
'''

# Import python libs
from __future__ import absolute_import, print_function, unicode_literals
# Not importing unicode_literals as pypureomapi needs byte strings not unicode
from __future__ import absolute_import, print_function

This comment has been minimized.

Copy link
@DmitryKuzmenko

DmitryKuzmenko Aug 21, 2018

Contributor

I don't see the reason to remove unicode_literals from here. Since we need binary strings here, all the string constants already explicitly forced to be binary strings by this PR. So unicode_literals breaks nothing. But having this here is following our coding standard and if there's no strong reason to remove, I'd better like to import it ever if it does nothing.

This comment has been minimized.

Copy link
@terminalmage

terminalmage Aug 21, 2018

Contributor

This is a good point, actually.

@damon-atkins

This comment has been minimized.

Copy link
Member

commented Aug 21, 2018

Cannot see the reason to remove unicode_literals as you have made changes are prefixed items with b''
msg.message.append((b'create', struct.pack(b'!I', 1)))

It also impacts how logs are return, trace stacks are return etc.

Please leave unicode_literals and work around it.

jacobweinstock added some commits Aug 22, 2018

@gtmanfred gtmanfred merged commit 5da743a into saltstack:2018.3 Aug 23, 2018

3 of 8 checks passed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details
jenkins/pr/py2-centos-7 The py2-centos-7 job has failed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has failed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has failed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has failed
Details
WIP ready for review
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint The lint job has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.