fix(macros): fix format_kwargs macro

[sticky-note]

PR progress checklist (to be filled in by reviewers)

• Changes to documentation are appropriate (or tick if not required)
• Changes to tests are appropriate (or tick if not required)
• Reviews completed

---

What type of PR is this?

Primary type

• [build] Changes related to the build system
• [chore] Changes to the build process or auxiliary tools and libraries such as documentation generation
• [ci] Changes to the continuous integration configuration
• [feat] A new feature
• [fix] A bug fix
• [perf] A code change that improves performance
• [refactor] A code change that neither fixes a bug nor adds a feature
• [revert] A change used to revert a previous commit
• [style] Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc.)

Secondary type

• [docs] Documentation changes
• [test] Adding missing or correcting existing tests

Does this PR introduce a BREAKING CHANGE?

No.

Related issues and/or pull requests

Quote kwargs values on format_kwargs macro to resolve sls rendering failure on special characters :

    Data failed to compile:
----------
    Rendering SLS 'base:postgres.manage' failed: found character '%' that cannot start any token; line 17
---
[...]
  postgres_user.present:
    - createdb: False
    - createroles: False
    - inherit: True
    - name: username
    - password: %0q`]O/)/d|v(Sj3-JR$HK8;    <======================
    - replication: False
    - user: postgres
    - onchanges:
      - test: postgres-reload-modules

[...]
---
ERROR: Minions returned with non-zero exit code

Describe the changes you're proposing

Pillar / config required to test the proposed changes

Debug log showing how the proposed changes work

Documentation checklist

• Updated the README (e.g. Available states).
• Updated pillar.example.

Testing checklist

• Included in Kitchen (i.e. under state_top).
• Covered by new/existing tests (e.g. InSpec, Serverspec, etc.).
• Updated the relevant test pillar.

Additional context

[myii]

@sticky-note Thanks for the PR. Just tracking back through the use of format_kwargs:

postgres-formula/postgres/upstream.sls

Lines 22 to 23 in 3df38b1

	pkgrepo.managed:
	{{- format_kwargs(postgres.pkg_repo) }}

• This should be fine.

postgres-formula/postgres/macros.jinja

Lines 13 to 32 in fbd8f65

	{%- macro format_state(name, state, kwarg) %}
	{%- if 'name' not in kwarg %}
	{%- do kwarg.update({'name': name}) %}
	{%- endif %}
	{%- if 'ensure' in kwarg %}
	{%- set ensure = kwarg.pop('ensure') %}
	{%- endif %}
	{%- set user_available = not (state == 'postgres_schema' and grains.saltversioninfo < [2018, 3]) %}
	{%- if 'user' not in kwarg and user_available %}
	{%- do kwarg.update({'user': postgres.user}) %}
	{%- endif -%}
	{{ state }}-{{ name }}:
	{{ state }}.{{ ensure|default('present') }}:
	{{- format_kwargs(kwarg) }}
	- onchanges:
	- test: postgres-reload-modules
	{%- endmacro %}

• This needs further looking in to, which follows below.

postgres-formula/postgres/manage.sls

Lines 26 to 30 in cd6eb0b

	{%- for name, user in postgres.users|dictsort() %}
	{{ format_state(name, 'postgres_user', user) }}
	{%- endfor %}

• This is the part you're trying to fix.
• However, there are potential issues based on the expected data here.

postgres-formula/pillar.example

Lines 135 to 150 in 35850da

	users:
	localUser:
	ensure: present
	password: '98ruj923h4rf'
	createdb: false
	createroles: false
	inherit: true
	replication: false
	remoteUser:
	ensure: present
	password: '98ruj923h4rf'
	createdb: false
	createroles: false
	inherit: true
	replication: false

• There are boolean values being supplied here; converting these to string may have consequences when consumed by the Salt state and modules -- almost definitely best to avoid quoting these booleans.

---

@vutny Can I get your opinion about this?

[sticky-note]

@myii hmmmm, you're right. Non-empty strings are evaluated as true. Is there a way to test the type of a value ?

[sticky-note]

Tested a salt-call state.show_sls postgres on this pillar:

postgres:
  users:
    test_dba:
      ensure: present
      password: '%=Hec$K8;'
      createdb: false
      createroles: false
      inherit: true
      replication: false

It seems that boolean strings are correctly computed in Booleans :

(...)
    postgres_user-test_dba:
        ----------
        __env__:
            base
        __sls__:
            postgres.manage
        postgres_user:
            |_
              ----------
              createdb:
                  False
            |_
              ----------
              createroles:
                  False
            |_
              ----------
              inherit:
                  True
            |_
              ----------
              name:
                  test_dba
            |_
              ----------
              password:
                  %=Hec$K8;
            |_
              ----------
              replication:
                  False
            |_
              ----------
              user:
                  postgres
            |_
              ----------
              onchanges:
                  |_
                    ----------
                    test:
                        postgres-reload-modules
            - present
            |_
              ----------
              order:
                  10011
(...)

I think it's because format_kwargs is never called directly but not sure at all

[pull-assistant]

Best reviewed: commit by commit

---

Optimal code review plan

     fix(macros): fix format_kwargs macro

Powered by Pull Assistant. Last update 5e6511b ... 5e6511b. Read the comment docs.

[sticky-note]

@myii @vutny Is that better like that ?

[sticky-note]

What about this PR @vutny @myii ? :)

[myii]

@sticky-note Perhaps this can be done more cleanly using https://docs.saltstack.com/en/latest/topics/jinja/#quote. Not tried this yet but how about:

Suggested change

	- {{ k }}: {{ "'" if v is string else '' }}{{ v }}{{ "'" if v is string else '' }}
	- {{ k }}: {{ v|quote if v is string else v }}

[sticky-note]

| quote doesn't work with : in strings for example.
yaml_dquote has better escaping power and encapsulate into " like I wanted

[myii]

@sticky-note Did you test this out first?

[sticky-note]

@myii
| quote doesn't work with : in strings for example.
| yaml_dquote has better escaping power and encapsulate into " like I wanted

[myii]

Merged, thanks @sticky-note -- apologies for the delay.

[sticky-note]

@myii No problem, I didn't have so much time too ;)

[saltstack-formulas-travis]

🎉 This PR is included in version 0.41.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀