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

Fixed unknown 'exceptions' under Python3 (#49152) #49162

Merged
merged 4 commits into from Aug 17, 2018

Conversation

Projects
None yet
3 participants
@erwindon
Copy link
Contributor

commented Aug 16, 2018

What does this PR do?

This PR fixes the (wheel)error.error command, which fails under python3.
The solution is a small code transplant from file _compat.py.

What issues does this PR fix or reference?

This error was previously reported as #49152.

Previous Behavior

Always exception about unknown identifier "exceptions"

New Behavior

Behavior according to documentation.

Tests written?

No
Tested using command sudo salt HOSTNAME saltutil.wheel error.error "Exception" "This is an error." after copying the updated file salt/utils/error.py into an installed system and restarting both the master and the minion of that system.
Manually verified that all import exceptions command are covered. I only found 2 cases: one in _compat.py, and this PR solves the issue for the other one (salt/utils/error.py).

Commits signed with GPG?

No


if PY3:
import builtins
exceptions = builtins

This comment has been minimized.

Copy link
@dwoz

dwoz Aug 16, 2018

Contributor

This can be from salt.ext.six.moves import builtins as exceptions

This comment has been minimized.

Copy link
@erwindon

erwindon Aug 16, 2018

Author Contributor

@dwoz
Are you suggesting to just combine lines 13+14?
Then why not just import builtins as exceptions?
And should that also be applied to _compat.py?

This comment has been minimized.

Copy link
@dwoz

dwoz Aug 17, 2018

Contributor

Using salt.ext.six also removes the need for the python 3 check. So 8-14 can become one line. Yes, the same can be done for lines 37-45 in _compat.py

This comment has been minimized.

Copy link
@erwindon

erwindon Aug 17, 2018

Author Contributor

The code for utils/error.py is updated using this suggestion and re-tested in the same way as before.
I realize that changing _compat.py should not be done as part of this bugfix, as there is no bug there. So I did not update that one.

@erwindon erwindon force-pushed the erwindon:wheel_error_error branch from dc20dbe to ec1f013 Aug 17, 2018

@erwindon erwindon changed the base branch from develop to 2017.7 Aug 17, 2018

@erwindon erwindon force-pushed the erwindon:wheel_error_error branch from bb3358d to 4335c5c Aug 17, 2018

Mike Place
@cachedout

This comment has been minimized.

Copy link
Collaborator

commented Aug 17, 2018

@dwoz Re-review requested.

@erwindon

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2018

@cachedout I was just thinking on how to force the regression test to re-run as older/failing jobs had finished later and github got confused about that... your merge action just solved that 👍

@dwoz

dwoz approved these changes Aug 17, 2018

@dwoz dwoz merged commit 7486fd5 into saltstack:2017.7 Aug 17, 2018

8 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 The lint job 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/py3-centos-7 The py3-centos-7 job has passed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has passed
Details

@erwindon erwindon deleted the erwindon:wheel_error_error branch Aug 17, 2018

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.