Port #52368 doc requirement tests to master #55914
Conversation
And easy documentation purposes.
Unit test to require execution module documentation
ghost
requested a review
from 
…hugh19/salt into doctest_master
|
@max-arnold Turns out we sure can! Added a test to check each module's rst for the text ".. _virtual" which successfully detects problems with shadow and service execution module naming. With this additional check, the doc test is expected to fail based on the mentioned issues with shadow and service. I don't think correcting those should be part of his PR, so waiting for input from core team on next steps. I suggest merging #53296 and an equivalent for service. |
Codecov Report
@@ Coverage Diff @@
## master #55914 +/- ##
==========================================
+ Coverage 17% 17% +0.01%
==========================================
Files 1202 1202
Lines 224545 224679 +134
Branches 49238 49266 +28
==========================================
+ Hits 38153 38177 +24
- Misses 183555 183632 +77
- Partials 2837 2870 +33
|
|
Looks like some builds are failing |
|
@xeacott yep! Please review the posted comments. This is creating new tests, some of which are detecting currently broken things and I asked for how the core team wants it handled. I can remove the test for broken stuff and you can merge the doc fixes. Or the broken stuff can be fixed and you can merge the whole thing. Just let me know what you want here. |
|
Interesting. I'm guessing that something may have incorrectly patched the beacons log, unless it's simply not being set. I'll take a look at this. |
|
Hrm. Still seeing that same error, grr. Test passes on its own, no problem. |
| @@ -37,9 +37,11 @@ | |||
| except ImportError: | |||
| HAS_CRYPT = False | |||
|
|
|||
| __virtualname__ = "shadow" | |||
There was a problem hiding this comment.
Should we also be renaming this file?
There was a problem hiding this comment.
And I guess the linux_service.py
There was a problem hiding this comment.
As in, is it appropriate to mv shadow to linux_shadow and service to linux_service? Yes, they have to be updated. See the previous comments for more context.
From above:
Added a test to check each module's rst for the text ".. _virtual" which successfully detects problems with shadow and service execution module naming.
With this additional check, the doc test is expected to fail based on the mentioned issues with shadow and service. I don't think correcting those should be part of his PR, so waiting for input from core team on next steps. I suggest merging #53296 and an equivalent for service.
(The referenced PR was since merged into this one to get the mentioned tests passing)
The issue is #49335 and also #52000. Salt docs generate index pages for virtual modules. Since service is a virtual module name, having a module also named service means that any links to the service module, instead are directed to the virtual module index page. This means that on
https://docs.saltstack.com/en/2018.3/ref/modules/all/salt.modules.service.html you are unable to click on the service module link name in the middle, as it just loads the current page rather than the module doc.
This was a problem for the generically named shadow and service modules. A test was added in this PR to detect modules which duplicate virtual module index names, so they had to be renamed in order to keep tests passing.
There was a problem hiding this comment.
Okay, I'm pretty sure I'm down with that. If we're renaming things only within salt, but they're not part of the API, I think it's a great idea to name them consistently.
If it borked with the public API then we'd have to deprecate things, but if people shouldn't be importing this code themselves then I'm 💯
There was a problem hiding this comment.
As far as I know, module file names are not part of the salt api. Advice has always been to call other modules via __salt__['whatever'] rather than direct imports (https://docs.saltstack.com/en/latest/ref/modules/index.html#cross-calling-execution-modules).
Feel free to discuss with the everyone, but as far as I know, this should be a-ok!
|
tests are green! (pre-commit looks like a new, not yet working one) |
|
@waynew thoughts on getting this merged? Already seeing merge conflicts. |
|
@waynew LGTM, you good on this now? |
|
@dwoz yeah I'm good 👍 |
|
@mchugh19 🎉 🚀 📖 |
What does this PR do?
Ports #52368 to master
What issues does this PR fix or reference?
Adds tests to ensure modules have needed doc entries and ensures all defined doc entries have associated modules
Tests written?
Yes
Commits signed with GPG?
No