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

Coerce string when calling compile_template_str #49943

Merged
merged 6 commits into from Oct 11, 2018

Conversation

Projects
None yet
6 participants
@thetaurean
Copy link
Contributor

commented Oct 8, 2018

What does this PR do?

This fixes a bug in the file_tree pillar which causes it to fail to render templates in python 3. The caller for the template rendering function passes bytes instead of string.

What issues does this PR fix or reference?

Issue #49927

Previous Behavior

else:
                data = contents
                if template is True:
                    data = salt.template.compile_template_str(template=contents,
                                                              renderers=renderers,
                                                              default=render_default,
                                                              blacklist=renderer_blacklist,
                                                              whitelist=renderer_whitelist)

New Behavior

else:
                data = contents
                if template is True:
                    data = salt.template.compile_template_str(template=salt.utils.stringutils.to_unicode(contents),
                                                              renderers=renderers,
                                                              default=render_default,
                                                              blacklist=renderer_blacklist,
                                                              whitelist=renderer_whitelist)

Tests written?

No. New functionality not added

Commits signed with GPG?

Yes

Coerce string when calling compile_template_str
Previously on python3 this would otherwise pass bytes (unhandled)

@thetaurean thetaurean requested a review from saltstack/team-core as a code owner Oct 8, 2018

@salt-jenkins salt-jenkins requested a review from saltstack/team-suse Oct 8, 2018

@gtmanfred
Copy link
Contributor

left a comment

just a styling change.

@@ -158,6 +158,7 @@
import salt.utils.stringio
import salt.template
from salt.ext import six
from salt.utils.stringutils import to_str

This comment has been minimized.

Copy link
@gtmanfred

gtmanfred Oct 8, 2018

Contributor

please don't import functions like this, it will add it to the functions in the modules.

import salt.utils.stringutils
@@ -251,7 +252,7 @@ def _construct_pillar(top_dir,
else:
data = contents
if template is True:
data = salt.template.compile_template_str(template=contents,
data = salt.template.compile_template_str(template=to_str(contents),

This comment has been minimized.

Copy link
@gtmanfred

gtmanfred Oct 8, 2018

Contributor

salt.utils.stringutils.to_str(contents)

@gtmanfred

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2018

@terminalmage should this be to_str? or something else?

Thanks,
Daniel

@thetaurean

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2018

@gtmanfred Addressed. Thank you.

@gtmanfred
Copy link
Contributor

left a comment

This looks good to me, still want @terminalmage or someone else to take a look.

@gtmanfred

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2018

I have also marked this to be backported, for future reference, PRs that fix bugs should be opened against the oldest feature release that the bug is present in, so in this case that would be the 2018.3 branch. (2018.3.3 is a release branch and not the main feature branch)

@thetaurean

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2018

@gtmanfred Good point, aww shucks!

I am happy to rebase the changes on 2018.3.3 and force push, thoughts?

@gtmanfred

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2018

There is 0 chance this makes it in 2018.3.3, it has already been tagged and is being packaged.

But i have marked it for backporting, so it will be in 2018.3.4

So don't worry about rebasing, that was just for future reference. :)

@dwoz dwoz self-requested a review Oct 9, 2018

@isbm

isbm approved these changes Oct 9, 2018

@rallytime rallytime requested a review from terminalmage Oct 9, 2018

@dwoz

dwoz approved these changes Oct 9, 2018

@terminalmage
Copy link
Contributor

left a comment

salt.templates.compile_template_str() writes the template to a tempfile in binary mode, and therefore encodes to UTF-8 before doing so. This is why Python 3 raised the error.

However, because we are encoding, we want the contents to be unicode first, otherwise Python 2 will break when non-ascii characters are present in the string we pass:

>>> 'groß'.encode('utf-8')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
UnicodeDecodeError: 'ascii' codec can't decode byte 0xc3 in position 3: ordinal not in range(128)

Therefore, please change from to_str() to to_unicode().

Handle non-ASCII chars during template data decode
Change requested by terminalmage, line 255.
@thetaurean

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2018

@terminalmage Addressed. Excellent observation and appreciate the insight gained from your thorough explanation 👍

@gtmanfred rgr that. thanks again, this is great exposure to public github etiquette

@terminalmage
Copy link
Contributor

left a comment

Well done, thank you for contributing!

@rallytime rallytime merged commit ea71844 into saltstack:develop Oct 11, 2018

6 of 10 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-windows-2016 The py2-windows-2016 job has failed
Details
jenkins/pr/py3-windows-2016 The py3-windows-2016 job has failed
Details
codeclimate All good!
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint The lint 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

rallytime added a commit that referenced this pull request Oct 15, 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.