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

Syntax checking #802

Open
j0nes2k opened this issue Mar 1, 2012 · 49 comments

Comments

Projects
None yet
@j0nes2k
Copy link

commented Mar 1, 2012

When a salt state contains YAML syntax errors, it is quite hard to find these errors at the moment. It would be great to have a basic syntax check (validation agains a schema) and/or references check (e.g. do the references given in require-statements really exist).

@thatch45

This comment has been minimized.

Copy link
Member

commented Mar 1, 2012

Yes, the problem is that when we parse the data it is a data structure and we don't know if it came from YAML or some other source. Also that it needs to be passed through the jinja as well. When you find an issue that was difficult to debug please let us know what the error was and what the solution was so we can make the errors better

@dcolish

This comment has been minimized.

Copy link
Member

commented Mar 3, 2012

In general, I think a litle tool that allowed you to render sls files locally and pass in the commands via the command line would be really helpful for developing these modules. This tool could easily try and load the yaml, json or any other format supported in the future.

@j0nes2k

This comment has been minimized.

Copy link
Author

commented Mar 6, 2012

Thank you for your input! The main problem is that when I create a state, I write the complete file including package installations, config file copying etc. in one go. And of course there will be simple typing errors, like "reqiure" vs. "require" (just an example). When I try to push this state to my machines, I only get empty responses back (e.g. no "{}", just an empty line). Then I have to comment out parts of my state, check whether the highstate now works, comment out other parts until I find this simple error.

It would be great to have a simple check (like: "method 'reqiure' is not known" or something like that) to make it easier to debug things - or am I missing a built-in solution available in salt already?

@thatch45

This comment has been minimized.

Copy link
Member

commented Mar 6, 2012

Ahh, yes, we do need better error checking, and maybe some kind of salt-lint would be nice. I am working towards something that will make this better, and there are some places where error checking is still lacking, we will keep this on the radar!

@halberom

This comment has been minimized.

Copy link

commented Mar 6, 2012

I've experienced the same issues, the current workaround/solution is to run "salt-call -l info state.highstate" on a minion as it always reports compile errors properly even though nothing is returned to the master. The output shows issues like typos and source file location errors. Note that it will only report the first compile errors it breaks on, so if there are a few, it'll be necessary to fix them and try again until it passes cleanly.

@thatch45

This comment has been minimized.

Copy link
Member

commented Mar 6, 2012

In git the errors should be returned to the master, and yes, there are a few rounds of errors that Salt can't proceed if there are errors early on.

@mlister2006

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2012

It would be nice to have simple YAML syntax checking for errors. e.g checking colon : , indentation etc.

test                                  # missing colon :
  - arg1
  - arg2

syntax error like above, simply pass thru to minions when running state.highstate and crashes minion (need to restart minion process)

@thatch45

This comment has been minimized.

Copy link
Member

commented Oct 2, 2012

Yes, I have started on a process for this which is called master compiling, but have been unable to finish it, there has been too much demand in too many places!
I will get back to it eventually and make this happen!

@alecthomas

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2013

I would very much like to see some better validation...some bugs are currently extremely hard to track down :\

For example, people new to salt are inclined to believe that the top-level directives in an sls file are a list, not a mapping, so they frequently duplicate keys. eg.

ntpd:
  pkg.installed: []
ntpd:
  service.running:
    - require:
      - pkg: ntpd

This silently fails and is difficult to track down.

Additionally, there doesn't seem to be any validation of state names in the top file? If I mistype a state name in the top file, it just gets dropped without error.

A possible solution might be to have two-stage validation:

  1. Per-data-source validators, eg. yaml, json, etc.
  2. The final parsed in-memory data-structure validator.
@thatch45

This comment has been minimized.

Copy link
Member

commented Jan 9, 2013

A state name gets dropped without an error? Can you give me an example? this should generate an error.
My plan for a validator is the masterstate approach, but it has been stagnat for a while

@alecthomas

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2013

I'll try to dig up an example, but in the interim I whipped up a quick YAML + Salt validator.

This has two stages:

  1. YAML structural validation - currently primarily disallowing duplicate keys.
  2. Once the YAML is parsed into a Python dictionary, validate the various Salt structures (top, state and pillar) with voluptuous. This is very broad, and has no knowledge of the module-specific structures.
@thatch45

This comment has been minimized.

Copy link
Member

commented Jan 10, 2013

This looks like it will easily get past many of the rendering issues and validate structure, how would you feel about us including this as a runner?

@alecthomas

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2013

That would be fine with me, please use as much of the code as you want.

I tested this on our salt tree and it works quite well, but I have no easy means of testing it after Jinja expansion, so it explodes on a few files.

And of course, it doesn't do source validation on any of the other data sources: json, etc.

@thatch45

This comment has been minimized.

Copy link
Member

commented Jan 10, 2013

Sounds good, at least it is a good starting point!

@jolestar

This comment has been minimized.

Copy link

commented Apr 15, 2013

me too !!!

@fsniper

This comment has been minimized.

Copy link

commented Jun 27, 2013

This should also consider jinja and python states. Maybe not python states but at least jinja states.

@rgbkrk

This comment has been minimized.

Copy link
Contributor

commented Jan 29, 2014

I've been curious about this for a bit, so I went ahead and figured out a simple setup on travis ci with help from @whiteinge and @thatch45. If your states are open source or you use Travis CI Pro, you're 👍.

.travis.yml

language: python
python:
- '2.7'

before_install:
  - sudo apt-get update
  - curl -L http://bootstrap.saltstack.org | sudo sh -s -- git develop

install:
  # Copy these states
  - sudo mkdir -p /srv/salt/states
  - sudo cp -r . /srv/salt/states
  - sudo cp .travis/minion /etc/salt/minion
  - sudo service salt-minion restart

  # Additional debug help
  - sudo cat /var/log/salt/*

  # See what kind of travis box you're on
  # to help with making your states compatible with travis
  - sudo salt-call grains.items --local

script:
  - sudo salt-call state.show_highstate --local  --retcode-passthrough

.travis/minion

file_client: local
file_roots:
  base:
    - /srv/salt/states

This setup was designed with the simplistic states that nbviewer uses. More about it in blog form.

You'll also want to load your pillar data onto the Travis minion and possibly run a highstate (careful with this as Travis' environment is likely a bit different from your infrastructure).

@lcosmin

This comment has been minimized.

Copy link

commented Feb 4, 2014

Not sure if still of use, but I've wasted ~30 minutes trying to find an error in pillar's top.sls (simplified for the sake of example):

base:
  'roles:test':
    - match:grain
    - test

Notice the missing space between match: and grain. My pillar data wasn't available to the minion and I was checking for spelling errors and such, but somehow my brain didn't "see" the missing space. Maybe some sort of warning could be added for such things.

@HontoNoRoger

This comment has been minimized.

Copy link

commented Oct 23, 2014

+1

@whiteinge

This comment has been minimized.

Copy link
Contributor

commented Oct 24, 2014

Voluptuous would be great for this. It's small, simple, and crazy straight-forward. We could toss a copy of it in salt/utils/validate/ with no impact. I started work writing a schema for the Salt master config and has worked well for validating the external_auth part. I'm not going to have time to put into that for a little while so if someone else wants to take the first stab, please feel free.

See also #15836.

@v6

This comment has been minimized.

Copy link

commented Jan 14, 2015

I'm coming from Puppet, myself. I would have preferred to install SaltStack, but everyone and their mother was using it at my last company.

Anyway, there were a lot of things that they put together to make life easier on non-expert users. These almost made up for the severe limitations of the Puppet system.

Anyway, one of the things I liked best was puppet validate.

It saved me a lot of time.

Why no salt validate?

@thatch45

This comment has been minimized.

Copy link
Member

commented Jan 14, 2015

While it is not as convenient the

salt-call state.show_highstate

Can compile the sls files and at least show you the errors. It is primarily because the sls files can be generated on the minions, so always getting the right data from Jinja can be tricky

@v6

This comment has been minimized.

Copy link

commented Jan 14, 2015

OK.

I just wish there was a validation checker that could point me to the
location in the files, specifically.

IDEs have this, and I'm going to be installing one for Sublime Text
and Pycharm.

As to the generation, isn't it deterministic?

On 1/14/15, Thomas S Hatch notifications@github.com wrote:

While it is not as convenient the

salt-call state.show_highstate

Can compile the sls files and at least show you the errors. It is primarily
because the sls files can be generated on the minions, so always getting the
right data from Jinja can be tricky


Reply to this email directly or view it on GitHub:
#802 (comment)

Nate B.

@v6

This comment has been minimized.

Copy link

commented Jan 14, 2015

---------- Forwarded message ----------
From: Nate B nathan@basanese.com
Date: Wed, 14 Jan 2015 15:37:25 -0800
Subject: Re: [salt] Syntax checking (#802)
​​
To: saltstack/salt <
reply@reply.github.com

Cc: saltstack/salt salt@noreply.github.com, Nathan Dataguake Basanese <
github.com-v6@basanese.com>, an.lin@wdc.com

OK.

I just wish there was a validation checker that could point me to the
location in the files, specifically.

IDEs have this, and I'm going to be installing one for Sublime Text
and Pycharm.

As to the generation, isn't it deterministic?

On 1/14/15, Thomas S Hatch notifications@github.com wrote:

While it is not as convenient the

salt-call state.show_highstate

Can compile the sls files and at least show you the errors. It is
primarily
because the sls files can be generated on the minions, so always getting
the
right data from Jinja can be tricky


Reply to this email directly or view it on GitHub:
#802 (comment)

@damon-atkins

This comment has been minimized.

Copy link
Member

commented Jan 15, 2015

http://docs.saltstack.com/en/latest/ref/states/testing.html
It would be nice if you could setup a environment which defaults to testing only (dry-run).

@pcn

This comment has been minimized.

Copy link
Contributor

commented Aug 18, 2015

I've got an alternate take on this at https://github.com/librato/salt-state-test. It's a bit rough, but feedback from someone beside me is appreciated.

@figtrap

This comment has been minimized.

Copy link

commented Oct 24, 2015

We really need something like this ...

@jfindlay jfindlay added the Core label Nov 4, 2015

@damon-atkins

This comment has been minimized.

Copy link
Member

commented Feb 25, 2016

Need a tool which pulls in the pillar data, either a file or a list of files, directory and then give it a state file, and then it prints out the final state/catalog/manifest. It can work outside of salt. In fact it would be better if it did, so the checking can be perform before committing changes.

salt-call state.dry-run
salt-call state.process-state-only

@thatch45

This comment has been minimized.

Copy link
Member

commented Feb 25, 2016

I recently added the mocked option to the state calls:
Salt-call state.high state mocked=True
This does the full state run just without executing the states, so everything compiles with the pillar etc.

@pcn

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2016

@thatch45 where's the commit/branch with that in it?

@pcn

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2016

@damon-atkins the repo I pointed to early does something like what you're asking, but the pillar and the grains can each only be one file.

@thatch45

This comment has been minimized.

Copy link
Member

commented Feb 25, 2016

It is in 2015.8.7

@tampakrap

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2016

@thatch45 Can you explain please how is that different from test=True?

@thatch45

This comment has been minimized.

Copy link
Member

commented Feb 25, 2016

test=True runs in no-op mode and checks the state of the system
mocked=True runs the state runtime and resolves all requisites but it does not run any states on the target system
state.show_highstate just runs the data compiler and returns the data tht the runtime would execute with

Basically, show_highstate does syntax checks, mocked does syntax + runtime (requisites and ordering) checks, and test=True does a full state run with checks to see if anything will be changed with an actual state run

So it is basically just stopping the process at certain points to give you a clear representation of what your states are and will do

@figtrap

This comment has been minimized.

Copy link

commented Feb 25, 2016

Oh what I really want is some sort of IDE for salt development that would
do things like autofill pillars and variables when you're writing jinja,
linting the yaml and things like that. There is just so much wasted time
with typos. But the "mocked" feature is much needed and appreciated.

Tim Kelley

On Thu, Feb 25, 2016 at 10:46 AM, Thomas S Hatch notifications@github.com
wrote:

test=True runs in no-op mode and checks the state of the system
mocked=True runs the state runtime and resolves all requisites but it does
not run any states on the target system
state.show_highstate just runs the data compiler and returns the data tht
the runtime would execute with

Basically, show_highstate does syntax checks, mocked does syntax + runtime
(requisites and ordering) checks, and test=True does a full state run with
checks to see if anything will be changed with an actual state run

So it is basically just stopping the process at certain points to give you
a clear representation of what your states are and will do


Reply to this email directly or view it on GitHub
#802 (comment).

@thatch45

This comment has been minimized.

Copy link
Member

commented Feb 25, 2016

Oh, yes, what you are describing would be great

@damon-atkins

This comment has been minimized.

Copy link
Member

commented Feb 26, 2016

One of the issue I was hoping this would resolve is the error messages not indicating where an issue is. For example JINJA processing only says:
Rendering SLS 'base:iscdhcp' failed: Jinja variable 'list object' has no attribute 'name'
Rendering SLS 'base:iscdhcp' failed: Jinja variable 'dict object' has no attribute 'path'
The problem is 'base:iscdhcp' as a whole can contain multiple name and path, which one is it in the structure. (2015.5.8) Maybe saltstack needs to submit a change into jinja to improve errors.

Just want to be able to find issues, as much as possible, and find them before they go live (committed/pushed).

Update for Fedora running 4.4.2-301.fc23.armv7hl i.e. ARM

[salt-latest]
name=SaltStack Latest Release Channel for RHEL/Centos
#baseurl=https://repo.saltstack.com/yum/redhat///latest
baseurl=https://repo.saltstack.com/yum/redhat/latest/x86_64/latest/
failovermethod=priority
enabled=1
gpgcheck=1
#gpgkey=https://repo.saltstack.com/yum/redhat///latest/SALTSTACK-GPG--KEY.pub
gpgkey=https://repo.saltstack.com/yum/redhat/latest/x86_64/latest/SALTSTACK-GPG-KEY.pub

===============================================================
 Package              Arch         Version               Repository        Size
===============================================================
Installing:
 python-cherrypy      noarch       3.5.0-1.fc22          fedora           459 k
 python-libcloud      noarch       0.20.1-1.fc23         updates          2.0 M
 python-tornado       armv7hl      4.2.1-1.fc23          updates          642 k
 salt                 noarch       2015.8.7-1.el7        salt-latest      5.2 M
 salt-api             noarch       2015.8.7-1.el7        salt-latest       13 k
 salt-cloud           noarch       2015.8.7-1.el7        salt-latest       16 k
 salt-master          noarch       2015.8.7-1.el7        salt-latest      1.2 M
 salt-minion          noarch       2015.8.7-1.el7        salt-latest       29 k

Transaction Summary
===============================================================
Install  8 Packages
@gravyboat

This comment has been minimized.

Copy link
Member

commented Jul 14, 2016

I'm just going to bump this again. People keep running into this issue of not being able to syntax check without doing a run, which doesn't provide great errors. There really needs to be a built in utility within salt to at least confirm the yaml/jinja is legit (even if it does not check against the pillar, though that would be nice). This has been a 'want' for both new and existing users for over 4 years now.

@jowolf

This comment has been minimized.

Copy link

commented Jul 20, 2016

bump - I wrote a custom yaml module (for another project) to trace yaml compiiles and line numbers, for example - so when it died you could see where it was last.

It was crude, and used globals for state like the line number, but it's an idea

@RobotLimeLtd

This comment has been minimized.

Copy link

commented Jul 20, 2016

The nearest you might get is the following, however it will only work for very simple Salt setups, and only on a server that has Salt Minion installed. Assuming your source repo contains both "salt" and "pillar" directories, each with a top.sls in them... you could run:

salt-call state.highstate --file-root=$PWD/salt --pillar-root=$PWD/pillar --local --retcode-passthrough mocked=True

However, if you have anything more exotic, such as an external pillar directory, then you're out of luck.

@gravyboat

This comment has been minimized.

Copy link
Member

commented Jul 20, 2016

@RobotLimeLtd Yeah we've discussed that as an option before, and it's not a significantly satisfactory option for anyone I've talked to. Thanks for noting it though!

@chrismcmacken

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2017

Do salt states have a documented grammar that is used to parse them?

@pcn

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2017

@chrismcmacken

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2017

@pcn I hadn't, salt supports a lot more renderers than I thought. Still though, if it gets parsed it must be against some set of rules. I'm sure it's a tough problem to solve, otherwise it would have been solved already, just thought I'd ask and bump the thread.

@pcn

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2017

It doesn't seem to support a grammar so much as a list of extensible grammars that map directly to a list of python dictionaries.

@soostdijck

This comment has been minimized.

Copy link

commented Aug 25, 2017

Bump as well. Has anyone made progress on this?
I'm still hoping that I'll be able to check syntax via commit hooks properly.

@JensRantil

This comment has been minimized.

Copy link
Contributor

commented Aug 25, 2017

Just wanted to share how we've created a workaround for syntax checking in our CI pipeline: In our top.sls files we've added a standardized way of adding example minion names:

# CI example: my-minion-name
'my-minion-*':
  - some.state

We then have a script that extracts all the example minion names (salt-dry-run.sh) for each minion name calls the script salt-dry-run-single.sh with the first (and only) parameter being the minion name.

You can find the code here: https://gist.github.com/JensRantil/c8757d607b379d3256299fd32a373a35

Obviously, this doesn't help with local development, but if you are for example using Github Pull Request you could add this as a CI step for this. Hope this makes the lives easier for some people.

@wyardley

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2018

Agree that this would be very useful, something like puppet parser validate for Puppet (to do at least a basic syntax check of the code itself.
Obviously, for YAML itself, it's not hard to just add commit hooks using yamllint or similar.

Having worked with Puppet for a while, something like puppet parser validate, for example, is super handy. That is not to say that you don't also need to do other types of testing, but being able to at least catch simple typos fast (ideally on a machine that doesn't even have Salt setup on it) would be super handy. I've been playing around with this again recently, and came up with something fairly similar to a comment above, but it's not ideal.

@wyardley

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2018

Just as a heads up, while mocked=True doesn't error, mock=True seems to be the correct arg now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.