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

Allow "multiple state declarations of the same type" when function is different #35592

Closed
srkunze opened this issue Aug 19, 2016 · 34 comments
Closed
Labels
Core relates to code central or existential to Salt Feature new functionality including changes to functionality and code refactors, etc. Renderers
Milestone

Comments

@srkunze
Copy link
Contributor

srkunze commented Aug 19, 2016

It would be great if one could do the following:

apache_sites:
  apache_site.enabled:
    - names:
      - my-server
      - your-server
  apache_site.disabled:
    - names:
      - 000-default

Right now, we circumvent the error by introducting another state such as:

apache_sites_enabled:
  apache_site.enabled:
    - names:
      - my-server
      - your-server

apache_sites_disabled:
  apache_site.disabled:
    - names:
      - 000-default
@Ch3LL
Copy link
Contributor

Ch3LL commented Aug 19, 2016

@srkunze I believe this is a duplicate of #29142 . If you agree please feel free to close this issue and comment on #29142 and we will track this feature request there. Thanks

@Ch3LL Ch3LL added Duplicate Duplicate of another issue or PR - will be closed info-needed waiting for more info labels Aug 19, 2016
@Ch3LL Ch3LL added this to the Blocked milestone Aug 19, 2016
@srkunze
Copy link
Contributor Author

srkunze commented Aug 19, 2016

@Ch3LL I looked that issue up already but it's not 100% the same I think.

They need 2x "apache_site.enabled". I need "apache_site.enabled" + "apache_site.disabled" which is different.

So, I let you the choice to close the issue if you think the internal implementation would be the same. :)

@Ch3LL
Copy link
Contributor

Ch3LL commented Sep 21, 2016

Thanks for clarifying . Will add this as a feature request instead of treating as a duplciate thanks.

@Ch3LL Ch3LL added Feature new functionality including changes to functionality and code refactors, etc. Core relates to code central or existential to Salt Renderers and removed Duplicate Duplicate of another issue or PR - will be closed info-needed waiting for more info labels Sep 21, 2016
@Ch3LL Ch3LL modified the milestones: Approved, Blocked Sep 21, 2016
@wayne-weibel
Copy link

Glad to see that this is already in the queue. Just wanted to add another (probably more common) example ... file.managed and file.symlink

@jktr
Copy link

jktr commented Jul 2, 2017

I ran into this exact issue today, with the following use case: locale.present and locale.system

@btorch
Copy link

btorch commented Jul 26, 2017

same here trying to do:

diamond:
   file.directory: 
    .....
   file.managed:
   ....

@houming818
Copy link

kafka-config-dir:
  file.absent:
    - name: /not/useful/dir
  file.directory:
    - name: /some/useful/dir

@ghost
Copy link

ghost commented Nov 9, 2017

Ran into this as well using:

lvm.pv_present:
  - name: /dev/xvdd
lvm.vg_present:
  - name: vol_grp1
lvm.lv_present:
  - name: logical_vol1

@tbennett6421
Copy link

I'd like this feature as well

/etc/ssh/sshd_config:
  file.uncomment:
    - regex: '.*AuthorizedKeysFile.*'

  file.replace:
    - pattern: '(.*LoginGraceTime.*)'
    - repl: 'LoginGraceTime 120'
    - show_changes: true 

@madfordmac
Copy link

Another use case:

Configure network time service:
  pkg.purged:
    - pkgs:
      - ntp
      - ntp-doc
      - ntpdate

  pkg.installed:
    - name: chrony

@naveen-rasiah
Copy link

One of the most 'in your face' features that should be added...but I'm guessing backwards compatibility limits this.

@faelin
Copy link

faelin commented Apr 18, 2019

And another, possibly quite common use case!

edit some file two ways when needed:
  file.replace:
    - name: '/some/file/path'
    - pattern: 'replace_me'
    - repl: 'better_string'
  file.line:
    - name: '/same/file/path'
    - match: 'delete_this_string'
    - mode: delete

@benfiedler
Copy link

benfiedler commented Jun 19, 2019

I would have liked to setup passwordless ssh keys to a repo host (without using 'id-rsa' for the keyfile) in one state

ssh_repo_passwordless:
  file.copy:
    - name: /root/.ssh/your_repo_rsa_priv
    - source: salt://sshkeys/your_repo_rsa_priv
    - user: root
    - group: root
    - mode: 0600
  file.append:
    - name: /root/.ssh/config
    - text: |
         Host your_repo your_repoIP
           HostName your_repoIP
           User repo_user
           IdentityFile /root/.ssh/your_repo_rsa_priv

@ssoto2
Copy link

ssoto2 commented Jun 20, 2019

additional case:

the alternatives system: https://docs.saltstack.com/en/latest/ref/states/all/salt.states.alternatives.html

openjdk.set.java:
  alternatives.install:
    - name: java
    - link: /usr/bin/java
    - path: /usr/lib/jvm/java-1.7.0-openjdk.x86_64/bin/java
    - priority: 5
    - require:
      - pkg: openjdk.install
  alternatives.set:
    - name: java
    - path: /usr/lib/jvm/java-1.7.0-openjdk.x86_64/bin/java

@dsaard
Copy link

dsaard commented Oct 18, 2019

👍

@pbusquemdf
Copy link

Another use case:

/home/{{ user }}/.ssh/config:
  file.managed:
    - user: {{ user }}
    - mode: '0644'
    - makedirs: True
    - dir_mode: '0766'
    - source:

  file.append:
   - name: /home/{{ user }}/.ssh/config
   - text:
     - "Host: git.example.org"
     - "  IdentityFile /etc/salt/rsa.id"

file.append would create file with the wrong permission (aka root) if the files doesn't exist and you can't define that info. So you must run an empty manage that will create an empty file if absent with the proper permissions, then append logically follow.

@stale
Copy link

stale bot commented Jan 7, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

@stale stale bot added the stale label Jan 7, 2020
@dsaard
Copy link

dsaard commented Jan 9, 2020

not stale

@stale
Copy link

stale bot commented Jan 9, 2020

Thank you for updating this issue. It is no longer marked as stale.

@stale stale bot removed the stale label Jan 9, 2020
@cdalvaro
Copy link
Contributor

👍

@Mukrosz
Copy link

Mukrosz commented May 6, 2020

Hello,

Is this still in the pipeline? Similar requirement:

install.salt.agent:
    pkg.refresh_db:
    pkg.installed:
        - name: salt-minion
        - version: latest

Thanks,

@srkunze
Copy link
Contributor Author

srkunze commented May 8, 2020

To us this is no longer a pressing issue as it is just a convenience feature is is actually not needed in more complex scenarios (which evolve when using salt a lot).

We also experienced confusion -- when reusing a state id -- among those who are not so experienced with salt but still need to write salt-sls-code. So, we would rather go the other way, removing reusing state names completely.

@srkunze
Copy link
Contributor Author

srkunze commented May 8, 2020

I created #57150 and let the community decide which way to go. :-)

@bsoldhogg
Copy link

Also run in to this with locale.present and locale.system and a few others.

@mariusvw
Copy link

Another use case where this might become handy:

/etc/rc.local:
  file.managed:
    - create: True
    - mode: 755
  file.prepend:
    - text: "#!/bin/sh -e"
  file.append:
    - name: /etc/rc.local
    - text: "exit 0"
  file.line:
    - name: /etc/rc.local
    - mode: ensure
    - before: "exit 0"
    - content: "/etc/init.d/somefancystuff restart"

@TheNetworkIsDown
Copy link

Yep, I was trying to use file.keyvalue to update one of the config files of a service and file.append (which is in another format) to update another. Not possible :(

@faelin
Copy link

faelin commented Feb 11, 2021

@NotAProfessionalDeveloper although there aren't many "hard and fast" rules for organizing your SLS scripts, the situation you have described might be more easily handled by two different State IDs:

file-to-update.conf:
  file.keyvalue
    - key: ...
    - value: ...

file-to-append.conf:
  file.append:
    - text: ...

Because these are two different files, the Salt standard would be to handle them as separate "steps" in your SLS file. This issue (#35592) was opened to address situations in which the changes enforced by a particular State would be considered one single operation on your local machine (for example, prepending and appending text to one file via a text-editor). Hypothetically, changes such as prepend+append could be managed by two states: prepend-file and append-file... but because states can use the State ID as the source-file/name/etc., and Salt won't let you reuse a State ID, it becomes inconvenient to separate these two steps that should both be performed on a single file.

@TheNetworkIsDown
Copy link

@NotAProfessionalDeveloper although there aren't many "hard and fast" rules for organizing your SLS scripts, the situation you have described might be more easily handled by two different State IDs:

file-to-update.conf:
  file.keyvalue
    - key: ...
    - value: ...

file-to-append.conf:
  file.append:
    - text: ...

Because these are two different files

Still, editing both files is part of the deployment of the same service: install the package, configure, start the service.

@faelin
Copy link

faelin commented Feb 11, 2021

Still, editing both files is part of the deployment of the same service: install the package, configure, start the service.

For the use case you describe, you should be using separate salt states with require keys:

some-package:
  pkg.installed

file-to-update.conf:
  file.keyvalue
    - key: ...
    - value: ...
    - require:
      - pkg: some-package

file-to-append.conf:
  file.append:
    - text: ...
    - require:
      - pkg: some-package

some-package-service:
  service.running:
    - enable: True
    - require:
      - file: /.../file-to-update.conf
      - file: /.../file-to-append.conf

This is the way Salt is expected to be used per the official documentation. The reason for this has to do, at least in part, with the completion of States and how they are confirmed; if you group all of those steps together, and any one part of them fails (such as the starting of the service), then the entire state is marked as failed when in fact maybe just the network connection was down when starting a networked service, etc etc.

@srkunze
Copy link
Contributor Author

srkunze commented Feb 15, 2021

then the entire state is marked as failed

That's actually a very good reason not to do it.

Also from a long-term user's perspective, I would recommend to avoid shared state ids altogether. They look nice in the beginning but later on it's easier when you setup grows.

@whytewolf
Copy link
Collaborator

So, I'm going to close this. there are some very big reasons this has not been considered yet.

first the way states actually work in the lower end data this actually can't work.

state_id:
  file.managed:
    - name: /tmp/test
    - source: salt://files/source
  file.append:
    - name: /tmp/test
    - text: "this change"

the above gets rendered down to this internally

{
"state_id": {
  "file": 
    [ "managed", 
      {"name":"/tmp/test"}, 
      {"source": "salt://files/source"}]
  }, { 
  "file":
    [ "append", 
      {"name": "/tmp/test"},
      {"text": "this change"}]
  }
}

if you know anything about python you know you can't have 2 labels with the same key in a python dict.

and if we tried merging them to get around this we end up with options without knowing which item goes to with function.

to change the way this functions is a huge change. and would take a complete rewrite of the way states work.

we just don't have the staff for a rewrite for only this functionality. I'm going to close this ticket with the above information.

the information about using 1 function per state id is a better work around and makes more sense in the overall state definition.

@henri9813
Copy link

Hello,

Thanks for your proposition, but what about:

state_id:
  file:
    managed:
      - name: /tmp/test
      - source: salt://files/source
    append:
      - name: /tmp/test
      - text: "this change"

Could this be a good "deal" ?
?

@whytewolf
Copy link
Collaborator

Hello,

Thanks for your proposition, but what about:

state_id:
  file:
    managed:
      - name: /tmp/test
      - source: salt://files/source
    append:
      - name: /tmp/test
      - text: "this change"

Could this be a good "deal" ? ?

That won't work. the entire system is built around lists. to switch to a dict would require rewriting the entire state system. which would also break backwards compatibility.

@henri9813
Copy link

Hello,

Okay thank you !

Thanks for maintaining saltstack !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core relates to code central or existential to Salt Feature new functionality including changes to functionality and code refactors, etc. Renderers
Projects
None yet
Development

No branches or pull requests