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 UnicodeDecodeError when parsing hosts file with non-ascii #48081

Merged
merged 12 commits into from Jun 15, 2018

Conversation

Projects
None yet
5 participants
@terminalmage
Member

terminalmage commented Jun 13, 2018

This also adds a regression test, and I also added the following goodies to mock_open while I was writing the test:

  1. Now works properly when read_data is a bytestring
  2. Now works properly when mocked filehandle is iterated (e.g. for line in fh:)
  3. Adds ability to make mocked filehandle raise an IOError when the path being opened doesn't match a pattern (or one of a list of patterns).

Fixes #48012.

terminalmage added some commits Jun 13, 2018

Fixes / enhancements for mock_open
This makes the following changes:

1. Now works properly when read_data is a bytestring
2. Now works properly when mocked filehandle is iterated (e.g. `for line in fh:`)
3. Adds ability to make mocked filehandle raise an IOError when the path
   being opened doesn't match a pattern (or one of a list of patterns).
@cachedout

This comment has been minimized.

Contributor

cachedout commented Jun 13, 2018

@terminalmage Failed tests here

@rallytime

This comment has been minimized.

Contributor

rallytime commented Jun 13, 2018

@terminalmage It looks like the tests didn't like this very much.

terminalmage added some commits Jun 13, 2018

Fix crappy mocking
This mocking was poorly done, which was revealed when I made an EAFP
modification to _generate_minion_id().
Get rid of additional newline append
This looks like it was just incorrectly done before. writelines()
expects you to take care of newlines yourself, so each element should
end in a newline. But we were doing two separate writes, one with the
key=value pair and one with a newline
@terminalmage

This comment has been minimized.

Member

terminalmage commented Jun 13, 2018

@twangboy can you have a look at the mac_sysctl changes? I have the tests passing now but I think that mac_sysctl.persist had a bug in it... .writelines() just does a .write() for each element in the iterable that is passed to it, and so they expect you to provide your own trailing newlines. However, the code in mac_sysctl.persist does two appends, one with the key=value pair and one with the newline. This technically would have produced the correct outcome, but the test would not reflect that we wrote what we would expect. I made changes to make it behave how it looks like it should, but I would appreciate another set of eyes on that.

@twangboy

Looks good to me

terminalmage added some commits Jun 14, 2018

@terminalmage

This comment has been minimized.

Member

terminalmage commented Jun 14, 2018

Fixed up a couple lint issues, the tests should pass.

@rallytime

This comment has been minimized.

Contributor

rallytime commented Jun 14, 2018

@terminalmage The tests on the Ubuntu 16 Py3 build are not failing on the branch, so I think they're related to this. Can you take another look?

https://jenkins.saltstack.com/job/PR/job/salt-pr-linode-ubuntu16-py3/10699/

terminalmage added some commits Jun 14, 2018

Fix definition of test data for wtmp/btmp beacon tests
There is no need to do things differently on PY2 and PY3
Fix cp.push test
mock_open now properly handles bytestrings, so the expected data needed
to be updated.
@terminalmage

This comment has been minimized.

Member

terminalmage commented Jun 14, 2018

@rallytime Those PY3 failures were all because bytestrings are now properly handled in mock_open. I updated those tests.

@terminalmage

This comment has been minimized.

Member

terminalmage commented Jun 15, 2018

Test failure appears unrelated (I can't reproduce locally).

@rallytime rallytime merged commit c05c176 into saltstack:2018.3 Jun 15, 2018

6 of 9 checks passed

jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #19806 — ABORTED
Details
default Build finished.
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #23682 — FAILURE
Details
WIP ready for review
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #25955 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #18010 — SUCCESS
Details
jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #5752 — SUCCESS
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #10723 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #22645 — SUCCESS
Details

@terminalmage terminalmage deleted the terminalmage:issue48012 branch Jun 18, 2018

@Waterdrips

This comment has been minimized.

Waterdrips commented Jul 10, 2018

Just bumped into this - many thanks for the fix @terminalmage .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment