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

jinja 2.10 compat: add missing built-in tests #64342

Closed

Conversation

jamescassell
Copy link
Contributor

@jamescassell jamescassell commented Nov 2, 2019

SUMMARY

fixed ansible template failures on older jinja due to missing built-in tests

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

jinja tests

ADDITIONAL INFORMATION

Playbooks and roles written on ansible 2.8 on a modern would fail if run with ansible 2.8 on RHEL 7 due to missing tests on that platform.

relevant jinja PR: pallets/jinja#665


@ansibot ansibot added affects_2.10 This issue/PR affects Ansible v2.10 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. needs_triage Needs a first human triage before being processed. new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests. labels Nov 2, 2019
@ansibot
Copy link
Contributor

ansibot commented Nov 2, 2019

The test ansible-test sanity --test pep8 [explain] failed with 17 errors:

lib/ansible/plugins/test/compat_jinja_2_10.py:14:1: E302: expected 2 blank lines, found 1
lib/ansible/plugins/test/compat_jinja_2_10.py:22:1: E302: expected 2 blank lines, found 1
lib/ansible/plugins/test/compat_jinja_2_10.py:29:18: E241: multiple spaces after ':'
lib/ansible/plugins/test/compat_jinja_2_10.py:30:18: E241: multiple spaces after ':'
lib/ansible/plugins/test/compat_jinja_2_10.py:31:18: E241: multiple spaces after ':'
lib/ansible/plugins/test/compat_jinja_2_10.py:32:23: E241: multiple spaces after ':'
lib/ansible/plugins/test/compat_jinja_2_10.py:33:18: E241: multiple spaces after ':'
lib/ansible/plugins/test/compat_jinja_2_10.py:34:18: E241: multiple spaces after ':'
lib/ansible/plugins/test/compat_jinja_2_10.py:35:17: E241: multiple spaces after ':'
lib/ansible/plugins/test/compat_jinja_2_10.py:36:18: E241: multiple spaces after ':'
lib/ansible/plugins/test/compat_jinja_2_10.py:38:18: E241: multiple spaces after ':'
lib/ansible/plugins/test/compat_jinja_2_10.py:39:18: E241: multiple spaces after ':'
lib/ansible/plugins/test/compat_jinja_2_10.py:40:17: E241: multiple spaces after ':'
lib/ansible/plugins/test/compat_jinja_2_10.py:41:18: E241: multiple spaces after ':'
lib/ansible/plugins/test/compat_jinja_2_10.py:42:24: E241: multiple spaces after ':'
lib/ansible/plugins/test/compat_jinja_2_10.py:43:18: E241: multiple spaces after ':'
lib/ansible/plugins/test/compat_jinja_2_10.py:44:18: E241: multiple spaces after ':'

click here for bot help

@ansibot ansibot added 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. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Nov 2, 2019
@felixfontein
Copy link
Contributor

Would be nice to have that. I had to add a local test plugin for some integration tests to get them working with RHEL7 (like https://github.com/ansible/ansible/blob/devel/test/integration/targets/openssl_csr_info/test_plugins/jinja_compatibility.py).

@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Nov 2, 2019
@jamescassell jamescassell reopened this Nov 2, 2019
@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 Nov 3, 2019
@bcoca bcoca self-assigned this Nov 5, 2019
@bcoca bcoca removed the needs_triage Needs a first human triage before being processed. label Nov 5, 2019
@samdoran
Copy link
Contributor

samdoran commented Nov 5, 2019

Per discussion in the IRC meeting today, please add a test that only uses this if the Jinja version is missing these tests.

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. core_review In order to be merged, this PR must follow the core review workflow. and removed core_review In order to be merged, this PR must follow the core review workflow. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Nov 6, 2019
Copy link
Contributor

@samdoran samdoran left a comment

Choose a reason for hiding this comment

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

Please add integration tests.

# -*- coding: utf-8 -*-

# Copyright (c) 2017, the Jinja Team
# BSD 3-Clause "New" or "Revised" License (see https://opensource.org/licenses/BSD-3-Clause)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this is compatible with the GPLv3 we use in Ansible.

Copy link

Choose a reason for hiding this comment

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

Jijna itself is licensed with BSD 3-Clause (new BSD?) [0].

[0]: https://github.com/pallets/jinja/blob/master/LICENSE.rst

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if this is compatible with the GPLv3 we use in Ansible

Looked it up: https://en.m.wikipedia.org/wiki/License_compatibility#GPL_compatibility

Many of the most common free-software licenses, especially the permissive licenses, such as the original MIT/X license, BSD licenses (in the three-clause and two-clause forms, though not the original four-clause form), MPL 2.0, and LGPL, are GPL-compatible.

Hence, I can apply the GPLv3 label of it helps get it merged.

changelogs/fragments/compat-jinja-2.10-test.yml Outdated Show resolved Hide resolved
@ansibot ansibot removed the core_review In order to be merged, this PR must follow the core review workflow. label Nov 12, 2019
@ansibot ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html and removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old. labels Jun 9, 2020
@ansibot
Copy link
Contributor

ansibot commented Jun 11, 2020

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

docs/docsite/rst/user_guide/intro_inventory.rst:183:0: duplicate-label: duplicate label variables_in_inventory, other instance in /root/ansible/docs/docsite/rst/user_guide/playbooks_variables.rst

click here for bot help

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. core_review In order to be merged, this PR must follow the core review workflow. labels Jun 11, 2020
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. core_review In order to be merged, this PR must follow the core review workflow. labels Jun 13, 2020
@jamescassell
Copy link
Contributor Author

bot_status

@ansibot
Copy link
Contributor

ansibot commented Jun 14, 2020

Components

changelogs/fragments/compat-jinja-2.10-test.yml
support: community
maintainers:

lib/ansible/plugins/test/compat_jinja_2_10.py
support: core
maintainers:

lib/ansible/plugins/test/compat_jinja_2_11.py
support: core
maintainers:

test/integration/targets/test_core/tasks/main.yml
support: community
maintainers:

Metadata

waiting_on: jamescassell
changes_requested_by: samdoran
needs_info: False
needs_revision: True
needs_rebase: False
merge_commits: []
too many files or commits: False
mergeable_state: clean
shippable_status: success
maintainer_shipits (module maintainers): False
community_shipits (namespace maintainers): False
ansible_shipits (core team members): False
shipit_actors (maintainer or core team member): None
shipit_actors_other:
automerge: automerge shipit test failed

click here for bot help

@samdoran
Copy link
Contributor

Rather than adding these directly to Ansible, use the sivel.jinja2 collection.

After the 2.10 release, we will look at making that an officially supported collection. This would allow us to update the Jinja filters and tests much faster than if we add this directly to ansible-base.

To discuss this further, please reach out on IRC or the mailing list:

   * IRC: #ansible-devel on irc.freenode.net  
   * mailing list: https://groups.google.com/forum/#!forum/ansible-devel

@samdoran samdoran closed this Jun 15, 2020
@ansible ansible locked and limited conversation to collaborators Jul 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.10 This issue/PR affects Ansible v2.10 bug This issue/PR relates to a bug. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community. 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

7 participants