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

Use cmd_check to validate config files #37

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nledez
Copy link

@nledez nledez commented Nov 4, 2018

In some case I have an error in config file. State detect a change, restart services, restart fail. It's bad.

Soo, I rewrite the resources to use file.managed allowed cmd_check.

It works as:

  Name: /usr/local/bin/consul - Function: file.symlink - Result: Clean Started: - 14:26:24.064532 Duration: 2.029 ms
----------
          ID: consul-config
    Function: file.managed
        Name: /etc/consul.d/config.json
      Result: False
     Comment: check_cmd execution failed
              Config validation failed: Error parsing /tmp/__salt.tmp.sQ8J8n.json: 1 error(s) occurred:

              * invalid config key asdf
     Started: 14:26:24.067033
    Duration: 193.813 ms
     Changes:
  Name: /etc/consul.d/services.json - Function: file.managed - Result: Clean Started: - 14:26:24.261664 Duration: 68.404 ms
  Name: /etc/default/consul - Function: file.managed - Result: Clean Started: - 14:26:24.330434 Duration: 13.754 ms
  Name: /etc/systemd/system/consul.service - Function: file.managed - Result: Clean Started: - 14:26:24.344581 Duration: 38.792 ms
----------
          ID: consul-service
    Function: service.running
        Name: consul
      Result: False
     Comment: One or more requisite failed: consul.config.consul-config
     Started: 14:26:24.386892
    Duration: 0.018 ms
     Changes:

Last think. I have an issue with whitespace and end of some lines :/
It's not a problem for Consul or Saltstack. Just for me when I open file with my vi doesn't like them and show me a "warning" (like my brain in fact :p ).

@nledez
Copy link
Author

nledez commented Nov 4, 2018

Work on Travis’s issue.

@nledez
Copy link
Author

nledez commented Nov 4, 2018

Ready to merge :)

@@ -0,0 +1 @@
{{ content | tojson(indent=2) }}
Copy link
Contributor

Choose a reason for hiding this comment

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

The tojson() Jinja filter is only available in the latest stable Salt 2018.3.3 release.
However, the built-in serializer has generic JSON filter for ages and also supports indentation. Simply like this:

content|json(indent=2)

This is more portable and reliable. As far as I understand upstream Jinja docs, the tojson() filter is designed to be used within HTML pages, and it could do some weird things when you need "raw" JSON file.

@@ -1,18 +1,24 @@
{%- from slspath + '/map.jinja' import consul with context -%}

consul-config:
file.serialize:
Copy link
Member

@aabouzaid aabouzaid Nov 7, 2018

Choose a reason for hiding this comment

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

IMHO, manage structured files with file.managed is really bad (I tried this myself and it always requires many hacks and checks).

I'd suggest to keep using file.serialize, and create another check for syntax specifically.
We could use mod_run_check_cmd, but just realized it doesn't work as expected (it's used as an internal function for file.managed. It's already exposed externally, however it doesn't met functions requirements, where it doesn't have name,changes in returned output).

So till fixing that (mostly I will make a PR for that), let's use cmd.run with unless:

consul-config-validate: 
  cmd.run:
    - name: /usr/local/bin/consul validate /etc/consul.d/config.json
    - unless: /usr/local/bin/consul validate /etc/consul.d/config.json
    - require:
      - file: consul-config

The unless clue, will prevent running the command when it has a valid syntax. So it avoids any extra noise.

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.

3 participants