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 _compat for py3 #51918

Merged
merged 2 commits into from Mar 7, 2019

Conversation

@twangboy
Copy link
Contributor

commented Feb 28, 2019

What does this PR do?

Fixes some code that is incompatible with PY3.

What issues does this PR fix or reference?

#51831

Tests written?

No

Commits signed with GPG?

Yes

@twangboy twangboy requested a review from saltstack/team-core Feb 28, 2019
@dwoz

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

@twangboy Is it possible to write a regression test for this?

@twangboy

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

@dwoz Probably. I'll give it a shot.

@KChandrashekhar KChandrashekhar added this to the March 15th milestone Mar 1, 2019
@twangboy twangboy removed their assignment Mar 1, 2019
@KChandrashekhar KChandrashekhar assigned twangboy and unassigned twangboy Mar 1, 2019
@twangboy

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2019

Closed in favor of #51931

@twangboy twangboy closed this Mar 2, 2019
@afischer-opentext-com

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

@twangboy , sorry, but are we sure that #51931 fixes this issue? In https://github.com/saltstack/salt/pull/51931/files#diff-246af447f37621b35b4927bacd8a23c8L194 I still see invalid Python 3 (line 193) as Python 3 bytearray() method requires at 2 arguments, not only one. Just asking...

@twangboy twangboy reopened this Mar 5, 2019
@twangboy

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

@afischer-opentext-com Do you have some sample data that failes on Py3 and not Py2. I'm going to have to write some tests for this.

@afischer-opentext-com

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

@twangboy , in our case it's a simple b's3.amazonaws.com' which causes the method to fail.

@twangboy

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

Sweet! Thanks. I'll get started on a test for that case.

twangboy added 2 commits Feb 28, 2019
@twangboy twangboy force-pushed the twangboy:fix_compat branch from 2c3d69e to a791901 Mar 6, 2019
@twangboy twangboy requested a review from terminalmage Mar 6, 2019
@dwoz
dwoz approved these changes Mar 6, 2019
@dwoz dwoz merged commit 48d298c into saltstack:2018.3 Mar 7, 2019
10 checks passed
10 checks passed
WIP Ready for review
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint Python lint test has passed
Details
jenkins/pr/py2-centos-7 The py2-centos-7 job has passed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has passed
Details
jenkins/pr/py2-windows-2016 The py2-windows-2016 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
jenkins/pr/py3-windows-2016 The py3-windows-2016 job has passed
Details
@twangboy twangboy deleted the twangboy:fix_compat branch Mar 7, 2019
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.