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

Don't overwrite builtin jinja2 filters with tests #37881

Merged
merged 2 commits into from Mar 26, 2018

Conversation

sivel
Copy link
Member

@sivel sivel commented Mar 24, 2018

SUMMARY

Don't overwrite builtin jinja2 filters with tests. Fixes #37856

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

lib/ansible/template/init.py

ANSIBLE VERSION
2.5
2.6
ADDITIONAL INFORMATION

@ansibot ansibot added bug This issue/PR relates to a bug. needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Mar 24, 2018
@ansibot ansibot added test This PR relates to tests. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Mar 24, 2018
@maxamillion maxamillion merged commit 1f824bd into ansible:devel Mar 26, 2018
@sivel sivel removed the needs_triage Needs a first human triage before being processed. label Mar 26, 2018
sivel added a commit to sivel/ansible that referenced this pull request Mar 26, 2018
* Don't overwrite builtin jinja2 filters with tests. Fixes ansible#37856

* Fix tests and other callers of _get_filters

(cherry picked from commit 1f824bd)
sivel added a commit to sivel/ansible that referenced this pull request Mar 26, 2018
@@ -176,7 +176,7 @@ def generic_visit(self, node, inside_call=False, inside_yield=False):
)
try:
e = templar.environment.overlay()
e.filters.update(templar._get_filters())
e.filters.update(templar._get_filters(e.filters))
Copy link
Contributor

Choose a reason for hiding this comment

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

could reverse the update ordering and update templar._get_filters() (or a copy...) with e.filters so that
the e.filters 'wins'

Copy link
Contributor

Choose a reason for hiding this comment

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

something like

custom_filters = templar._get_filters()
custom_filters.update(e.filters)
e.filters = custom_filters

Copy link
Member Author

Choose a reason for hiding this comment

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

We may want to override a builtin filter with an ansible provided by filter, such as groupby. We just don't want this to happen with custom tests.

Eventually custom tests will no longer be registered as filters (2.9), but have been kept thus far for backwards compat.

sivel added a commit that referenced this pull request Mar 28, 2018
* Don't overwrite builtin jinja2 filters with tests (#37881)

* Don't overwrite builtin jinja2 filters with tests. Fixes #37856

* Fix tests and other callers of _get_filters

(cherry picked from commit 1f824bd)

* Add changelog for #37881
ryancurrah pushed a commit to ryancurrah/ansible that referenced this pull request Apr 4, 2018
* Don't overwrite builtin jinja2 filters with tests. Fixes ansible#37856

* Fix tests and other callers of _get_filters
doubleplush pushed a commit to doubleplush/ansible that referenced this pull request Apr 10, 2018
The abs test for absolute paths overwrites the built-in Jinja2 abs
filter, preventing the absolute value of numbers from being taken and
breaking existing code which makes use of the absolute value filter.

Superceded by ansible#37881 in Ansible 2.6.
@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
bug This issue/PR relates to a bug. 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.

Ansible 2.5.0: jinja2 abs-filter triggers AttributeError and deprecation warning
4 participants