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.

Sign up for GitHub

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

Fix #89 by ignoring only keys which are actually used in Pillar 'postfix:mapping' #90

Merged
merged 3 commits into from
Apr 26, 2019
Merged

Fix #89 by ignoring only keys which are actually used in Pillar 'postfix:mapping' #90

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c432ae2
mapping.j2: fixed handling of OrderedDict in Python 3
alxwr Apr 9, 2019
bb6746c
Explain multiple entries in virtual_alias_maps via pillar.example
alxwr Apr 9, 2019
fbaa2dc
main.cf: ignore only actually used keys of 'postfix:mapping'; fixes #89
alxwr Apr 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions pillar.example
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,3 +235,24 @@ postfix:
- [email protected]
- [email protected]
- singlealiasexample: [email protected]


###
#
# Multiple virtual_alias_maps entries:
#
# You are free to define alternative mapping names
# and use them as 'variables' in your Postfix config:
# (Credit for the idea and the example goes to @roskens.)

postfix:
config:
virtual_alias_maps: $virtual_alias_1_maps $virtual_alias_2_maps
virtual_alias_1_maps: hash:/etc/postfix/virtual
virtual_alias_2_maps: pcre:/etc/postfix/virtual.pcre
mapping:
virtual_alias_1_maps:
root:
- me
virtual_alias_2_maps:
- '/(\S+)_(devel|preprod|prod)@sub.example.com$/': '$(1)@$(2).sub.example.com'
13 changes: 2 additions & 11 deletions postfix/files/main.cf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,8 @@
{%- from "postfix/map.jinja" import postfix with context -%}
{%- set config = salt['pillar.get']('postfix:config', {}) -%}

{%- if not salt['pillar.get']('postfix:mapping', False) %}
{#- Let the user configure mapping manually. -#}
{%- set processed_parameters = [] %}
{%- else -%}
{#- TODO: alias_maps probably belongs here, too: #}
{%- set processed_parameters = [
'virtual_alias_maps',
'smtp_sasl_password_maps',
'sender_canonical_maps',
] %}
{%- endif -%}
{#- " | list": Python3.6 retuns dict_keys here, which needs to be converted into a list here. -#}
{%- set processed_parameters = salt['pillar.get']('postfix:mapping', {}).keys() | list %}
Copy link

Choose a reason for hiding this comment

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

@alxwr I don't think you need additional list filter. The keys() method will always return a new list.

Copy link
Member Author

Choose a reason for hiding this comment

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

In Python 3.6 keys() returns a dict_keys object, so the list is mandatory.

Without | list:

jinja2.exceptions.UndefinedError: 'dict_keys object' has no attribute 'append'

As far as I understand it: In Python 3.6 keys() of an OrderedDict object returns a view (which may change) on the keys of the object.

Copy link

Choose a reason for hiding this comment

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

Oh, thanks for pointing me out to Python 3.6 specifics, I wasn't aware of that.

However, still think it looks weird. As I can tell from the code, the processed_parameters used only once to verify the key exists. No need for such complexity.
We could avoid making the list at all:

{%- set processed_parameters = salt['pillar.get']('postfix:mapping', {}) %}

Then update the key instead of appending at line 14:

{%- do processed_parameters.update({parameter: value}) %}

Finally, at line 130 the if statement will work the same way.

Copy link
Member Author

Choose a reason for hiding this comment

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

@vutny Thanks for your suggestions regarding the elegance of the code. I appreciate that.

processed_parameters was a list, therefore I kept it that way. In my mind it makes more sense to manage a list of parameter names than to needlessly copy their values. postfix:mapping's contents may be quite large. Therefore I suggest we stick with the list.

I disagree with using a dict instead of a list, but if this is non-negotiable, I'll give way. :-)

Copy link

@vutny vutny Apr 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@alxwr No worries, I would just like to make things here as portable as possible because of two reasons:

  • Generally using Python methods directly is undesirable, unless there's no other clear way.
    As we've already learned, different Python version have "smaaaall" differences. And the Jinja code is not really Python, it is a layer on top. So it is better to go with Jinja semantics as much as possible in common situations.
  • The most heavy call here is pillar.get. It needs to fork minion process, talk to the master via message bus and network layer, receive probably megabytes back, cache it, deserialize and finally return back to the caller.
    Lots of stuff is going on under the hood. Comparing to that, storing even a big dict in temporary memory is nothing.

If your concern is that holding the values is unnecessary, we could use pillar.keys function! Nothing else need to be changed then.

Copy link
Member Author

Choose a reason for hiding this comment

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

@vutny Then we're on the same page. :-) Thanks for the thorough explanation!

Why didn't I think of pillar.keys in the first place? O.o
Thanks for the review and the suggestion! I updated the PR.

The | list is needed because of Py3.6: "jinja2.exceptions.UndefinedError: 'dict_keys object' has no attribute 'append'".


{%- macro set_parameter(parameter, default=None) -%}
{% set value = config.get(parameter, default) %}
Expand Down
3 changes: 2 additions & 1 deletion postfix/files/mapping.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
{%- else %}
{#- Some settings need order, handle OrderedDict #}
{% for item in data %}
Copy link

Choose a reason for hiding this comment

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

You can simply unpack data in the for loop right away:

{% for k, v in data %}
{{ format_value(k, v) }}
{%- endfor -%}

Copy link
Member Author

Choose a reason for hiding this comment

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

You can simply unpack data in the for loop right away:

Just tried that. It won't fly in Python 3.6:

ValueError: not enough values to unpack (expected 2, got 1)

Copy link

Choose a reason for hiding this comment

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

Yeah, I have mixed the list of dicts with the list of tuples. Your changes definitely looks great.

{{ format_value(item.keys()[0], item.values()[0]) }}
{%- set key, value = item.popitem() %}
{{ format_value(key, value) }}
{%- endfor -%}
{%- endif %}