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

encode topic before using hashlib #51983

Closed
wants to merge 3 commits into from

Conversation

@arsiesys
Copy link

commented Mar 5, 2019

What does this PR do?

Fix #51982

Tested with python2 and python3

Copy link
Contributor

left a comment

@arsiesys are you able to write a regression test for this?

@arsiesys

This comment has been minimized.

Copy link
Author

commented Mar 8, 2019

@arsiesys are you able to write a regression test for this?

I don't think so.. I checked the test_zeromq.py and I think it's going too low level in salt/zeromq for me..

@pengyao

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

@arsiesys use salt.utils.stringutils.to_str(messages[0]) and salt.util.stringutils.to_bytes(topic) will more clearly

@arsiesys arsiesys force-pushed the arsiesys:fix_py3_zmq_encoding branch from 905b235 to 5e4ed28 Mar 13, 2019
@arsiesys

This comment has been minimized.

Copy link
Author

commented Mar 13, 2019

@arsiesys use salt.utils.stringutils.to_str(messages[0]) and salt.util.stringutils.to_bytes(topic) will more clearly

Thanks, I applied the change.

Loïc Yavercovski
@arsiesys arsiesys force-pushed the arsiesys:fix_py3_zmq_encoding branch from 5e4ed28 to cbfa616 Mar 14, 2019
@Ch3LL

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

ping @arsiesys any chance you can add some tests to ensure this doesn't regress in the future again?

@Ch3LL Ch3LL added the 2019.2.1 label Apr 9, 2019
@dwoz

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

@arsiesys @Ch3LL I still think this needs a regression test of some kind. I'm guessing an integration tests will work for this. @arsiesys, Can you take a crack at creating an integration test to cover this? If not please assign the issue to me and I will look into it.

@arsiesys

This comment has been minimized.

Copy link
Author

commented Apr 17, 2019

Hello @dwoz, I don't think I can do that.. It's too low level for me :(.

I can't change this PR assignee user. Can you take it over please ?

@Ch3LL Ch3LL self-assigned this Apr 26, 2019
@Ch3LL

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

since we really want to get this into 2019.2.1 i went ahead and pushed the tests in this PR #52767 so the tests can start running and we can try to merge. I'll go ahead and close here and we can work together on that PR

@Ch3LL Ch3LL closed this Apr 30, 2019
dwoz added a commit that referenced this pull request May 3, 2019
[2019.2.1] Add tests to PR  #51983
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.