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

values need to be bytes when writing back to ldap #48666

Merged
merged 16 commits into from Oct 1, 2018

Conversation

Projects
None yet
5 participants
@angeloudy
Copy link
Contributor

commented Jul 19, 2018

What does this PR do?

What issues does this PR fix or reference?

fix for #48599

@rallytime rallytime requested a review from terminalmage Jul 20, 2018

Show resolved Hide resolved salt/modules/ldap3.py Outdated
@dwoz

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2018

Should the encoding/decoding be specific to the ldap server? It looks like this PR is basing the encoding/decoding based on the client's system settings. Have you thought about what happens if the client system is using a non utf-8 encoding?

angeloudy and others added some commits Jul 23, 2018

@angeloudy

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2018

@dwoz
That's a good point!
I have no idea how to get the encoding.
What do you suggest?

@dwoz

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2018

@angeloudy I think it's best to use utf-8 here (it looks like that is what LDAPv3 expects by default). We can make it configurable down the road if needed

@cachedout

This comment has been minimized.

Copy link
Collaborator

commented Aug 10, 2018

@dwoz and @angeloudy There is also __salt_system_encoding__ that should be available.

@cachedout

This comment has been minimized.

Copy link
Collaborator

commented Aug 17, 2018

Hi @angeloudy. Where are we on this one?

@angeloudy

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2018

@cachedout should salt.utils.data.encode use __salt_system_encoding__ by default?
I did have any 'utf8' hardcoded in my changes.

@terminalmage

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2018

@angeloudy It would try utf8 first, and then try the detected encoding if that raises a UnicodeEncodeError.

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Aug 31, 2018

@angeloudy Are you planning on making the requested changes here?

@angeloudy

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2018

@rallytime what requested changes.
I am fine with my current version.

@angeloudy

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2018

@terminalmage @rallytime
if an UnicodeEncodeError exception catching is necessary, shouldn't it be done inside salt.utils.data.encode?

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2018

@terminalmage and @dwoz can you comment/re-review?

angeloudy added some commits Sep 5, 2018

@angeloudy

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2018

Hi, any plans to merge this pull request so that I don't have to maintain my own patch?
I don't understand why are you guys worrying about UnicodeEncodeError, the encode method is being used everywhere in saltstack.

@rallytime rallytime requested review from terminalmage and dwoz Sep 24, 2018

@dwoz

dwoz approved these changes Sep 24, 2018

@terminalmage
Copy link
Contributor

left a comment

I don't think anyone is worrying about an encoding error. You still haven't made the suggested changes from a few months ago, which is probably why this hasn't been merged.

Show resolved Hide resolved salt/modules/ldap3.py Outdated

angeloudy added some commits Sep 27, 2018

@rallytime rallytime requested a review from terminalmage Sep 27, 2018

@rallytime rallytime merged commit 365d7f7 into saltstack:develop Oct 1, 2018

6 of 11 checks passed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details
codeclimate 3 issues to fix
Details
jenkins/pr/py2-centos-7 The py2-centos-7 job has failed
Details
jenkins/pr/py2-windows-2016 The py2-windows-2016 job has failed
Details
jenkins/pr/py3-windows-2016 The py3-windows-2016 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
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has passed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has passed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has passed
Details

rallytime added a commit that referenced this pull request Oct 9, 2018

@ghost ghost referenced this pull request Mar 8, 2019

Open

ldap.managed Errors #52022

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.