-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
[EPIC] Documented options in minion/master config files don't match minion/master config documentation, and vice versa #58112
Comments
I also decided to audit the master related conf and docs, in a similar fashion, after seeing that an issue was previously opened back in 2016 concerning documenting both minion and master configs: #32400 Master audit#!/usr/bin/env bash
# Format for option names
sed -n 1,5818p doc/ref/configuration/master.rst | \
egrep '\-\-\-|\*\*\*' -B1 | \
grep -v ':' | \
egrep -v '\-\-|\*\*| ' | \
sed 's/``//g' | \
sort > master-options-docs.txt
# Grab all config options for master conf from conf
cat conf/master | \
grep ':' | \
egrep -v "^# |\." | \
sed 's:#::g' | \
cut -d':' -f1 | \
sort | \
uniq > master-options-conf.txt
# What's potentially missing from master conf docs?
diff master-options-docs.txt master-options-conf.txt | \
grep '>' | \
sed 's:>\ ::g' > settings-missing-from-master-options-docs.txt
# What's missing from master conf file?
diff master-options-docs.txt master-options-conf.txt | \
grep '<' | \
sed 's:<\ ::g' > settings-missing-from-master-options-conf.txt This ends up creating two files:
This seems to say that quite a few options are missing from the Missing from
|
Also: according to #32400, the following are completely undocumented at the moment and aren't to be found in the above audits of master/minion config files or config documentation:
It looks like the best source for all available options is: Which, running an audit against all default options for master/minion here led to the following 49 options potentially needing to be documented (or removed from
|
ran into the need for |
Options like Lines 3854 to 3855 in 3f30104
They should be included in VALID_OPTS and in DEFAULT_MASTER_OPTS (as empty strings). They need to be included like pki_dir is (as an str ), with a default value in default master opts as "" :
Lines 180 to 181 in 3f30104
A messy audit shows the following options that may or may not be undocumented: egrep -n "opts\[\".*\"\] \=" salt/config/__init__.py | grep -v "opts\[\"\_"
Further filtered: egrep "opts\[\".*\"\] \=" salt/config/__init__.py | grep -v "opts\[\"\_" | cut -d'"' -f2 | sort | uniq
These would require further research by SME's to learn whether other options here - if not already picked up in the other audits within this issue - need to be included in places like |
This is a really good research! My two cents:
I'm not sure if it is practical, but maybe it makes sense to wrap the options dictionary into a class (only in unit tests) that logs every key access attempt. That (and some log/docs parsing automation) could help reveal other undocumented options as well as prevent adding new ones. |
By the way, many minion options are also useful in the MasterMinion context (orchestration states). For example, you can add the
|
But the leaky bucket needs to be fixed first, otherwise it would be a never-ending job... Another undocumented option has been added: #59833 |
Description
There are options listed in the
conf/minion
file that are undocumented in the minion configuration documentation, doc/ref/configuration/minion.rst, and vice versa.Setup
Clone the latest version of this repository.
Steps to Reproduce the behavior
See the information in Additional context below.
Expected behavior
The options listed in the minion configuration file and the minion configuration documentation should match, or at a minimum the minion configuration documentation should include information regarding all options listed in the
conf/minion
file. This is the bug.An improvement/enhancement could be for the
conf/minion
file to also include all the options in the minion configuration documentation that are currently missing. This would be helpful for users, such as what was experienced in #57931Additional context
I whipped up these scripts to find all options missing from the
conf/minion
file that are listeddoc/ref/configuration/minion.rst
, and vice versa.Minion audit
This ends up creating two files:
settings-missing-from-minion-options-conf.txt
settings-missing-from-minion-options-docs.txt
This seems to say that quite a few options are missing from the
minion
conf file, and some are missing from the minion configuration documentation.Missing from
conf/minion
Source file: https://github.com/saltstack/salt/blob/master/conf/minion
conf/minion
Missing from
doc/ref/configuration/minion.rst
Source file: https://github.com/saltstack/salt/blob/master/doc/ref/configuration/minion.rst
doc/ref/configuration/minion.rst
Other Thoughts
Are the minion configuration options auto-populated anywhere? If not, it would be a good idea to introduce automation that would autopopulate content in either the
minion
conf file, or the rst documentation page (or both, ideally). That, or audit to see whether there are currently conf options that aren't included in theminion
conf, nor the rst documentation page.The text was updated successfully, but these errors were encountered: