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

Add assert filter #22529

Closed
wants to merge 1 commit into
base: devel
from

Conversation

Projects
None yet
4 participants
@AndiDog

AndiDog commented Mar 11, 2017

SUMMARY

Introduces a new Jinja template filter assert to fail on falsy expression or otherwise resolve to nothing (empty string).

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

assert [new filter plugin]

ANSIBLE VERSION
ansible 2.2.1.0
ADDITIONAL INFORMATION

Example use case (there are many others): for a missing dict key, Ansible doesn't give great error messages nor location, and even if it did, you may want to provide your own error message.

Usage:

user_uid: '{{ (username in dict_of_uids)|assert(username + " not found in dict_of_uids, please add that user") }}{{ dict_of_uids[username] }}'

On successful assertion, the filter resolves to an empty string for obvious reasons.

Output:

TASK [test assertion] **********************************************************
fatal: [somehostname]: FAILED! => {"failed": true, "msg": "assertion failed: myuser not found in dict_of_uids"}
@bcoca

This comment has been minimized.

Member

bcoca commented Mar 13, 2017

how is this different than the mandatory fitler?

@bcoca bcoca added the needs_info label Mar 13, 2017

@AndiDog

This comment has been minimized.

AndiDog commented Mar 20, 2017

The mandatory filter applies to hopefully very few people only, as Ansible's safe default is to always check for variable existence. My filter can check "truthiness" of any expression, not just variables.

@ansibot ansibot added affects_2.4 and removed needs_info labels Mar 20, 2017

@sivel

This comment has been minimized.

Member

sivel commented Mar 20, 2017

That sounds more like a "test" and not a filter. Also I am against calling it assert, as that will cause confusion with the assert module.

@AndiDog

This comment has been minimized.

AndiDog commented Mar 20, 2017

@sivel you're right about the name clash. The filter is something like the assert module, but "inline". I explicitly called it "assert", not "test" (or similar) because assertions are known to fail and stop the whole program if the expression isn't true, while e.g. test -e /some/path will give a result instead of stopping & failing Jinja template execution.

Any name suggestions?

@sivel

This comment has been minimized.

Member

sivel commented Mar 20, 2017

I think that the mandatory filter should just be updated to support this. Using a kwarg, that activates this functionality. Just like the default filter has the boolean=False kwarg to activate a boolean style check instead of just undefined.

Something like:

diff --git a/lib/ansible/plugins/filter/core.py b/lib/ansible/plugins/filter/core.py
index f109c3e..ad79f0a 100644
--- a/lib/ansible/plugins/filter/core.py
+++ b/lib/ansible/plugins/filter/core.py
@@ -278,11 +278,11 @@ def get_encrypted_password(password, hashtype='sha512', salt=None):
 def to_uuid(string):
     return str(uuid.uuid5(UUID_NAMESPACE_ANSIBLE, str(string)))
 
-def mandatory(a):
+def mandatory(a, boolean=False):
     from jinja2.runtime import Undefined
 
     ''' Make a variable mandatory '''
-    if isinstance(a, Undefined):
+    if isinstance(value, Undefined) or (boolean and not value):
         raise errors.AnsibleFilterError('Mandatory variable not defined.')
     return a
@AndiDog

This comment has been minimized.

AndiDog commented Mar 20, 2017

My use case is to create a filter that explicitly has no output, but only does validation. Therefore it can accept any bool-testable expression without inserting the result into the text (I guess you don't want True/False to show up in generated text 😉).

@sivel

This comment has been minimized.

Member

sivel commented Mar 20, 2017

Yeah, I will say I don't like your approach in the slightest. In my view the filter should:

  1. fail if the condition is false
  2. return the passed in value if the condition is true

It shouldn't just be a magical thing you stick somewhere.

So it should work like:

foo|mandatory(some_condition)

Where it either fails, or returns the contents of foo

However, the more I think about it, the more I think you should just use the assert module before your task that attempts to use the data. This functionality basically exists now, and can work without any changes.

- assert:
    that:
        - username in dict_of_uids
    msg:  "{{ username }} not found in dict_of_uids, please add that user"
@AndiDog

This comment has been minimized.

AndiDog commented Mar 20, 2017

Fair points. If you don't see it in core, then I have to be fine with that. Using mandatory that way wouldn't work for the use cases I had in mind (such as dict lookup) because the first lookup will already fail before even going into the filter (I guess: dict_of_uids[username]|mandatory(username in dict_of_uids)). It also won't provide a clear/custom error message. In my view, the advantage of "inlining" is to have the condition right where it is used, and the filter can be used in Jinja template files, unlike the assert module which can only be used in Ansible tasks (i.e. roles/playbooks). We use configuration files everywhere (nginx, services, what not, ...) so having such a filter for those files is much more important than in a single line of Ansible playbook. Nevertheless, since you don't like the idea, I'm closing this and keeping the filter in our local repository.

@AndiDog AndiDog closed this Mar 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment