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

Pull documentation of ansible.module_utils from its doc strings. #48416

Merged

Conversation

aknrdureegaesr
Copy link
Contributor

@aknrdureegaesr aknrdureegaesr commented Nov 9, 2018

SUMMARY

I find the current module_utils - documentation not very helpful.

The source code itself contains better documentation (even though not perfect).

This pull request activates the documentation from the source code for docs.ansible.com
and improves on that documentation somewhat, in particular, so that the tests pass.

ISSUE TYPE
  • Docs Pull Request
COMPONENT NAME

module_utils

@ansibot
Copy link
Contributor

ansibot commented Nov 9, 2018

Hi @aknrdureegaesr, thank you for submitting this pull-request!

click here for bot help

@ansibot
Copy link
Contributor

ansibot commented Nov 9, 2018

@ansibot ansibot added affects_2.8 This issue/PR affects Ansible v2.8 core_review In order to be merged, this PR must follow the core review workflow. docs This issue/PR relates to or includes documentation. needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Nov 9, 2018
@mkrizek mkrizek removed the needs_triage Needs a first human triage before being processed. label Nov 9, 2018
@ansibot

This comment has been minimized.

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Nov 9, 2018
@aknrdureegaesr
Copy link
Contributor Author

aknrdureegaesr commented Nov 9, 2018

Interestingly, the present pull request is actually building the documentation for the module_utils that's installed in the load path of the python that's being used to run the build, not (as was intended) the module_utils of the ansible that's being build.

(For the record: The new version of this pull request, commit 456df7000024be4fbaba289f20492200056561b6, fixes that.)

@aknrdureegaesr aknrdureegaesr force-pushed the improve_module_utils_documentation branch from d448aa4 to 456df70 Compare November 9, 2018 23:35
@ansibot ansibot added the support:community This issue/PR relates to code supported by the Ansible community. label Nov 9, 2018
@ansibot

This comment has been minimized.

@aknrdureegaesr aknrdureegaesr force-pushed the improve_module_utils_documentation branch from 456df70 to 2e84d84 Compare November 10, 2018 08:44
@aknrdureegaesr
Copy link
Contributor Author

aknrdureegaesr commented Nov 14, 2018

The test passes my change on centos 6 but fails it on centos 7. I find this suspicious, as the change itself is documentation-only, so it should not be platform - dependent. Probing into the centos 7 failure, I find (slightly edited for readability):

https://download.fedoraproject.org/pub/epel/7/x86_64/repodata/721a6ec0aacec22adcb969c3e421a19ab4805ade557b2e6a6cda3c73ab0253c0-updateinfo.xml.bz2: [Errno 14] HTTP Error 302 - Found
Trying other mirror.
One of the configured repositories failed (EPEL yum repo),
and yum doesn't have enough cached data to continue. At this point the only
safe thing yum can do is fail. ...\n\n 

So I presume a flaky test environment. To test this assumption, I'll re-trigger the build by simply re-basing, without any further change, and see what happens.

@aknrdureegaesr aknrdureegaesr force-pushed the improve_module_utils_documentation branch from 2e84d84 to dbc0d1b Compare November 14, 2018 11:30
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Nov 16, 2018
Copy link
Contributor

@gundalow gundalow left a comment

Choose a reason for hiding this comment

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

Nice PR. We can look at doing similar for other parts of the code base in the future.

# not those may happen to be installed in the version of Python used to run sphinx.
# As sphinx loads in order to document, the repository version needs to be loaded:
sys.path.insert(0, os.path.abspath(os.path.join('..', '..', '..', 'lib')))

VERSION = '2.7'
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, would expect this to be 2.8 on devel, perhaps it isn't used by anything.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gundalow I'm not sure what that does - I updated it (from 2.6) as part of the 2.7 release, thinking perhaps the VERSION setting in the Sphinx config should match the latest setting on the docsite. We could experiment . . .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm in the same boat with you regarding the "not being sure".

So I left this alone. I think it is not needed for the purpose of this pull request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Being curious, I did an experiment, namenly, setting this VERSION to 3.14, then running make clean && make webdocs and greping all files for 3.14. Lessons learned:

"3.14" was not a good choice, as many tests contain an IP 11.12.13.14.

Every single .html - file generated contains a piece of javascript like this:

    <script type="text/javascript">
      var DOCUMENTATION_OPTIONS = {
        URL_ROOT:'./',
        VERSION:'3.14',
        COLLAPSE_INDEX:false,
        FILE_SUFFIX:'.html',
        HAS_SOURCE:  false
      };
    </script>

, checked by

$ for f in $(find docs/docsite/_build/html -name \*.html); do grep -q "VERSION:'3.14'" $f || echo $f; done
$

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for running the experiment, @aknrdureegaesr. I'll open a separate PR to update the version to 2.8 on devel.

This functionality is available via ``import ansible.module_utils.basic``:

.. automodule:: ansible.module_utils.basic
:members:
Copy link
Contributor

Choose a reason for hiding this comment

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

What does members limit this to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"Limit" does not describe it very well... Just saying automodule:: just gives the doc string of the module itself. I considered it useful to also have the documentation of the module's members, so I added :members:. The sphinx doc explains this.

@aknrdureegaesr
Copy link
Contributor Author

I don't think you are waiting for anything I should do, or are you?

@acozine
Copy link
Contributor

acozine commented Nov 20, 2018

@aknrdureegaesr we're not waiting on you, no - we might discuss this PR at the docs working group meeting today - starts in five minutes on the #ansible-docs channel on Freenode IRC.

@aknrdureegaesr
Copy link
Contributor Author

In not-so-important news, this PR ended up not being discussed in the meeting.

@ansibot 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 Nov 25, 2018
@acozine
Copy link
Contributor

acozine commented Nov 27, 2018

@aknrdureegaesr very true, would you like to add it properly to next week's agenda? If you can join us, to answer any questions and speak up for your PR, that would be even better!

@acozine acozine added this to To do in OLD Ansible Documentation via automation Nov 27, 2018
@aknrdureegaesr aknrdureegaesr force-pushed the improve_module_utils_documentation branch from dbc0d1b to 48e8c74 Compare December 4, 2018 21:59
@aknrdureegaesr
Copy link
Contributor Author

At this point in time, the result of 48e8c74 is (temporarily) available on my server.

@ansibot
Copy link
Contributor

ansibot commented Dec 4, 2018

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/module_utils/common/file.py:33:1: E302 expected 2 blank lines, found 1

click here for bot help

@ansibot ansibot added docsite This issue/PR relates to the documentation website. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Dec 4, 2018
@aknrdureegaesr aknrdureegaesr force-pushed the improve_module_utils_documentation branch 2 times, most recently from 6a4958d to 35a237f Compare December 5, 2018 10:22
@aknrdureegaesr
Copy link
Contributor Author

aknrdureegaesr commented Dec 5, 2018

Blocked by broken build

My current plan:

@aknrdureegaesr
Copy link
Contributor Author

Ready for merge!

The test status has turned to "All checks have passed" without my doing anything. Fine. In my opinion, this thing can and should now be merged into devel. This was agreed upon in the ansible-docs-meeting day before yesterday.

ready_for_review

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Dec 6, 2018
@aknrdureegaesr aknrdureegaesr force-pushed the improve_module_utils_documentation branch from 35a237f to a947785 Compare December 7, 2018 10:51
@aknrdureegaesr
Copy link
Contributor Author

I did happen to see an opportunity for improved language in one comment, which I acted upon.

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Dec 7, 2018
@aknrdureegaesr
Copy link
Contributor Author

aknrdureegaesr commented Dec 9, 2018

A build had been red (undeservedly), we "fixed" that be rerunning without change until green (thank you, @gundalow !), so now it should all be ready for @acozine (or someone else) to have a look and merge. But somehow, this still has a label "needs_revision". I try to remove this label by:

ready_for_review

Later remark: Doesn't work. Apparently, as a mere contributor, I do not have the power to clean up wrong labels (which comes at no surprise).

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Dec 9, 2018
@gundalow
Copy link
Contributor

@aknrdureegaesr It worked, label was removed.

@acozine acozine merged commit 18bf48c into ansible:devel Dec 10, 2018
kbreit pushed a commit to kbreit/ansible that referenced this pull request Jan 11, 2019
@acozine acozine moved this from To do to Done in OLD Ansible Documentation Mar 7, 2019
@ansible ansible locked and limited conversation to collaborators Jul 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.8 This issue/PR affects Ansible v2.8 core_review In order to be merged, this PR must follow the core review workflow. docs This issue/PR relates to or includes documentation. docsite This issue/PR relates to the documentation website. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants