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

plugin loader: return collection name; ansible-doc: handle ansible.builtin correctly #70026

Merged
merged 3 commits into from Jul 10, 2020

Conversation

felixfontein
Copy link
Contributor

@felixfontein felixfontein commented Jun 11, 2020

SUMMARY

Follow-up to #70401. Determines the collection name properly also for ansible.builtin (avoid confusion with user-supplied plugins).

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

lib/ansible/cli/doc.py
lib/ansible/plugins/loader.py

@ansibot ansibot added affects_2.10 This issue/PR affects Ansible v2.10 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. needs_triage Needs a first human triage before being processed. 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. labels Jun 11, 2020
@ansibot
Copy link
Contributor

ansibot commented Jun 11, 2020

The test ansible-test sanity --test package-data [explain] failed with the error:

Command "/usr/bin/python3.6 /root/ansible/test/sanity/code-smell/package-data.py" returned exit status 1.
>>> Standard Error
Traceback (most recent call last):
  File "/root/ansible/test/sanity/code-smell/package-data.py", line 373, in <module>
    main()
  File "/root/ansible/test/sanity/code-smell/package-data.py", line 350, in main
    sdist_path = create_sdist(tmp_dir)
  File "/root/ansible/test/sanity/code-smell/package-data.py", line 164, in create_sdist
    raise Exception('make snapshot failed:\n%s' % stderr)
Exception: make snapshot failed:
fatal: not a git repository (or any parent up to mount point /)
Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).
fatal: not a git repository (or any parent up to mount point /)
Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).
fatal: not a git repository (or any parent up to mount point /)
Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).
fatal: not a git repository (or any parent up to mount point /)
Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).
fatal: not a git repository (or any parent up to mount point /)
Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).
fatal: not a git repository (or any parent up to mount point /)
Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).
ERROR! Unexpected Exception, this is probably a bug: not all arguments converted during string formatting
Traceback (most recent call last):
  File "/usr/local/lib/python3.6/dist-packages/antsibull_changelog/cli.py", line 213, in run
    return arguments.func(arguments)
  File "/usr/local/lib/python3.6/dist-packages/antsibull_changelog/cli.py", line 316, in command_release
    use_ansible_doc=use_ansible_doc)
  File "/usr/local/lib/python3.6/dist-packages/antsibull_changelog/plugins.py", line 296, in load_plugins
    paths, plugin_type, collection_name, use_ansible_doc=use_ansible_doc)
  File "/usr/local/lib/python3.6/dist-packages/antsibull_changelog/plugins.py", line 242, in load_plugin_metadata
    plugins_list = list_plugins_ansibledoc(paths, plugin_type, collection_name)
  File "/usr/local/lib/python3.6/dist-packages/antsibull_changelog/plugins.py", line 198, in list_plugins_ansibledoc
    output = subprocess.check_output(command)
  File "/usr/lib/python3.6/subprocess.py", line 356, in check_output
    **kwargs).stdout
  File "/usr/lib/python3.6/subprocess.py", line 438, in run
    output=stdout, stderr=stderr)
subprocess.CalledProcessError: Command '['ansible-doc', '--json', '-t', 'become', '--list']' returned non-zero exit status 250.
make: *** [changelog] Error 1

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

lib/ansible/plugins/loader.py:492:161: E501: line too long (169 > 160 characters)

click here for bot help

@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 Jun 11, 2020
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. core_review In order to be merged, this PR must follow the core review workflow. labels Jun 12, 2020
@felixfontein
Copy link
Contributor Author

/rebuild_failed

@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 Jun 15, 2020
@felixfontein
Copy link
Contributor Author

ready_for_review

@nitzmahone @bcoca the collection name generation now works, and it can correctly distinguish between no collection (for plugins/modules from library/ etc.) and ansible.builtin (for plugins/modules from Ansible itself). For this I had to do some changes in the plugin loader:

  1. the path list returned by PluginLoader._get_paths() contains for every path name a flag whether this is an Ansible-internal path, or whether this is another (user-supplied) path;
  2. the PluginLoader._plugin_path_cache cache now stores pairs (path, collection_name) instead of , and PluginLoadContext has a new field plugin_resolved_collection.

@samdoran samdoran added docs This issue/PR relates to or includes documentation. and removed needs_triage Needs a first human triage before being processed. labels Jun 16, 2020
@samdoran samdoran requested a review from nitzmahone June 16, 2020 15:04
@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 Jun 24, 2020
@felixfontein
Copy link
Contributor Author

At today's project meeting, we decided that this is the wrong approach since we do not want ansible.builtin. to be used for builtin plugins/modules. Will create a new simpler PR.

@felixfontein felixfontein changed the title ansible-doc: include collection name in man page [WIP] plugin loader: return collection name; ansible-doc: handle ansible.builtin correctly Jul 10, 2020
@ansibot ansibot added the WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. label Jul 10, 2020
@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Jul 10, 2020
@felixfontein felixfontein changed the title [WIP] plugin loader: return collection name; ansible-doc: handle ansible.builtin correctly plugin loader: return collection name; ansible-doc: handle ansible.builtin correctly Jul 10, 2020
@ansibot ansibot removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. labels Jul 10, 2020
@nitzmahone
Copy link
Member

rebuild_merge

@ansibot ansibot added shipit This PR is ready to be merged by Core 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. shipit This PR is ready to be merged by Core labels Jul 10, 2020
@felixfontein
Copy link
Contributor Author

/rebuild

@ansibot ansibot added shipit This PR is ready to be merged by Core and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jul 10, 2020
@ansibot ansibot merged commit 24dcaf8 into ansible:devel Jul 10, 2020
@felixfontein felixfontein deleted the ansible-doc-collection-name branch July 10, 2020 22:34
@felixfontein
Copy link
Contributor Author

@nitzmahone @samdoran thanks a lot for reviewing!

felixfontein added a commit to felixfontein/ansible that referenced this pull request Jul 10, 2020
…iltin correctly (ansible#70026)

* Determine collection in plugin loader.

* Fix test.

* Use PluginPathContext objects in PluginLoader._plugin_path_cache instead of tuples.

(cherry picked from commit 24dcaf8)
nitzmahone pushed a commit that referenced this pull request Jul 17, 2020
…oader: return collection name; ansible-doc: handle ansible.builtin correctly (#70572)

* ansible-doc: include collection name in text output (#70401)

* ansible-doc: include collection name in text output

* Be more careful to not accidentally pass ansible.builtin for user-supplied modules.

(cherry picked from commit f4c89ea)

* plugin loader: return collection name; ansible-doc: handle ansible.builtin correctly (#70026)

* Determine collection in plugin loader.

* Fix test.

* Use PluginPathContext objects in PluginLoader._plugin_path_cache instead of tuples.

(cherry picked from commit 24dcaf8)
@ansible ansible locked and limited conversation to collaborators Aug 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.10 This issue/PR affects Ansible v2.10 bug This issue/PR relates to a bug. docs This issue/PR relates to or includes documentation. has_issue shipit This PR is ready to be merged by Core 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
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants