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

Properly handle latin-1 encoding in file diffs #48934

Merged
merged 12 commits into from Aug 7, 2018

Conversation

Projects
None yet
3 participants
@terminalmage
Member

terminalmage commented Aug 3, 2018

This changes the encoding/decoding helpers to attempt latin-1 after initially trying UTF-8, which prevents a traceback when displaying diffs when a file with latin-1 encoding is updated using a file.managed state.

Resolves #48777

terminalmage added some commits Aug 3, 2018

Make to_unicode/to_str/to_bytes helpers attempt latin-1
Also allow for multiple encodings to be passed
PY3 scoping fix
In PY3 the caught exceptions now drop out of scope when leaving the for
loop.
@rallytime

It seems that __salt__ is being used instead of __utils__ in a couple of places. There's some lint/test errors as well.

@@ -1567,7 +1567,7 @@ def comment_line(path,
check_perms(path, None, pre_user, pre_group, pre_mode)
# Return a diff using the two dictionaries
return ''.join(difflib.unified_diff(orig_file, new_file))
return __salt__['stringutils.get_diff'](orig_file, new_file)

This comment has been minimized.

@rallytime

rallytime Aug 5, 2018

Contributor

Should this be __utils__ instead of __salt__?

changes_diff = ''.join(difflib.unified_diff(
path_content, body
))
changes_diff = __salt__['stringutils.get_diff'](path_content, body)

This comment has been minimized.

@rallytime

rallytime Aug 5, 2018

Contributor

Same question here as above.

@@ -2688,7 +2683,7 @@ def _add_content(linesep, lines=None, include_marker_start=True,
)
if block_found:
diff = ''.join(difflib.unified_diff(orig_file, new_file))
diff = __salt__['stringutils.get_diff'](orig_file, new_file)

This comment has been minimized.

@rallytime

rallytime Aug 5, 2018

Contributor

Here, too?

terminalmage added some commits Aug 6, 2018

@rallytime

This comment has been minimized.

Contributor

rallytime commented Aug 6, 2018

@terminalmage This looks like the tests will need some mocking love, due to the change to __utils__.

terminalmage added some commits Aug 6, 2018

@rallytime

This comment has been minimized.

Contributor

rallytime commented Aug 7, 2018

Fix to_unicode test
Since latin-1 is not being automatically decoded, we need to explicitly
pass it on the test.

@rallytime rallytime merged commit ab1a713 into saltstack:2018.3 Aug 7, 2018

5 of 8 checks passed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details
jenkins/pr/py2-centos-7 The py2-centos-7 job has failed
Details
jenkins/pr/py2-ubuntu-1604 running py2-ubuntu-1604...
Details
WIP ready for review
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint The lint 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

@terminalmage terminalmage deleted the terminalmage:issue48777 branch Aug 7, 2018

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