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

[WIP] Docs: Read arg_spec documentation from module #45805

Open
wants to merge 3 commits into
base: devel
from

Conversation

@dagwieers
Member

dagwieers commented Sep 18, 2018

SUMMARY

This PR includes:

  • Reading arg_spec documentation from module
    (no longer do we need to duplicate parameter details in documentation)
  • Showing parameter type for every parameter
  • Show type in purple
  • Convert types to fully written variant

To do:

  • Fix support for suboptions
  • Add support for ansible-doc
  • And we can also auto-generate things like:
    • "This parameter is deprecated and will be removed in Ansible X.Y" (removed_in_version)
    • "This module supports check-mode" (supports_checkmode)
    • "Parameter x is mutual exclusive with parameter y" (mutually_exclusive)
    • "Parameter x and y need to be used together" (required_together)

Impact:

  • Contributors no longer have to ensure documentation matches the module arg_spec.
  • Reviewers don't need to verify the documentation, only descriptions.
  • Should work transparently, so we can clean up the modules in a phased manner.
ISSUE TYPE
  • Docs Pull Request
COMPONENT NAME

Module docs

ANSIBLE VERSION

v2.8 and earlier

ADDITIONAL INFORMATION

screenshot from 2018-09-18 18-26-00

@dagwieers dagwieers added the docs label Sep 18, 2018

@dagwieers dagwieers requested a review from acozine Sep 18, 2018

@acozine

This comment has been minimized.

Contributor

acozine commented Sep 18, 2018

@dagwieers is this a replacement for #42888?

@dagwieers

This comment has been minimized.

Member

dagwieers commented Sep 18, 2018

@acozine Not really, this is about adding types the right way. I hope we can find a better solution for indicating required parameters.

@acozine acozine added this to To do in Ansible Core Documentation via automation Sep 18, 2018

@dagwieers dagwieers changed the title from Show parameter types (in purple) to Docs: Show parameter types (in purple) Sep 18, 2018

@dagwieers dagwieers force-pushed the dagwieers:module-docs-type branch Sep 19, 2018

@ansibot

This comment was marked as resolved.

Contributor

ansibot commented Sep 19, 2018

Thanks @dagwieers! This PR was evaluated as a bad PR for the following reasons:

  • More than 50 changed files.

Such PR can only be merged by human, contact core team member to review this PR on IRC: #ansible-devel on irc.freenode.net

click here for bot help

@dagwieers dagwieers force-pushed the dagwieers:module-docs-type branch Sep 19, 2018

@dagwieers dagwieers changed the title from Docs: Show parameter types (in purple) to [WIP] Docs: Show parameter types (in purple) Sep 19, 2018

@dagwieers dagwieers requested a review from gundalow Sep 19, 2018

@dagwieers dagwieers changed the title from [WIP] Docs: Show parameter types (in purple) to [WIP] Docs: Read arg_spec documentation from module Sep 19, 2018

@ansibot

This comment has been minimized.

Contributor

ansibot commented Sep 19, 2018

@gundalow

This comment has been minimized.

Contributor

gundalow commented Sep 19, 2018

Could you please split out the module changes from the docs changes to allow us to backport as needed.

@gundalow

Great idea

I know this is WIP, but I hough some initial thoughts.

If we go ahead with this I think it would need to be multiple PRs
PR1: plugin formatter & template - could be back ported into stable-2.7
PR2: remove type from validate-modules's schema.py`, remove from How to document your module, remove from all modules. No nsure if we've want to backport that, would need to ask Toshio.

We can revisit this after AnsibleFest.

docs/bin/plugin_formatter.py Outdated
@@ -58,6 +58,9 @@ def html_escape(text, quote=True):
from ansible.utils.display import Display
from ansible.utils._build_helpers import update_file_if_different
sys.path.append('../../test/sanity/validate-modules')
from module_args import AnsibleModuleImportError, get_argument_spec

This comment has been minimized.

@gundalow

gundalow Sep 19, 2018

Contributor

Oh, very nice.

This comment has been minimized.

@dagwieers

dagwieers Sep 19, 2018

Member

IMO it's an ugly hack. I think this functionality should move into Ansible to be used both by test-scripts and doc-generators.

This comment has been minimized.

@webknjaz

webknjaz Sep 19, 2018

Member

Exactly

docs/bin/plugin_formatter.py Outdated
# args = list()
# kwargs = dict()
display.error('BLAH' + str(spec))

This comment has been minimized.

@gundalow

gundalow Sep 19, 2018

Contributor

ERROR rather than BLAH :)

This comment has been minimized.

@dagwieers

dagwieers Sep 19, 2018

Member

It is a WIP. There's a very weird issue with Windows modulesthe user module where information coming from this module corrupts the next processed module. And I cannot pinpoint how this can happen. Part of the logic is skipped in mysterious ways.

This comment has been minimized.

@dagwieers
<b>@{ key }@</b></br>
{# Clean up parameter types #}
{% if value.type is not defined or value.type == 'str' %}
{% set type = 'string' %}

This comment has been minimized.

@gundalow

gundalow Sep 19, 2018

Contributor

+1 to informing the reader what the default type is

This comment has been minimized.

@dagwieers

dagwieers Sep 19, 2018

Member

Well, this change stands on its own (see first commit). The only problem is that the majority of the modules have incorrect documentation. So I have to fall back on the arg_spec. There's no practical way of fixing all the documentation errors. I started doing it, but I gave up. Fixing this once and for all is going to be a major improvement in contributor experience (and reviewing).

@@ -135,7 +149,7 @@ Parameters
{% endif %}
{# Show possible choices and highlight details #}
{% if value.choices %}
<ul><b>Choices:</b>
<ul style="margin: 0; padding: 0"><b>Choices:</b>

This comment has been minimized.

@gundalow

gundalow Sep 19, 2018

Contributor

What does this achieve?

This comment has been minimized.

@dagwieers

dagwieers Sep 19, 2018

Member

First of all, this should all go into CSS, but you know how difficult it is to ensure this turns up in the right place. And before we were told that CSS was off limits, that's why everything is hard-coded here. Should be fixed at some point though.

This comment has been minimized.

@dagwieers

dagwieers Sep 19, 2018

Member

But it causes a listing to not have trailing whitespace (and so we get more condensed tables).

@gundalow gundalow requested a review from sivel Sep 19, 2018

@dagwieers

This comment has been minimized.

Member

dagwieers commented Sep 19, 2018

This is a WIP, and for testing I need to have these modules in the same branch.

I do not plan to rewrite every module doc, the whole purpose of this implementation is that this is not required. We merge the docs with the arg_spec, but the docs have priority over the arg_spec.

The only thing we need in the arg_spec is a way to hide certain stuff from the docs. As some modules have parameters that should not be exposed to the user.

@dagwieers dagwieers added this to In progress in Contributor Experience via automation Sep 19, 2018

@ansible ansible deleted a comment from webknjaz Sep 19, 2018

@ansible ansible deleted a comment from webknjaz Sep 19, 2018

@dagwieers dagwieers force-pushed the dagwieers:module-docs-type branch Sep 19, 2018

@dagwieers

This comment has been minimized.

Member

dagwieers commented Sep 20, 2018

So the status now is that if I fix the CI errors locally, it fails the sanity tests locally. If I fix them locally, they fail in Shippable :-/ Possibly a Python 2 vs Python 3 thing ?

@sivel sivel requested a review from abadger Sep 20, 2018

@sivel

This comment has been minimized.

Member

sivel commented Sep 20, 2018

One thing I feel should be noted here, is that by moving module_args.py into the main codebase, we now have "production" code that makes use of mock.

@dagwieers

This comment has been minimized.

Member

dagwieers commented Sep 20, 2018

@sivel Take it up with @abadger as I followed his instructions to make that file private like we did elsewhere for building the docs. If that's not the way to do it, we need to devise something different out-of-tree.

@dagwieers

This comment has been minimized.

Member

dagwieers commented Sep 20, 2018

So this is not finished, as suboptions are currently not supported (the validate-modules test also do not support suboptions I think). It works well for modules that lack suboptions. Feedback welcome, but this is not the final product :)

@mattclay

This comment has been minimized.

Member

mattclay commented Sep 20, 2018

@dagwieers The sanity tests are run on Shippable using Python 3.6 -- so you could try running the tests with both Python 2.7 and 3.6 to see if you can reproduce the differences locally.

@dagwieers dagwieers force-pushed the dagwieers:module-docs-type branch 4 times, most recently from f04a809 to 43d4983 Sep 20, 2018

<b>@{ key }@</b>
{% if value.get('type', None) %}<br/><div style="font-size: small; color: red">@{ value.type }@</div>{% endif %}
{% if value.get('required', False) %}<br/><div style="font-size: small; color: red">required</div>{% endif %}
<b>@{ key }@</b></br>

This comment has been minimized.

@webknjaz

webknjaz Sep 21, 2018

Member

</br> -> <br />

This comment has been minimized.

@webknjaz

webknjaz Sep 21, 2018

Member

(or just <br>)

<b>@{ key }@</b>
<br/><div style="font-size: small; color: red">@{ value.type }@</div>
{% if value.version_added %}<br/><div style="font-size: small; color: darkgreen">(added in @{value.version_added}@)</div>{% endif %}
<b>@{ key }@</b></br>

This comment has been minimized.

@webknjaz

@dagwieers dagwieers removed the aci label Sep 21, 2018

@dagwieers dagwieers force-pushed the dagwieers:module-docs-type branch from 43d4983 to db65160 Sep 21, 2018

@webknjaz webknjaz requested a review from gundalow Sep 21, 2018

@abadger

This comment has been minimized.

Member

abadger commented Sep 21, 2018

One thing I feel should be noted here, is that by moving module_args.py into the main codebase, we now have "production" code that makes use of mock.

Yeah, we can't do that. mock isn't builtin to the stdlib until python3 and we don't want to add deps. (The previous bit of code didn't have any new deps...)

I guess we need to set up an alternate directory for code which is not part of ansible but needs to be shared by the build tools. Maybe hacking/lib? And env-setup should add that to PYTHONPATH (we'd probably also need the Makefile to modify PYTHONPATH if we do it this way)? Or we could have the tools add the directory to sys.path and then we would not need to modify both Makefile and env-setup.

We should probably move the _build_helper.py file to the new place as well.

@dagwieers

This comment has been minimized.

Member

dagwieers commented Sep 22, 2018

@abadger Ok, I'll do the necessary.

<br/><div style="font-size: small; color: red">@{ value.type }@</div>
<b>@{ key }@</b><br/>
{# Clean up parameter types #}
{% if value.type is not defined or value.type == 'str' %}

This comment has been minimized.

@felixfontein

felixfontein Sep 23, 2018

Contributor

How about putting this whole type detection logic into a jinja2 macro (or make it a filter), as it is repeated twice in this template?

This comment has been minimized.

@dagwieers
{% elif value.type == 'dict' %}
{% set type = 'dictionary' %}
{% else %}
{% set type = value.type %}

This comment has been minimized.

@felixfontein

felixfontein Sep 23, 2018

Contributor

If value.type happens to be raw, printing raw isn't exactly helpful :) I've seen raw for docker_container's command; also, grep -R "type='raw'" in lib/ansible/modules gives 54 results, so there are some more cases. (raw options usually don't have a type entry in the options part of DOCUMENTATION, so this will always show raw in these cases.)

This comment has been minimized.

@dagwieers

dagwieers Sep 23, 2018

Member

The idea is that in the case of raw, we can still override the type in the documentation. In most cases we have a preference in these cases, and we use raw either because of backward compatibility (we have to support e.g. both list and dicts) or because of exceptional circumstances.

This comment has been minimized.

@felixfontein

felixfontein Sep 23, 2018

Contributor

From how your change to _validate_argument_spec() looks, this currently isn't possible (because the type defined in the documentation shouldn't be raw). Anyway, I think we should make sure that the type defined in the documentation must be specified for raw options.

This comment has been minimized.

@dagwieers

dagwieers Sep 23, 2018

Member

I do not agree that we should mandate this. And I would like to keep that discussion out of this PR, but if necessary as a feature PR after this is merged.

This comment has been minimized.

@felixfontein

felixfontein Sep 24, 2018

Contributor

If that's the case, I would add handling of raw to print out something more useful for the user. The string raw itself is less useful (and more confusing) for most users than not showing the type (as we are currently doing) in my opinion.

This comment has been minimized.

@felixfontein

felixfontein Sep 24, 2018

Contributor

I agree that this discussion should be moved out of this PR, but I'd really like something more helpful than raw to be printed out by this PR :)

This comment has been minimized.

@dagwieers

dagwieers Sep 24, 2018

Member

I disagree that raw is less useful than not showing a type. At least you have something to work with, and look up/ask around what it means. It gives a clue, even if not the answer you were expecting.

Again, in most (if not all) cases we want the author to override the type in the documentation, but in some cases raw might be the best thing to return.

We intend to do a cleanup of certain things, and documenting the raw type parameters would be one. (In some cases the parameter description would say in this case: This parameter accepts list or dict input, look at the examples on how this works. Similar to how we now handle conditional defaults.

@gundalow gundalow added this to In progress in Contributor Experience Sep 25, 2018

dagwieers added some commits Sep 18, 2018

Show parameter types (in purple)
This PR includes:
- Showing parameter type for every parameter
- Show type in purple
- Convert types to fully written variant

@dagwieers dagwieers force-pushed the dagwieers:module-docs-type branch from db65160 to cc37b92 Oct 4, 2018

@gundalow gundalow moved this from In progress to Next in Contributor Experience Nov 8, 2018

@gundalow gundalow moved this from Next to Medium in Contributor Experience Nov 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment