Implement ability to limit module documentation building #24576

azaghal · 2017-05-13T11:53:57Z

SUMMARY

The purpose of this pull request is to introduce option for limiting which module documentation gets built. Implementation achieves this via MODULES environment variable that can get passed-in to make webdocs.

The reason for this change is to allow both module developers and documentation contributors to more quickly see and test their changes. Building entire documentation for all modules can take quite a while, and it deters from fixing small syntax, typos and similar errors.

ISSUE TYPE

Docs Pull Request

COMPONENT NAME

Documentation build system

ANSIBLE VERSION

ansible 2.4.0 (doc_module_selection 7b6dd1f4d0) last updated 2017/05/13 13:43:50 (GMT +200)
  config file = 
  configured module search path = [u'REDACTED', u'/usr/share/ansible/plugins/modules']
  ansible python module location = REDACTED
  executable location = REDACTED
  python version = 2.7.12 (default, Dec 13 2016, 14:29:11) [GCC 4.9.3]

ADDITIONAL INFORMATION

In order to test the change, you can do the following:

cd docs/docsite/

# Build documentation for apt module.
MODULES=apt make webdocs

# Build documentation for a couple of modules.
MODULES=apt,setup,yum make webdocs

# Don't build documentation for modules.
MODULES=none make webdocs

# Build documentation for all modules.
MODULES=all make webdocs
make webdocs

ansibot · 2017-05-13T12:35:42Z

The test ansible-test sanity --test pep8 failed with the following error:

docs/bin/plugin_formatter.py:203:161: E501 line too long (163 > 160 characters)

click here for bot help

ansibot · 2017-05-13T12:35:43Z

cc @dharmabumstead
click here for bot help

- Added new option to plugin_formatter.py to support passing-in a list of modules for which the documentation should be built. - Updated docuemtnation Makefile to allow specifying list of modules via environment variables (defaulting to all modules). - Update instructions for building documentation and module development to include commands and description of limiting module documentation builds.

azaghal · 2017-05-13T13:21:46Z

Ok, next time I should run the sanity test myself :P

dharmabumstead

Fantastic - thanks @azaghal!

dharmabumstead · 2017-05-15T09:06:34Z

Looks good to me. Someone from the core team please review and merge if no issues.

bcoca · 2017-05-18T16:53:40Z

making it a 'limit' but also forcing to specify seem counter indicative, I would either rename to 'list' or would default to None/absent and filter against list if it is passed in.

Also i would make into list on args parsing, not in the function that is rendering.

other than that minutia, looks good

alikins · 2017-05-18T17:14:57Z

docs/bin/plugin_formatter.py

@@ -125,12 +125,13 @@ def write_data(text, options, outputname, module):
 #####################################################################################


-def list_modules(module_dir, depth=0):
+def list_modules(module_dir, depth=0, limit_to_modules="all"):


I think I would make limit_to_modules arg a list here, and do the comma split on the cli arg before calling list_modules.

(currently it looks this would do odd things if attempting to build module docs for a module with a name like 'ec2_vps_cwt_jkg_none_facts' or 'firewalld' or 'easy_install'? Haven't tested though so that may be covered...)

limit_to_modules is currently supposed to be comma-separated string, so modules with mentioned names should not be a problem.

I'll see what I can do to switch to lists and conditionals. One problem is my Makefile expertise is virtually non-existent (and don't have much time/will to learn it atm :)

Hm... Another thing is how to specify "no modules" at all. I.e. without using the "all" or "none" keywords? Or would it be ok to preserve a variant where value of all or none means that?

Sheesh... So, before I get any deeper into this, the limit_to_modules parameter received after option parser is invoked should end-up producing one of the following values:

None - no limitation, default if option is not specified.

[] - don't build any modules. This should be set if you specify special keyword - --limit-to-modules none.

['a', 'b', 'c'] - build only specified module. This should be set if you specify --limit-to-modules a,b,c.

Three ways that I can see to go around this (with one simply not working):

Massage option value after call to (options, args) = p.parse_args().

Add a new type to option parser.

Use callback, but this won't work since we need to set type, and there is no list type (plus, we need list + None type in this particular case I guess).

Preferences? Or am I just overcomplicating this?

- Pass list of modules (or None) to list_modules function instead of string. - Move conversion of module list to argument parsing code. - No special keywords. Default ("") means build all modules. For no modules just specify non-existing module name. - Updated documentation to reflect the changes.

azaghal · 2017-05-18T19:08:49Z

Ok, updated the pull request based on comments from review (gave-up on complicated solution :).

bcoca · 2017-05-18T20:11:10Z

docs/bin/plugin_formatter.py

@@ -194,6 +197,8 @@ def generate_parser():
    p.add_option("-v", "--verbose", action='store_true', default=False, help="Verbose")
    p.add_option("-o", "--output-dir", action="store", dest="output_dir", default=None, help="Output directory for module files")
    p.add_option("-I", "--includes-file", action="store", dest="includes_file", default=None, help="Create a file containing list of processed modules")
+    p.add_option("-l", "--limit-to-modules", action="store", dest="limit_to_modules", default="",


making None the default saves you some code

Ah, right... Was playing around with setting type, and that one did not like None. Will fix.

Actually... I don't think it does, but I will fix it.

Pushed again.

- Use better default value, and don't treat "" as special case. - Conditionally invoke different variants of command in Makefile instead of using special value "".

alikins · 2017-06-08T15:25:36Z

docs/bin/plugin_formatter.py

-    mod_info, categories, aliases = list_modules(options.module_dir)
+    # Convert passed-in limit_to_modules to None or list of modules.
+    if options.limit_to_modules is not None:
+        options.limit_to_modules = [s.lower() for s in options.limit_to_modules.split(",")]


dont reassign options.limit_to_modules, just created a new limit_to_modules var

if options.limit_to_modules is not None: limit_to_modules = [s.lower() for s in options.limit_to_modules.split(",")] mod_info, categories, aliases = list_modules(options.module_dir, limit_to_modules=limit_to_modules)

I wouldn't block on this but stylewise, I do agree with alikins that it's better to do all modifications of things in options inside of the argument parsing functions and then set other variables if the code is outside of it. (In my own code I tend to have a parse_options function that would combine generate_parser, validate_options, and also do transformations on the arguments like this. That function would return the options data structure at the end and then I'd try to treat it as immutable thereafter).

gundalow · 2017-06-19T07:39:40Z

so @nitzmahone #25313 gives the ability to generate a single non-module html page. Combined with this we can generate any page.

This would save lots of people lots of time, so +1000 from me

abadger · 2017-06-19T08:20:18Z

+1 for this. Note that running html docs build with a subset of modules can lead to false positive errors (due to modules being able to link to other modules and those other modules perhaps not being present). So you do have to be careful if you add this to some sort of automated testing.

jhawkesworth · 2017-06-19T09:08:06Z

Likely we don't need #25791 if this gets merged, so cross referencing it.
Would perhaps be nice if this wasn't limited just to modules and you could use it to build a page in the documentation too.

azaghal · 2017-06-19T09:39:57Z

Given that #25313 got merged, not sure this pull request is relevant? I actually thought I had closed it already :)

gundalow · 2017-06-26T09:48:54Z

What's the plan for this & #25791
Looks like we are very close to amazingness, just need to get the ability to build a single module done and then update the docs.

azaghal · 2017-06-28T10:26:05Z

Well, at least I have no idea what the plans are. I took a bit different approach compared to #25313, more centered around modules. For what it's worth, my patch is faster on initial build (since processing of most modules is skipped). Otherwise, the other approach covers everything (minus the docs :)

jhawkesworth · 2017-06-28T12:14:57Z

I updated #25791 with docs and shellcheck fixes now.
I just wanted an easy-to-remember command I could run to build a bit of the docs as a full build of the docs takes longer than my daily train journey. No objection to making ansible-makepage.sh faster though, if it gets accepted.

mattclay · 2017-06-28T14:19:53Z

docs/docsite/README.md

+make webdocs` instead. This should make testing module documentation syntax much
+faster. Instead of a single module, you can also specify a comma-separated list
+of modules. In order to skip building documentation for all modules, specify
+non-existing module name, for example `MODULES=none make webdocs`.


I recommend passing vars as args in all new make examples. Example: make webdocs MODULES=none

Also, use a non-existent module name instead of non-existing module name.

Not a blocker, just a suggestion to improve readability. If you do change it, update all new occurrences.

gundalow · 2017-06-29T15:47:22Z

We've agreed to go with this and close #25791

Wording tweak

dharmabumstead

Docs OK with some minor edits. Can't wait to implement this. Thanks @azaghal!

azaghal · 2017-07-19T10:52:07Z

Sorry for late reply (as usual...), but going through my mail backlog :)

Should I continue in the same direction, probably just double-checking on reviews? I am happy to invest time as long as it's not a dead (or super-prolonged) end :)

dharmabumstead · 2017-08-04T20:10:48Z

Thanks very much @azaghal!

azaghal · 2017-08-11T11:50:30Z

Oh, no changes needed in the end? Great, I hope it will help people :)

azaghal force-pushed the doc_module_selection branch from 7b6dd1f to 7561797 Compare May 13, 2017 12:51

ansibot removed ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels May 13, 2017

gundalow requested review from abadger, mattclay, bcoca, gundalow and dharmabumstead May 13, 2017 18:14

gundalow removed the needs_triage Needs a first human triage before being processed. label May 13, 2017

dharmabumstead approved these changes May 14, 2017

View reviewed changes

alikins reviewed May 18, 2017

View reviewed changes

bcoca reviewed May 18, 2017

View reviewed changes

Updated implementation for limiting module documentation building:

4d85dec

- Use better default value, and don't treat "" as special case. - Conditionally invoke different variants of command in Makefile instead of using special value "".

azaghal mentioned this pull request Jun 8, 2017

Public Core Meeting Meeting - April, May 2017 ansible/community#159

Closed

thaumos mentioned this pull request Jun 8, 2017

Public Core Meeting Agenda - June, July 2017 ansible/community#162

Closed

alikins reviewed Jun 8, 2017

View reviewed changes

ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jun 8, 2017

gundalow mentioned this pull request Jun 26, 2017

Adds docs/bin/ansible-makepage bash script, to make a single doc page with one command #25791

Closed

mattclay reviewed Jun 28, 2017

View reviewed changes

ansibot added the support:core This issue/PR relates to code supported by the Ansible Engineering Team. label Jun 29, 2017

Minor edits

0bc6d47

Wording tweak

dharmabumstead approved these changes Jun 29, 2017

View reviewed changes

ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jun 30, 2017

ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jul 18, 2017

dharmabumstead merged commit f78baa1 into ansible:devel Aug 4, 2017

ansibot added docs This issue/PR relates to or includes documentation. and removed docs_pull_request labels Mar 4, 2018

ansible locked and limited conversation to collaborators Apr 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement ability to limit module documentation building #24576

Implement ability to limit module documentation building #24576

azaghal commented May 13, 2017

ansibot commented May 13, 2017

ansibot commented May 13, 2017

azaghal commented May 13, 2017

dharmabumstead left a comment

dharmabumstead commented May 15, 2017

bcoca commented May 18, 2017

alikins May 18, 2017

azaghal May 18, 2017

azaghal May 18, 2017

azaghal May 18, 2017

azaghal May 18, 2017

azaghal commented May 18, 2017

bcoca May 18, 2017

azaghal May 18, 2017

azaghal May 18, 2017

azaghal May 18, 2017

alikins Jun 8, 2017

abadger Jun 19, 2017

gundalow commented Jun 19, 2017

abadger commented Jun 19, 2017

jhawkesworth commented Jun 19, 2017

azaghal commented Jun 19, 2017

gundalow commented Jun 26, 2017

azaghal commented Jun 28, 2017

jhawkesworth commented Jun 28, 2017

mattclay Jun 28, 2017

gundalow commented Jun 29, 2017

dharmabumstead left a comment •

edited

azaghal commented Jul 19, 2017

dharmabumstead commented Aug 4, 2017

azaghal commented Aug 11, 2017

Implement ability to limit module documentation building #24576

Implement ability to limit module documentation building #24576

Conversation

azaghal commented May 13, 2017

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

ADDITIONAL INFORMATION

ansibot commented May 13, 2017

ansibot commented May 13, 2017

azaghal commented May 13, 2017

dharmabumstead left a comment

Choose a reason for hiding this comment

dharmabumstead commented May 15, 2017

bcoca commented May 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azaghal commented May 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gundalow commented Jun 19, 2017

abadger commented Jun 19, 2017

jhawkesworth commented Jun 19, 2017

azaghal commented Jun 19, 2017

gundalow commented Jun 26, 2017

azaghal commented Jun 28, 2017

jhawkesworth commented Jun 28, 2017

Choose a reason for hiding this comment

gundalow commented Jun 29, 2017

dharmabumstead left a comment • edited

Choose a reason for hiding this comment

azaghal commented Jul 19, 2017

dharmabumstead commented Aug 4, 2017

azaghal commented Aug 11, 2017

dharmabumstead left a comment •

edited