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

template: Add option to lstrip_blocks' and fix settingtrim_blocks` inline #37478

Merged
merged 5 commits into from Mar 23, 2018

Conversation

alextsits
Copy link
Contributor

@alextsits alextsits commented Mar 15, 2018

lstrip_blocks_test.zip

trim_blocks_test.zip

SUMMARY

Add option to set lstrip_blocks when using the template module to render Jinja templates. The Jinja documentation suggests that trim_blocks and lstrip_blocks is a great combination and the template module already provides an option for `trim_blocks'.

Note that although trim_blocks in Ansible is enabled by default since version 2.4, in order to avoid breaking things keep lstrip_blocks disabled by default. Maybe in a future version it could be enabled by default.

Also fix the trim_blocks argument to make it possible to set it to False inline.

This seems to address issue #10725 in a more appropriate way than the
suggested.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

template

ANSIBLE VERSION
ansible 2.6.0
  config file = None
  configured module search path = [u'/home/alex/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /home/alex/git/ansible-ve/lib/python2.7/site-packages/ansible-2.6.0-py2.7.egg/ansible
  executable location = /home/alex/git/ansible-ve/bin/ansible
  python version = 2.7.14 (default, Jan  5 2018, 10:41:29) [GCC 7.2.1 20171224]

ADDITIONAL INFORMATION

Attached is a simple test. The zip file contains the playbook lstrip_blocks_test.yml and the template test.j2. The playbook contains two tasks. The first task should render the template without setting the lstrip_blocks option, creating the file test_default in the working directory.
The second task should render the template with the lstrip_blocks options set to True, creating the test_with_lstrip_blocks file in the working directory.

To run the playbook simply run ansible-playbook lstrip_blocks_test.yml.

To test the trim_blocks inline option, we provide another test. The zip file contains a playbook trim_blocks_test.yml and the template trim_blocks.j2. The playbook has a single task, to render the trim_blocks.j2 template with trim_blocks set to False, creating the file trim_blocks.templated in the working directory. Before this PR the result is the following:

Hello World
Goodbye

After the PR the result is the following (which is the correct and expected):


Hello World

Goodbye

To run the playbook simply run ansible-playbook trim_blocks_test.yml.

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. feature This issue/PR relates to a feature request. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Mar 15, 2018
@samdoran
Copy link
Contributor

This looks good. Please add integration tests and I'll merge it.

@samdoran samdoran removed the needs_triage Needs a first human triage before being processed. label Mar 15, 2018
@alextsits
Copy link
Contributor Author

Added integration tests as requested.

@ansibot ansibot added the test This PR relates to tests. label Mar 15, 2018
@alextsits
Copy link
Contributor Author

Please do not merge this yet. It seems that lstip_blocks, just like trim_blocks does not work as expected when passed as inline argument to the template module. Will update the PR, and will fix trim_blocks too.

@alextsits alextsits force-pushed the templates_lstrip_blocks_option branch 2 times, most recently from 72bb9e2 to 65ef531 Compare March 15, 2018 23:40
@alextsits
Copy link
Contributor Author

Fixed trim_blocks argument, added appropriate integration tests for it too.
Also updated the PR description and added an attachment to easily reproduce the trim_blocks bug. I'm done with changes, ready to merge.

@alextsits alextsits changed the title template: Add option to `lstrip_blocks' template: Add option to lstrip_blocks' and fix settingtrim_blocks` inline Mar 15, 2018
@alextsits alextsits force-pushed the templates_lstrip_blocks_option branch from 65ef531 to f8f0255 Compare March 16, 2018 00:10
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Mar 16, 2018
@alextsits
Copy link
Contributor Author

It seems to me that tests fail on centos6 because the lstrip_blocks option is available from Jinja2 version 2.7 and later, and centos6 has an older version installed. If this is true, what is the correct way to deal with this? Should I skip lstrip_blocks tests if Jinja2 version in <2.7?

@samdoran
Copy link
Contributor

Thanks for adding tests!

Raise an AnsibleError exception if lstrip_blocks is True but the Jinja2 version is < 2.7. If you silently skip it, the generated file could be different than what the playbook author intended, potentially creating a misconfigured file.

Also, please state in the option description that it requires Jinja2 >= 2.7.

# VERIFY trim_blocks

- name: Render a template with "trim_blocks" set to False
template: src=trim_blocks.j2 dest={{output_dir}}/trim_blocks_false.templated trim_blocks=False
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use multi-line YAML rather than k=v syntax. I know the rest of the tasks use k=v syntax, but they need to be changed at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted, it's just that my commit for trim_blocks fixes exactly that, that the k=v syntax for trim_blocks was not working. I will gladly change to multi-line syntax though if you insist.

copy: src=trim_blocks_false.expected dest={{output_dir}}/trim_blocks_false.expected

- name: compare templated file to known good trim_blocks_false
shell: diff -u {{output_dir}}/trim_blocks_false.templated {{output_dir}}/trim_blocks_false.expected
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than a shell task, you can use the stat module and assert that the checksum matches. See lineinfile tests for examples.

@alextsits alextsits force-pushed the templates_lstrip_blocks_option branch from f8f0255 to ac8d1ec Compare March 19, 2018 13:11
@ansibot ansibot added the needs_info This issue requires further information. Please answer any outstanding questions. label Mar 19, 2018
@alextsits
Copy link
Contributor Author

Added a new commit to check for Jinja2 version.
Also updated the commits that add integration tests to implement the requested changes.

Note that while assertion in tests is now done with checksum, the checksum is not hardcoded in tests. The "expected" files still remain, in case anything goes wrong to know what the expected output actually is.

It also seems to me that github did not get the commits in the right order? Possibly due to overwriting and force-pushing? The commits in the branch should be in the correct order nevertheless.

Ready for review.

@alextsits alextsits force-pushed the templates_lstrip_blocks_option branch from ac8d1ec to 6bf83ae Compare March 19, 2018 16:33
@ansibot ansibot removed the needs_info This issue requires further information. Please answer any outstanding questions. label Mar 19, 2018
@alextsits alextsits force-pushed the templates_lstrip_blocks_option branch 2 times, most recently from d292362 to a8744fc Compare March 19, 2018 19:32
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Mar 19, 2018
@ansibot ansibot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. and removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. labels Mar 21, 2018
@ansibot
Copy link
Contributor

ansibot commented Mar 21, 2018

The test ansible-test sanity --test no-tests-as-filters [explain] failed with 3 errors:

test/integration/targets/template/tasks/main.yml:196:9: use `"lstrip_block_support is failed` instead of `"lstrip_block_support | failed`
test/integration/targets/template/tasks/main.yml:203:9: use `"lstrip_block_support is successful` instead of `"lstrip_block_support | succeeded`
test/integration/targets/template/tasks/main.yml:209:9: use `"lstrip_block_support is successful` instead of `"lstrip_block_support | succeeded`

The test ansible-test sanity --test yamllint [explain] failed with 1 error:

test/integration/targets/template/tasks/main.yml:203:3: key-duplicates duplication of key "when" in mapping

click here for bot help

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Mar 21, 2018
@alextsits alextsits force-pushed the templates_lstrip_blocks_option branch from 5207149 to 185b495 Compare March 21, 2018 10:05
@ansibot
Copy link
Contributor

ansibot commented Mar 21, 2018

The test ansible-test sanity --test yamllint [explain] failed with 1 error:

test/integration/targets/template/tasks/main.yml:203:3: key-duplicates duplication of key "when" in mapping

click here for bot help

@alextsits alextsits force-pushed the templates_lstrip_blocks_option branch 2 times, most recently from e5ea8f0 to 18122ca Compare March 21, 2018 22:38
@mattclay
Copy link
Member

CI failure in integration tests: https://app.shippable.com/github/ansible/ansible/runs/58672/40/tests

test/integration/targets/template/tasks/main.yml:204 / [testhost] testhost: template : Verify templated lstrip_blocks_true matches known good using checksum that=[u'lstrip_blocks_true_result.checksum == lstrip_blocks_true_good.stat.checksum']

failure: The conditional check 'lstrip_blocks_true_result.checksum == lstrip_blocks_true_good.stat.checksum' failed. The error was: error while evaluating conditional (lstrip_blocks_true_result.checksum == lstrip_blocks_true_good.stat.checksum): 'dict' object has no attribute 'checksum'

{
"msg": "The conditional check 'lstrip_blocks_true_result.checksum == lstrip_blocks_true_good.stat.checksum' failed. The error was: error while evaluating conditional (lstrip_blocks_true_result.checksum == lstrip_blocks_true_good.stat.checksum): 'dict' object has no attribute 'checksum'"
}

@mattclay mattclay added the ci_verified Changes made in this PR are causing tests to fail. label Mar 23, 2018
@alextsits
Copy link
Contributor Author

alextsits commented Mar 23, 2018

This is why the integration tests fail:

The command inside the template.py module that results in an exception is the following:

try:
    jinja2.defaults.LSTRIP_BLOCKS
except AttributeError:
    raise AnsibleError("Option `lstrip_blocks' is only available in Jinja2 versions >=2.7")

This is an expected behavior.

The same command in the main.yml succeeds, resulting in skipping other tests than the expected:

- name: Check support for lstrip_blocks in Jinja2
  shell: python -c 'import jinja2; jinja2.defaults.LSTRIP_BLOCKS'
  register: lstrip_block_support
  ignore_errors: True

I had also noticed that previously when we attempted to access the jinja2.__version__ attribute, in the template.py module the value unknown was returned, while in main.yml the attribute had the value 2.10.

I don't know why the YAML and the Python module use different environments, I haven't checked the exact setup used to run the tests, yet.

Also note that this happens only in the centos6 tests. Same goes for the jinja2.__version__ attribute.

@jsumners
Copy link
Contributor

I really wish this could be set to true by default on a global basis. Ideally I'd be able to put the setting in my ansible.cfg and forget about it.

# VERIFY lstrip_blocks

- name: Check support for lstrip_blocks in Jinja2
shell: python -c 'import jinja2; jinja2.defaults.LSTRIP_BLOCKS'
Copy link
Member

Choose a reason for hiding this comment

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

Use {{ ansible_python_interpreter }} instead of python.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, this indeed did the trick. I just did grep -R jinja2.__version__ in test and saw another shell: module using plain python -c. My bad.

Copy link
Member

Choose a reason for hiding this comment

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

Sometimes our existing tests do things that they shouldn't, such as hard-coding references to python instead of using {{ ansible_python_interpreter }}.

Copy link
Contributor

Choose a reason for hiding this comment

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

The reason this fixes the test failure is because there is another integration test, template_jinja2_latest, which sets up a virtualenv with a newer version of Jinja2. During that test, the shell task calling python executes to the Python version inside the virtualenv, which passes the Jinja2 version check. But the Python being used by Ansible is /usr/bin/python, which would fail the Jinja2 version test and doesn't support lstrip_blocks.

This causes the "Render a template with "lstrip_blocks" set to True" task to run but fail when in should be skipped, making checksum undefined in lstrip_blocks_true_result.

Fix passing `trim_blocks' option to the template module as inline
argument. Previously passing the `trim_blocks' option inline instead of
using the YAML dictionary format resulted in it always being set to
`True', even if `trim_blocks=False' was used.

Signed-off-by: Alex Tsitsimpis <alextsi@arrikto.com>
Add option to set `lstrip_blocks' when using the template module to
render Jinja templates. The Jinja documentation suggests that
`trim_blocks' and `lstrip_blocks' is a great combination and the
template module already provides an option for `trim_blocks'.

Note that although `trim_blocks' in Ansible is enabled by default since
version 2.4, in order to avoid breaking things keep `lstrip_blocks'
disabled by default. Maybe in a future version it could be enabled by
default.

This seems to address issue ansible#10725 in a more appropriate way than the
suggested.

Signed-off-by: Alex Tsitsimpis <alextsi@arrikto.com>
Since the `lstrip_blocks' option was added in Jinja2 version 2.7, raise
an exception when `lstrip_blocks' is set but Jinja2 does not support it.
Check support for `lstrip_blocks' option by checking `jinja2.defaults'
for `LSTRIP_BLOCKS' and do not use `jinja2.__version__' because the
latter is set to `unknown' in some cases, perhaps due to bug in
`pkg_resources' in Python 2.6.6.

Also update option description to state that Jinja2 version >=2.7 is
required.

Signed-off-by: Alex Tsitsimpis <alextsi@arrikto.com>
Signed-off-by: Alex Tsitsimpis <alextsi@arrikto.com>
Signed-off-by: Alex Tsitsimpis <alextsi@arrikto.com>
@alextsits alextsits force-pushed the templates_lstrip_blocks_option branch from 18122ca to 073235a Compare March 23, 2018 15:02
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Mar 23, 2018
@samdoran samdoran merged commit c3ab6cb into ansible:devel Mar 23, 2018
@ansible ansible locked and limited conversation to collaborators Apr 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
core_review In order to be merged, this PR must follow the core review workflow. feature This issue/PR relates to a feature request. module This issue/PR relates to a module. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants