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

Improve handling of non-ascii characters in terminal output. (Fixes: #49523) #50146

Merged
merged 1 commit into from Oct 22, 2018

Conversation

Projects
None yet
4 participants
@MTecknology
Copy link
Contributor

commented Oct 20, 2018

What does this PR do?

Prevents exception when stdout contains non-ascii characters.

What issues does this PR fix or reference?

Tests written?

No

Commits signed with GPG?

Yes

@rallytime rallytime merged commit 17b5b0f into saltstack:develop Oct 22, 2018

10 of 11 checks passed

codeclimate 5 issues to fix
Details
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/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
@damon-atkins

This comment has been minimized.

Copy link
Member

commented Oct 22, 2018

Please read the comments in the original issue. #49523

@damon-atkins

This comment has been minimized.

Copy link
Member

commented Oct 22, 2018

output_encoding param from cmdmod is not passed onto vt and should be if stdout/stderr is going to be processed in vt

@damon-atkins

This comment has been minimized.

Copy link
Member

commented Oct 22, 2018

cmdmod uses code like:

            out = salt.utils.stringutils.to_unicode(
                proc.stdout,
                encoding=output_encoding)

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

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

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

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Oct 25, 2018

@MTecknology So this change caused all of our salt-ssh tests to fail, and we didn't catch it in your PR here because we made some recent updates to the way our tests run on PRs and we didn't account for the salt.utils.vt file to affect salt-ssh. (I've fixed that problem today in #50230.)

That being said, we need to pull back this fix. I have been looking at it today and I think I have something that will work, but I am unsure because I haven't been able to reproduce the cloud error you were attempting to fix. I think the only encode part we need is that first line you adjusted at the self.stream_stdout.write. I'll submit a PR right now, but can you test it and make sure it does what you need it to do?

@MTecknology MTecknology deleted the MTecknology:fix-49523 branch Oct 25, 2018

@MTecknology

This comment has been minimized.

Copy link
Contributor Author

commented Oct 25, 2018

Yup, I can test it once I see the PR come through.

rallytime added a commit to rallytime/salt that referenced this pull request Oct 25, 2018

Don't encode the return values in utils/vt.py
These encodings were causing the salt-ssh tests to fail.

This is a partial revert of saltstack#50146.

In order to see if the tests succeed, PR saltstack#50230 will need to be
merged first.
@rallytime

This comment has been minimized.

Copy link
Contributor

commented Oct 25, 2018

@MTecknology Ok, give #50235 a try. It might not be right, since I couldn't test it. But that fixes the salt-ssh test failures. I think it would also fix the cloud problems based on the descriptions, but please give that a try and let me know how it goes.

rallytime added a commit to rallytime/salt that referenced this pull request Oct 25, 2018

Don't encode the return values in utils/vt.py
These encodings were causing the salt-ssh tests to fail.

This is a partial revert of saltstack#50146.

In order to see if the tests succeed, PR saltstack#50230 will need to be
merged first.

rallytime added a commit to rallytime/salt that referenced this pull request Oct 25, 2018

Fix salt-cloud UnicodeEncodeError when writing to stdout
This is a partial back-port of saltstack#50146.

The full back-port was reverted due to the original change causing all
of the salt-ssh tests to fail.

This change fixes the salt-cloud error, but allows salt-ssh to work
correctly.

Refs saltstack#49523, saltstack#50146, saltstack#50174, saltstack#50231, and saltstack#50235
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.