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

module.run documentation issues #43130

Open
boltronics opened this issue Aug 23, 2017 · 7 comments
Open

module.run documentation issues #43130

boltronics opened this issue Aug 23, 2017 · 7 comments
Labels
Bug doc-ex-missing docstring-update Documentation severity-medium State-Module time-estimate-sprint
Milestone

Comments

@boltronics
Copy link
Contributor

@boltronics boltronics commented Aug 23, 2017

Description of Issue/Question

According to https://docs.saltstack.com/en/latest/ref/states/all/salt.states.module.html#module-salt.states.module the new module.run syntax is:

fetch_out_of_band:
  module.run:
    git.fetch:
      - cwd: /path/to/my/repo
      - user: myuser
      - opts: '--all'

This example is incorrect. It should read:

fetch_out_of_band:
  module.run:
    - git.fetch:
      - cwd: /path/to/my/repo
      - user: myuser
      - opts: '--all'

All the other examples on that page also have the same error, which I found quite confusing.

That entire section doesn't read very well, as the old and new syntax formats are mixed together. It would be clearer if it was broken into sections with separate headings such as "The new way" and "The old way (supported until Oxygen)". It would also allow anyone new to module.run to simply skip the section on the old format.

Finally, I'd like to see a more complicated example. eg.

Remove appserver server from HAProxy host:
  module.run:
    - publish.publish:
      - tgt: 'rproxy*'
      - fun: haproxy_cmds.disable_server
      - arg: >-
          [
          'server=appserver',
          'backend=bk_appservers'
          ]
    - require_in:
      - git: Deploy code
    - onchanges_in:
      - module: Add appserver server to HAProxy host

I tried experimenting with a YAML list for arg, but it didn't work. This might save someone time.

Setup

N/A

Steps to Reproduce Issue

N/A

Versions Report

Applicable to the documentation in 2017.7.0+.

@Ch3LL
Copy link
Contributor

@Ch3LL Ch3LL commented Aug 25, 2017

@boltronics I don't think the new module.run is set by default. If i recall correctly to use the new syntax you would need to add an additional argument to your config. Is this accurate from your setup?

I completely agree we should add some examples to these docs for the new syntax but i don't think we want to do a clear removal of the old syntax because its still applicable i believe.

@Ch3LL Ch3LL self-assigned this Aug 25, 2017
@Ch3LL Ch3LL added Bug Documentation severity-medium P2 State-Module labels Aug 25, 2017
@Ch3LL Ch3LL added this to the Approved milestone Aug 25, 2017
@boltronics
Copy link
Contributor Author

@boltronics boltronics commented Aug 28, 2017

@Ch3LL That is correct. I have added the documented use_superseded argument to my config.

I didn't mean to imply that the old syntax should be removed. I just think it's not very clear at a glance which examples are using which syntax, and it would be more readable if the two different approaches were split out under separate sub-headings or some such.

In case it wasn't clear, both are currently documented on that page to some degree.

@epcim
Copy link
Contributor

@epcim epcim commented Jun 13, 2018

This should be closed I guess.

@boltronics
Copy link
Contributor Author

@boltronics boltronics commented Jun 14, 2018

The examples are fixed, but it's still not as clear as it could/should be IMO.

For example, the first four examples use the new syntax. Then the documentation switches to giving an example of the old way. Initially the reader would think it's just a quick one-off example of something they don't really need to care about, but then it goes on for another five additional examples. At this point, the docs read "Another option is to use the new version of module.run" and give one final example using the new format again.

So it comes off as unclear which the user should use. On one hand the new format is introduced first, but then the old format is discussed so much that it could easily give rise to some doubts. The fact that the new format requires a configuration file change to work doesn't help matters.

Additionally the use_superseded syntax is documented right at the end - after spending so much time talking about the old format. I imagine some readers would get half way through and think "the rest of this document doesn't apply to me - I'm using the new format" and completely miss that little minion configuration section at the very end and then spend hours debugging why their states aren't working correctly.

At least the information is all there now and correct, but without the readability and clarity problems being addressed, I think this probably shouldn't have been closed so quickly.

@Ch3LL Ch3LL reopened this Jun 15, 2018
@flassman
Copy link

@flassman flassman commented Sep 21, 2018

I agree with boltronics. The naming of the "use_superseded" switch is weird enough but should be mentioned right on the top, then there should be a section for new format and finally a section for old format.
I hope there is a plan for deprecation of the old format or at least for making the new format a default? If yes, this should be mentioned as well. If not, please decide how this should be handled, to avoid further confusion.

@byteranger
Copy link

@byteranger byteranger commented Nov 16, 2018

I just wasted my morning because of the lack of clarity in the documentation as @boltronics has pointed out. Please fix this.

@ntrepid8
Copy link

@ntrepid8 ntrepid8 commented Jul 8, 2020

I struggled with this for hours on version 3001 until I tried adding the use_superceded configuration to the minion. The documentation indicates that in version 3001 the "new style" should be the only thing that works:

The legacy style will no longer be available starting in the 3001 release.

However, the "new style" did not work for me on version 3001 until I added this to my /etc/salt/minion configuration file:

use_superseded:
  - module.run

Did we ever make the switch to using the new style only in version 3001? Given how the documentation reads I would not expect to need to use use_superceded on 3001 to get the new style to work.

If the change has been made, is there some possibility that upgrading the minion from 2017.7.4 to 3001 could create a state where the legacy style was still active?

Right now I'm wondering if I really need to modify the configuration of every single minion or if it's just easier to use the legacy style instead of the new style.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug doc-ex-missing docstring-update Documentation severity-medium State-Module time-estimate-sprint
Projects
None yet
Development

No branches or pull requests

9 participants