New Jinja2 filters: hash_merge & hash_replace #7872

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
9 participants
@darkk
Contributor

darkk commented Jun 20, 2014

Link to intro_configuration.html#hash-behaviour is coded without :doc: prefix as I've not managed to create proper link with anchor for sphinx.

This code was inspired by PR #7629

New Jinja2 filters: hash_merge & hash_replace
Link to intro_configuration.html#hash-behaviour is coded without :doc:
prefix as I've not managed to create proper link with anchor for sphinx.

@jimi-c jimi-c added P3 labels Jun 20, 2014

@mpdehaan

This comment has been minimized.

Show comment
Hide comment
@mpdehaan

mpdehaan Jun 20, 2014

Contributor

Thanks.

I'd prefer to see this documented without reference to the ansible.cfg setting at all, but still need to think about these.

Contributor

mpdehaan commented Jun 20, 2014

Thanks.

I'd prefer to see this documented without reference to the ansible.cfg setting at all, but still need to think about these.

@mpdehaan

This comment has been minimized.

Show comment
Hide comment
@mpdehaan

mpdehaan Jun 20, 2014

Contributor

I'd also like to see the examples to be a bit more real world rather than the Mortal Kombat references, so it's clear when they might be used.

Contributor

mpdehaan commented Jun 20, 2014

I'd also like to see the examples to be a bit more real world rather than the Mortal Kombat references, so it's clear when they might be used.

@jirutka

This comment has been minimized.

Show comment
Hide comment
@jirutka

jirutka Jun 20, 2014

Contributor

I like Mortal Kombat references! 😃

Contributor

jirutka commented Jun 20, 2014

I like Mortal Kombat references! 😃

@darkk

This comment has been minimized.

Show comment
Hide comment
@darkk

darkk Jun 20, 2014

Contributor

@mpdehaan I referenced settings value as it's sort of "compatibility" filter that inherits all bugs of dict merging implementation. I'm unsure what is better: fix bugs in merge_hash that leads to NoneType dereference or leave warning sign about possible hash_merge hazard - that's what mortal example is about.

I try to use (abuse?) ansible as scripting language to generate monitoring system configuration as ansible is already used as deploy tool and having single DSL to learn is a bit better than two different alike DSLs. I use dict merging to provide different sets of default values for different checks and nested structures are used for syntax sugar when

http:
  path: /ping
  disable_ipv6: yes

is used as shortcut for

active: http
active_args:
  path: /ping
  disable_ipv6: yes

In this case hash_merge is used to provide cluster-wide variable that controls disable_ipv6 setting and leaves /path service-specific. I'm unsure if this real-world example is useful as juggler (monitoring system I am speaking about) is not released yet.

I dive into ansible-project@ mailing list a bit and found several requests for different merge behavior, but none of them used complex-args as an example… So the point of this PR is to move global hash_merge settings to template and make it explicit.

Contributor

darkk commented Jun 20, 2014

@mpdehaan I referenced settings value as it's sort of "compatibility" filter that inherits all bugs of dict merging implementation. I'm unsure what is better: fix bugs in merge_hash that leads to NoneType dereference or leave warning sign about possible hash_merge hazard - that's what mortal example is about.

I try to use (abuse?) ansible as scripting language to generate monitoring system configuration as ansible is already used as deploy tool and having single DSL to learn is a bit better than two different alike DSLs. I use dict merging to provide different sets of default values for different checks and nested structures are used for syntax sugar when

http:
  path: /ping
  disable_ipv6: yes

is used as shortcut for

active: http
active_args:
  path: /ping
  disable_ipv6: yes

In this case hash_merge is used to provide cluster-wide variable that controls disable_ipv6 setting and leaves /path service-specific. I'm unsure if this real-world example is useful as juggler (monitoring system I am speaking about) is not released yet.

I dive into ansible-project@ mailing list a bit and found several requests for different merge behavior, but none of them used complex-args as an example… So the point of this PR is to move global hash_merge settings to template and make it explicit.

@jhriggs

This comment has been minimized.

Show comment
Hide comment
@jhriggs

jhriggs Sep 17, 2014

+1 to this functionality. My use case is slightly different in that I just need a way to combine/merge dictionaries. I would like to see these filters accept keyword arguments too, though, as a simpler way of passing in values. My use case:

vars:
  default_ec2_tags:
    environment: 'dev'
    application: 'doSomething'

tasks:
  - ec2_tag:
      resource: 'vpc-1234567'
      tags: '{{ default_ec2_tags | hash_replace(Name="MyVPC") }}'

I think that the keyword syntax hash_replace(Name="MyVPC") is simpler than hash_replace({Name: "MyVPC"}) in this case. It may just be my personal preference, but at least it can be possible with a quick change (the keyword args dict is just added as an item to the terms tuple):

--- hash_merge.py   2014-09-17 09:46:10.000000000 -0500
+++ library/plugins/filter_plugins/hash_merge.py    2014-09-17 09:31:19.000000000 -0500
@@ -22,19 +22,19 @@

 # These are two possible behaviours expressed by `ansible.utils.combine_vars`.

-def hash_merge(*terms):
+def hash_merge(*terms, **kwterms):
     for t in terms:
         if not isinstance(t, dict):
             raise errors.AnsibleFilterError("|hash_merge expects dictionaries, got " + repr(t))

-    return reduce(utils.merge_hash, terms)
+    return reduce(utils.merge_hash, terms + (kwterms,))

-def hash_replace(*terms):
+def hash_replace(*terms, **kwterms):
     for t in terms:
         if not isinstance(t, dict):
             raise errors.AnsibleFilterError("|hash_replace expects dictionaries, got " + repr(t))

-    return dict(itertools.chain(*map(dict.iteritems, terms)))
+    return dict(itertools.chain(*map(dict.iteritems, terms + (kwterms,))))

 class FilterModule(object):
     ''' Ansible jinja2 filter to avoid global hash_behaviour=merge when it's needed '''

jhriggs commented Sep 17, 2014

+1 to this functionality. My use case is slightly different in that I just need a way to combine/merge dictionaries. I would like to see these filters accept keyword arguments too, though, as a simpler way of passing in values. My use case:

vars:
  default_ec2_tags:
    environment: 'dev'
    application: 'doSomething'

tasks:
  - ec2_tag:
      resource: 'vpc-1234567'
      tags: '{{ default_ec2_tags | hash_replace(Name="MyVPC") }}'

I think that the keyword syntax hash_replace(Name="MyVPC") is simpler than hash_replace({Name: "MyVPC"}) in this case. It may just be my personal preference, but at least it can be possible with a quick change (the keyword args dict is just added as an item to the terms tuple):

--- hash_merge.py   2014-09-17 09:46:10.000000000 -0500
+++ library/plugins/filter_plugins/hash_merge.py    2014-09-17 09:31:19.000000000 -0500
@@ -22,19 +22,19 @@

 # These are two possible behaviours expressed by `ansible.utils.combine_vars`.

-def hash_merge(*terms):
+def hash_merge(*terms, **kwterms):
     for t in terms:
         if not isinstance(t, dict):
             raise errors.AnsibleFilterError("|hash_merge expects dictionaries, got " + repr(t))

-    return reduce(utils.merge_hash, terms)
+    return reduce(utils.merge_hash, terms + (kwterms,))

-def hash_replace(*terms):
+def hash_replace(*terms, **kwterms):
     for t in terms:
         if not isinstance(t, dict):
             raise errors.AnsibleFilterError("|hash_replace expects dictionaries, got " + repr(t))

-    return dict(itertools.chain(*map(dict.iteritems, terms)))
+    return dict(itertools.chain(*map(dict.iteritems, terms + (kwterms,))))

 class FilterModule(object):
     ''' Ansible jinja2 filter to avoid global hash_behaviour=merge when it's needed '''
@gimoh

This comment has been minimized.

Show comment
Hide comment
@gimoh

gimoh Mar 13, 2015

Contributor

+1

My use case is wrapping a builtin module (docker) to provide extra optional features, and I need to be able to inject settings into the parameters for the docker module. See the role for details.

Contributor

gimoh commented Mar 13, 2015

+1

My use case is wrapping a builtin module (docker) to provide extra optional features, and I need to be able to inject settings into the parameters for the docker module. See the role for details.

@amenonsen

This comment has been minimized.

Show comment
Hide comment
@amenonsen

amenonsen Jul 26, 2015

Contributor

I could also use the hash_merge and hash_replace functionality, in fact I have a custom filter that does the former and would be happy to get rid of it. @darkk any chance you could rebase this to apply to current devel?

Contributor

amenonsen commented Jul 26, 2015

I could also use the hash_merge and hash_replace functionality, in fact I have a custom filter that does the former and would be happy to get rid of it. @darkk any chance you could rebase this to apply to current devel?

@tersmitten

This comment has been minimized.

Show comment
Hide comment
@tersmitten

tersmitten Jul 28, 2015

Contributor

Nice

Contributor

tersmitten commented Jul 28, 2015

Nice

amenonsen added a commit to amenonsen/ansible that referenced this pull request Aug 27, 2015

Add a combine filter with documentation
This is based on some code from (closed) PR #7872, but reworked based on
suggestions by @abadger and the other core team members.

Closes #7872 by @darkk (hash_merge/hash_replace filters)
Closes #11153 by @telbizov (merged_dicts lookup plugin)

@abadger abadger closed this in #12078 Aug 27, 2015

@ansibot ansibot added feature and removed feature_pull_request labels Mar 4, 2018

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