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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mark vars that shouldn't be templated with AnsibleUnsafe instead of {% raw %} #1071

Merged
merged 1 commit into from Mar 10, 2019

Conversation

Projects
None yet
3 participants
@louim
Copy link
Contributor

louim commented Feb 28, 2019

Hey! 馃憢

Been a long time since I contributed here. I maintain https://github.com/louim/bedrock-site-protect and I was investigating louim/bedrock-site-protect#19. The issue is related to string containing jinja delimiters like {{ or {%, like salts or passwords. I see that trellis deal with those strings with raw_vars to wrap those string with jinja {% raw %} tags.

However, it seems like templating those value in a variable (using the values in a variable that will itself be templated) will break the raw escaping, which is causing the issue in my case. I had a look at an issue where they recommended using !unsafe to deal with values that need to be escaped in yaml variables.

I had a look at what the raw_vars does in trellis, and tweaked it a little to use Ansible UnsafeProxy instead of wrapping the variables with raw tags. UnsafeProxy wrap_var is what adding !unsafe in a yaml template does under the hood.

I redid the test that @fullyint mentioned in #615 (comment) to confirm that the code is still producing the same result.

I also confirmed that it fixes louim/bedrock-site-protect#19.

However, It's been a loooong time since I used Trellis, I may not be aware of all the subtleties of the code and uses-case, so I would really appreciate 馃憖 to ensure I haven't broken anything by mistake. Also happy to discuss! 馃帀

@swalkinshaw

This comment has been minimized.

Copy link
Member

swalkinshaw commented Feb 28, 2019

However, It's been a loooong time since I used Trellis, I may not be aware of all the subtleties of the code and uses-case

To be honest I don't know too much about this specific functionality either. This looks good to me and seems much better with a cleaner implementation.

Maybe @fullyint can chime in if he's around. If not, I'll merge this in a bit. Thanks for your testing attempts 馃槃

@fullyint

This comment has been minimized.

Copy link
Member

fullyint commented Mar 3, 2019

Works in my basic testing. I don't anticipate any problems.
Looks like a solid solution to a difficult to debug issue.
Thank you @louim!

@swalkinshaw swalkinshaw merged commit 721894d into roots:master Mar 10, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.