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

Replacing selectattr('tls', 'true'|'false') with selectattr('tls', '==', true|false) #169

Merged
merged 1 commit into from
Aug 20, 2020

Conversation

nhosoi
Copy link
Contributor

@nhosoi nhosoi commented Aug 18, 2020

Note: In the jinja2 doc:
true(value) and false(value) are "New in version 2.11."
Plus, eq is not supported in Jinja 2.7.2.

@pcahyna
Copy link
Member

pcahyna commented Aug 19, 2020

I am afraid that this will not work in older Jinja2: linux-system-roles/storage#49

@nhosoi
Copy link
Contributor Author

nhosoi commented Aug 19, 2020

I am afraid that this will not work in older Jinja2: linux-system-roles/storage#49

(;_;) Thanks for your input, @pcahyna. Replacing 'eq' with '=='.

@nhosoi nhosoi changed the title Replacing selectattr('tls', 'true'|'false') with selectattr('tls', 'e… Replacing selectattr('tls', 'true'|'false') with selectattr('tls', '==', True|False) Aug 19, 2020
@pcahyna
Copy link
Member

pcahyna commented Aug 19, 2020

@nhosoi please verify that it actually works with Jinja 2.7, you can use RHEL 7 or CentOS 7 as a control host for that

@richm
Copy link
Collaborator

richm commented Aug 19, 2020

@nhosoi please verify that it actually works with Jinja 2.7, you can use RHEL 7 or CentOS 7 as a control host for that

I don't think it works there either. In fact, I think there is a lot of code that won't work with jinja2 2.7. I think selectattr('foo','==','something') will not work with jinja2 2.7.

One easy way to test is with a venv:
python3 -mvenv .venv
. .venv/bin/activate
pip install 'ansible<2.9' 'jinja2<2.8'
ansible-playbook -vv some-playbook-that-uses-selectattr-and-==.yml

@nhosoi
Copy link
Contributor Author

nhosoi commented Aug 19, 2020

Thanks, @richm. Confirmed selectattr('foo','==','something') does not work with ansible 2.8.14 & Jinja2 2.7.3.

TASK [linux-system-roles.logging : Set rsyslog_output_elasticsearch for the omelasticsearch output] ***
task path: /home/nhosoi/linux-system-roles/logging/tests/roles/linux-system-roles.logging/tasks/main.yml:5
An exception occurred during task execution. To see the full traceback, use -vvv. The error was: jinja2.exceptions.TemplateRuntimeError: no test named '=='
fatal: [/home/nhosoi/Downloads/CentOS-7-x86_64-GenericCloud.qcow2c]: FAILED! => {}
MSG:
Unexpected failure during module execution.

Sadly, replacing == with equalto did not solve the error, either.

Logging role is the one that uses selectattr most (not just in this PR, but it's been in the code for a long time...). Should we replace it with something backward compatible???

But I think @richm is right. There should be more cases which do not work with ansible 2.8.14 & Jinja2 2.7.3. For instance, I ran certificate/tests_basic_self_signed.yml and ran it with ansible 2.8.14 & Jinja2 2.7.3, which failed like this. Even namespace is not available in jinja2 2.7...

TASK [linux-system-roles.certificate : Ensure provider packages are installed] ********************
task path: /home/nhosoi/linux-system-roles/certificate/tasks/main.yml:34
fatal: [/home/nhosoi/Downloads/CentOS-7-x86_64-GenericCloud.qcow2c]: FAILED! => {}
MSG:
'namespace' is undefined

@nhosoi
Copy link
Contributor Author

nhosoi commented Aug 20, 2020

BTW, I'm wondering why this molecule failure has started. I can reproduce it with "tox -e molecule". Again, this is likely a version issue since if I run "tox -e molecule" with ansible 2.8.14 &Jinja2 2.7.3, I don't see this error...

--> Executing Ansible Lint on /home/travis/build/linux-system-roles/logging/tests/tests_default.yml...
Traceback (most recent call last):
  File "/home/travis/build/linux-system-roles/logging/.tox/env-3.6/bin/ansible-lint", line 8, in <module>
    sys.exit(main())
  File "/home/travis/build/linux-system-roles/logging/.tox/env-3.6/lib/python3.6/site-packages/ansiblelint/__main__.py", line 123, in main
    options.verbosity, checked_files)
  File "/home/travis/build/linux-system-roles/logging/.tox/env-3.6/lib/python3.6/site-packages/ansiblelint/runner.py", line 43, in __init__
    self._update_exclude_paths(exclude_paths)
  File "/home/travis/build/linux-system-roles/logging/.tox/env-3.6/lib/python3.6/site-packages/ansiblelint/runner.py", line 52, in _update_exclude_paths
    paths = ansiblelint.utils.expand_paths_vars(exclude_paths)
  File "/home/travis/build/linux-system-roles/logging/.tox/env-3.6/lib/python3.6/site-packages/ansiblelint/utils.py", line 783, in expand_paths_vars
    paths = [expand_path_vars(p) for p in paths]
  File "/home/travis/build/linux-system-roles/logging/.tox/env-3.6/lib/python3.6/site-packages/ansiblelint/utils.py", line 783, in <listcomp>
    paths = [expand_path_vars(p) for p in paths]
  File "/home/travis/build/linux-system-roles/logging/.tox/env-3.6/lib/python3.6/site-packages/ansiblelint/utils.py", line 775, in expand_path_vars
    path = path.strip()
AttributeError: 'PosixPath' object has no attribute 'strip'
ERROR: InvocationError for command /bin/bash .travis/runsyspycmd.sh molecule lint -s default (exited with code 1)
___________________________________ summary ____________________________________
ERROR:   molecule: commands failed

@pcahyna
Copy link
Member

pcahyna commented Aug 20, 2020

@nhosoi, concerning the last problem: @i386x said in chat: ansible-lint issue is fixed in upstream now: ansible/ansible-lint@5b5c000

Copy link
Member

@pcahyna pcahyna left a comment

Choose a reason for hiding this comment

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

Ready to merge? This will solve compatibility with Jinja 2.10 and 2.7 will have to be solved later.

@pcahyna
Copy link
Member

pcahyna commented Aug 20, 2020

one more comment: true and false in Jinja 2 are lowercase, although upercase versions work as well, they are not preferred. https://jinja.palletsprojects.com/en/2.10.x/templates/#literals

@nhosoi
Copy link
Contributor Author

nhosoi commented Aug 20, 2020

Ready to merge? This will solve compatibility with Jinja 2.10 and 2.7 will have to be solved later.

If that's the case, we could close this pr and live with selectattr('tls', 'true'|'false'), right? If ok with you, closing this.

@pcahyna
Copy link
Member

pcahyna commented Aug 20, 2020

Ready to merge? This will solve compatibility with Jinja 2.10 and 2.7 will have to be solved later.

If that's the case, we could close this pr and live with selectattr('tls', 'true'|'false'), right? If ok with you, closing this.

No, we need to support 2.10, the true/false tests were added in 2.11. Or have you verified that the true/false tests work in RHEL 8?

Copy link
Member

@pcahyna pcahyna left a comment

Choose a reason for hiding this comment

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

true and false should be lowercase

…=', true|false).

In the jinja2 doc:
true(value) and false(value) are "New in version 2.11."
@nhosoi nhosoi changed the title Replacing selectattr('tls', 'true'|'false') with selectattr('tls', '==', True|False) Replacing selectattr('tls', 'true'|'false') with selectattr('tls', '==', true|false) Aug 20, 2020
Copy link
Collaborator

@richm richm left a comment

Choose a reason for hiding this comment

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

lgtm

@nhosoi nhosoi merged commit 238a748 into linux-system-roles:master Aug 20, 2020
@nhosoi
Copy link
Contributor Author

nhosoi commented Aug 20, 2020

Thank you, @richm. Merged!

@nhosoi nhosoi deleted the selectattr branch August 27, 2020 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants