Skip to content
This repository was archived by the owner on Aug 14, 2026. It is now read-only.

Fix mustache escaping #262

Closed
taylorjacklespriggs wants to merge 10 commits into from
Closed

Fix mustache escaping #262

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
9eb7e60
use custom chevron version
tjs-intel Oct 13, 2021
f27a6ea
use escape option
tjs-intel Oct 13, 2021
590868e
fix test
tjs-intel Oct 13, 2021
f1b2b49
chevron as submodule
tjs-intel Oct 14, 2021
0586afd
remove chevron git
tjs-intel Oct 14, 2021
d97666b
use vendored chevron
tjs-intel Oct 14, 2021
e039a69
remove vendored
tjs-intel Oct 14, 2021
1be964a
add chevron submodule top level
tjs-intel Oct 14, 2021
7c97d0a
setup submodules
tjs-intel Oct 14, 2021
1ab97cb
chevron patch branch
tjs-intel Oct 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ Pint>=0.16.0,<0.17.0
PyYAML==5.4.1,<6.0.0
backoff>=1.10.0,<2.0.0
boto3==1.16.34,<2.0.0
chevron>=0.14.0,<1.0.0
click>=8.0.0
docker>=4.4.0,<5.0.0
ipython>=5.0.0
Expand Down
1 change: 1 addition & 0 deletions setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@

with open(os.path.join(here, 'requirements.txt')) as requirements_fp:
install_requires = requirements_fp.read().split('\n')
install_requires.append("chevron @ git+https://github.com/taylorjacklespriggs/chevron@support-custom-escaping")

@taylorjacklespriggs taylorjacklespriggs Oct 13, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

noahmorrison/chevron#107

@taylorjacklespriggs taylorjacklespriggs Oct 13, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

last commit was mar 21, 2021. not confident this will get merged any time soon

@dwanderson-intel dwanderson-intel Oct 13, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't a cannonical address (since we can update the branch and it will install the newer updates). Not sure if that's better or not, but wanted to mention

@simonhowey simonhowey Oct 13, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would prefer we maybe fork into sigopt?

@taylorjacklespriggs taylorjacklespriggs Oct 14, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense to me, almost identical for licensing. As a bonus, chevron has no requirements so we wouldn't even need to update requirements.txt

with open(os.path.join(here, 'requirements-dev.txt')) as requirements_dev_fp:
dev_install_requires = requirements_dev_fp.read().split('\n')

Expand Down
6 changes: 2 additions & 4 deletions sigopt/orchestrate/template/service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,6 @@ class TemplateService(Service):
def _dockerfile_escape(self, s):
return s.replace('\\', '\\\\').replace('\n', '\\\n')

def _escape_args(self, args, escape):
return {k: escape(v) for k, v in args.items()}

def render_dockerfile_template_from_file(self, relative_filename, template_args):
return self._raw_render_template_from_file(self._dockerfile_escape, relative_filename, template_args)

Expand All @@ -23,5 +20,6 @@ def _raw_render_template_from_file(self, escape, relative_filename, template_arg
template = self.services.resource_service.read('template', relative_filename).decode("utf-8")
return chevron.render(
template=template,
data=self._escape_args(template_args, escape),
data=template_args,
escape=escape,
)
2 changes: 0 additions & 2 deletions test/orchestrate/template/service_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,15 +32,13 @@ def test_dockerfile_escape_newline(self, template_service):
# Ensure that the newline is preceded by a backslash, so Dockerfile doesn't interpret it as a new command
assert 'ENV SIGOPT_HOME "\\\nCOPY ."' in rendered

@pytest.mark.skip(reason="chevron doesn't handle quotes gracefully")
def test_dockerfile_escape_backslash(self, template_service):
rendered = template_service.render_dockerfile_template_from_file('model_packer/Dockerfile.ms', dict(
sigopt_home='echo ""\\',
))
# Ensure that the trailing backslash is escaped, and not interpreted as an escape sequence for the newline
assert 'ENV SIGOPT_HOME "echo ""\\\\"\n' in rendered

@pytest.mark.skip(reason="chevron doesn't handle quotes gracefully")
def test_yaml_escape_quotes(self, template_service):
rendered = template_service.render_yaml_template_from_file('test.yml.ms', dict(
endpoint_url='"', base64_encoded_ca_cert='"',
Expand Down