i am not totally sure about this one, as it changes style of how salt might be used.
it might make some problems with config unnoticed.
but on the other hand, it could be really handy
i wanted to use this feature when i was writing mount.mounted state -- and i wanted the state to be applied only when the device is present. I know this could be solved by grains or pillars, but i prefer the easy way.
I personally don' t agree this should be the case. The reason onlyif and unless exist in my cmd is otherwise the command will be run each time the state runs. In the case of pkg, service, and so on, it's likely they will only be run once and otherwise verify the pkg is installed or the service is running. There isn't a need to put onlyif or unless everywhere.
I would, however, agree that it's possible to have onlyif and unless on other states. I just think we should have a similar justification to cmd and document its reasoning before creating these functions.
Yes, I think that adding onlyif and unless to all states would likely not be a great idea, primarily due to the fact that this functionality is available already in jinja.
As for mount.mounted, what would you expect the return to be if the device is not present? It should return a failure, and if there was an onlyif statement it would still return a failure, what behavior do you want to see?
for mount.mounted, my planned alternative was file.directory :)
For tests of the storage system, i do not need large disk mounted, directory is enough.
if you want people to use jinja for this, maybe more jinja examples calling salt['cmd.run']('something')
should be in docs.
It just didnt come on my mind even after using salt for a while. I have seen 'unless' many times, so i thought it belongs to 'requirements' . So i just typed unless: something
-- which didnt work. Why i am writing this? if we close this issue, we need to be sure others will get it right from docs.
Sounds good, I will set this as a documentation issue, good call!
@thatch45 Here you think it is a bad idea, did you changed your mind in one year ? :)
As on #9111 we are thinking both the contrary
Yes, I have changed my mind. I started investigating this a few months ago but got bogged down in other things. If I recall off the top of my head it would need to happen in the call_chunk method in salt/state.py
1.The call_chunk code referenced by @thatch45 for a possible fix is here; https://github.com/saltstack/salt/blob/a4198310371c15f3814cc1a164f705ee409c8b65/salt/state.py#L1541 and the method continues til about line 1800.
2.There seem to be lots of duplicates of this ticket, including #2642 and #9111 if I am not mistaken.
//Jumping in to learn via http://www.CodeTriage.com
I can certainly think of more use cases for this. In fact, I've hit one right now (and I was surprised when unless
was silently failing - I had no idea that this didn't already exist for some of the more popular state modules!).
Basically I want to clobber a file /etc/default/buildslave with a bunch of default settings from a salt:// location. Then I want to use file.append to add a bunch of lines to this for every buildslave configuration that there may be, which will vary depending on pillar, grains, etc.
Ideally, I'd just put some unique identifying string (say # Managed by Salt
) in the managed file we deploy, and then use file.managed to call - unless: grep -q '^# Managed by Salt' /etc/default/buildslave
. That way, when file.append executes later, it doesn't make file.manage undo everything.
As it stands, I can't see any obvious way to avoid having these file.managed and file.append states execute every time, which is really frustrating. I'm sure there are many such use cases that are important to support, but probably not obvious.
I would like to add a "load all the files in /etc/profile.d/*" section in /etc/profile, unless the OS has provided such a section already. Given that the formatting of this code snippet differs between platforms, a block replace won't work -- my hope was that "unless: grep profile.d /etc/profile" would work, but it seems not :(
From the discussion here I guess something like the following is the current recommendation?
{% if not salt['cmd.run']('grep profile.d /etc/profile') %}
/etc/profile:
file.append:
- contents: blah
{% endif %}
This issue has foiled my master plan, yet again!
myapp:
pkg.latest
{% for setting in ('VALUE1', 'VALUE2', 'VALUE3') %}
Configure /etc/somecfg.conf:
file.replace:
- name: /etc/somecfg.conf
- pattern: define\('{{ setting }}',.*
- repl: "define('{{ setting }}', '{{ pillar['myapp'][setting] }}');"
- backup: False
- require:
- pkg: myapp
Configure /etc/somecfg.conf.dpkg-dist:
file.replace:
- name: /etc/somecfg.conf.dpkg-dist
- pattern: define\('{{ setting }}',.*
- repl: "define('{{ setting }}', '{{ pillar['myapp'][setting] }}');"
- backup: False
- onlyif: test -f /etc/somecfg.conf.dpkg-dist
- require_in:
- file: mv /etc/somecfg.conf.dpkg-dist /etc/somecfg.conf
- require:
- pkg: myapp
{% endfor %}
mv /etc/somecfg.conf.dpkg-dist /etc/somecfg.conf:
file.rename:
- name: /etc/somecfg.conf
- source: /etc/somecfg.conf.dpkg-dist
- force: True
- onlyif: test -f /etc/somecfg.conf.dpkg-dist
As a start, _onlyif_ and _unless_ should at least this should be added to all _file_ states. Not having it is becoming a major annoyance.
Closing, looks like it was implemented! =D
looks like this was not implemented for file.*
e.g. when using it in file.symlink nothing shows up in my log and it doesn't behave as expected.
because of #10017 there were no warnings/errors. used up to 2014.1.10.
@pille , can you give us an example that fails?
I sympathize about missing error messages on your referenced ticket :). It's so great Saltstack fixes these things fast.
warn when using non-existant parameters to state functions #10017
in the following example the symlink is always replaced.
it should just be set, when it's not already there, but never overwritten.
{{ prefix }}/rails/current:
file.symlink:
- target: releases/bootstrap
- unless: test -L {{ prefix }}/rails/current
my current workaround is to wrap this in:
{% if salt['cmd.retcode']('test -L ' + prefix + '/rails/current') != 0 %}
...
{% endif %}
This isn't globally available until 2014.7
On Sep 4, 2014 10:49 AM, "pille" [email protected] wrote:
in the following example the symlink is always replaced.
it should just be set, when it's not already there, but never overwritten.{{ prefix }}/rails/current:
file.symlink:
- target: releases/bootstrap
- unless: test -L {{ prefix }}/rails/currentmy current workaround is to wrap this in:
{% if salt'cmd.retcode' != 0 %}
...
{% endif %}—
Reply to this email directly or view it on GitHub
https://github.com/saltstack/salt/issues/1976#issuecomment-54498272.
@gtmanfred - thanks for this mention cause I assumed that we couldn't use onlyif/unless in file*.
I just tested this in Helium and it appears to be working.
@basepi
Shouldn't the docs be updated here to mention this?:
http://docs.saltstack.com/en/latest/ref/states/all/salt.states.file.html
Just wanted to leave my comment here (I already talked to tehmaspc on IRC) -- I think we just need to _remove_ the unless/onlyif docs from the cmd state, as now all states use the global state args documented here: http://docs.saltstack.com/en/latest/ref/states/requisites.html
@basepi - agreed +1; thanks man!
Most helpful comment
if you want people to use jinja for this, maybe more jinja examples calling
salt['cmd.run']('something')
should be in docs.It just didnt come on my mind even after using salt for a while. I have seen 'unless' many times, so i thought it belongs to 'requirements' . So i just typed
unless: something
-- which didnt work. Why i am writing this? if we close this issue, we need to be sure others will get it right from docs.