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

Loader docs #50633

Merged
merged 29 commits into from Dec 10, 2018
Merged

Loader docs #50633

merged 29 commits into from Dec 10, 2018

Conversation

AstraLuma
Copy link
Contributor

@AstraLuma AstraLuma commented Nov 26, 2018

What does this PR do?

Add documentation about module loading and in general attempts to improve documentation about writing modules for salt.

What issues does this PR fix or reference?

#50594

Notes

I already know that there's changes for the 2018.3 branch. I'm not sure how to handle that?

  • Add token modules
  • File server modules can now be loaded from the file server
  • Search system was removed
  • Documentation for Executors was added.

For flourine:

@AstraLuma
Copy link
Contributor Author

@AstraLuma AstraLuma commented Nov 26, 2018

I am looking for some feedback about the restructuring I'm doing.

I am also thinking about writing some docs about specific module interfaces, for the module types that don't have it yet.


This is done via setuptools entry points:

.. code-block:: python
Copy link
Contributor

@cachedout cachedout Nov 26, 2018

Choose a reason for hiding this comment

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

This needs a newline below it or the documentation will not build.

Copy link
Contributor Author

@AstraLuma AstraLuma Nov 26, 2018

Choose a reason for hiding this comment

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

Thanks! Handled.

@AstraLuma
Copy link
Contributor Author

@AstraLuma AstraLuma commented Nov 26, 2018

@cachedout Did you have any feedback about the "big picture" changes I'm doing?

@cachedout
Copy link
Contributor

@cachedout cachedout commented Nov 27, 2018

@astronouth7303 I think that overall these changes make a lot of sense. 👍

@cachedout
Copy link
Contributor

@cachedout cachedout commented Nov 29, 2018

@astronouth7303 Is this ready to go? If so, please remove WIP from the PR subject line.

@AstraLuma
Copy link
Contributor Author

@AstraLuma AstraLuma commented Nov 30, 2018

I want to look at ref/file_server/dynamic_modules.rst (since a lot of its content overlaps with what I've been writing). There's also a few other sections that need at least a cursory summary.

I'm also hoping to write full interface references for each of the systems, but I guess that can be saved for future PRs.

@cachedout
Copy link
Contributor

@cachedout cachedout commented Nov 30, 2018

@astronouth7303 Sounds good. Just let us know when it's all ready to go. Thanks!

@AstraLuma AstraLuma changed the title [WIP] Loader docs Loader docs Dec 1, 2018
@AstraLuma
Copy link
Contributor Author

@AstraLuma AstraLuma commented Dec 1, 2018

Since I don't feel like writing detailed interface documentation for 20-some modules just yet, this is ready for a more intense review.

@max-arnold
Copy link
Contributor

@max-arnold max-arnold commented Dec 3, 2018

@cachedout Should the docs discourage using direct module imports? There are tons of them:

grep -r 'salt\.modules' salt/ | grep import

For example, there is an unconditional import of vsphere.py which is quite large and slow:

% grep -r 'salt\.modules.\vsphere' salt | grep import
salt/grains/esxi.py:import salt.modules.vsphere

% ls -la salt/modules/vsphere.py
-rw-r--r--  1 user  staff  373420 Oct 29 20:12 salt/modules/vsphere.py

What is the proper way to cross-reference loadable modules?

@max-arnold
Copy link
Contributor

@max-arnold max-arnold commented Dec 3, 2018

Also it would be useful to document all the disable_* loader options. So far only the disable_modules and disable_returners are documented: https://docs.saltstack.com/en/latest/ref/configuration/minion.html#minion-execution-module-management

In theory, disable_* should work:

  1. both in master and minion config
  2. for all module types https://github.com/saltstack/salt/blob/develop/salt/loader.py#L1180 (briefly mentioned in the PR #42567)

@cachedout
Copy link
Contributor

@cachedout cachedout commented Dec 3, 2018

@cachedout Should the docs discourage using direct module imports? There are tons of them:

grep -r 'salt\.modules' salt/ | grep import

For example, there is an unconditional import of vsphere.py which is quite large and slow:

% grep -r 'salt\.modules.\vsphere' salt | grep import
salt/grains/esxi.py:import salt.modules.vsphere

% ls -la salt/modules/vsphere.py
-rw-r--r--  1 user  staff  373420 Oct 29 20:12 salt/modules/vsphere.py

What is the proper way to cross-reference loadable modules?

IIRC, there are a few very specialized cases in this turns out to be necessary, but as a general practice, cross-calling modules should be done using the __salt__[module_name.module_func] form. I agree that this would be a good idea to put this in the docs.

@AstraLuma
Copy link
Contributor Author

@AstraLuma AstraLuma commented Dec 4, 2018

In my experience, it's actually pretty hard to do for any modules that aren't shipped with salt. But it's been noted.

@AstraLuma
Copy link
Contributor Author

@AstraLuma AstraLuma commented Dec 4, 2018

Added both a note about importing and a quick list of loader-related settings.

@max-arnold
Copy link
Contributor

@max-arnold max-arnold commented Dec 4, 2018

Undocumented settings like disable_grains, disable_beacons etc, can have significant performance impact: #48773 (comment)

@AstraLuma
Copy link
Contributor Author

@AstraLuma AstraLuma commented Dec 4, 2018

I'm cutting this PR off at conducting discovery for undocumented settings and documenting them. I really wanted to focus on documenting the salt.load entry point and what's the name for all of the systems in various contexts (since some vary by pluralization, etc). I also ended up doing some dunder completionism since I was hip-deep in loader.py anyways.

I also don't think setting docs should live in this corner of the documentation. They should live with the settings reference and be referred to.

@AstraLuma
Copy link
Contributor Author

@AstraLuma AstraLuma commented Dec 4, 2018

And for some reason, the link to the whitelist_modules setting isn't working locally.

EDIT: Figured it out, the link ID is apparently enable_whitelist_modules for some reason.

@AstraLuma
Copy link
Contributor Author

@AstraLuma AstraLuma commented Dec 6, 2018

@cachedout Can I get your updated review?

@cachedout cachedout merged commit e4e9563 into saltstack:2017.7 Dec 10, 2018
6 of 10 checks passed
@AstraLuma AstraLuma mentioned this pull request Jan 17, 2019
@max-arnold max-arnold mentioned this pull request May 1, 2019
@AstraLuma AstraLuma deleted the loader-docs branch Apr 22, 2021
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.

None yet

3 participants