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

allow plugin loader to load sidecar docs #83170

Open
wants to merge 1 commit into
base: devel
Choose a base branch
from

Conversation

guppy0130
Copy link

  • if you have an inventory plugin (or other non-test/non-filter plugin, I'd assume) that has docs only provided via sidecar, then the plugin's config defs need to be populated by the sidecar; otherwise, you're forced to have a DOCUMENTATION attribute in the inventory plugin that duplicates the content of the sidecar
SUMMARY

Started using antsibull-docs and noticed that sidecar YAML files could be used for documentation instead of variables in ansible modules. Converted some configurable inventory plugins to use this feature, only to find that the inventory plugin would no longer understand how to read its config file. Tracked it down to the PluginLoader using the DOCUMENTATION attribute of the module (*.py) only. Figured that it wouldn't make sense for the documentation to be duplicated, and wanted to use the sidecar YAML instead of the DOCUMENTATION variable in the plugin, so this PR allows both in-plugin and sidecar DOCUMENTATION snippets to be used for the plugin loader's config def loading.

Saw there was a TODO related to this, so did it.

You'll note that the added lines does some path joining and existence testing; I realize that lib/ansible/utils/plugin_docs.py has a find_plugin_docfile, but I couldn't figure out how to get PluginLoader._load_config_defs to give me a name that find_plugin_docfile would understand (my context in find_plugin_docfile was either falsy or not resolved, because I got a lot of <inventory_plugin_name> was not found). FWIW, how I've structured my collections, etc. may be slightly nonstandard; regardless, since the code looks at the same directory as the plugin file (*.py), I figured they were equivalent, and at least this gets the ball rolling on this feature. If you have any suggestions on how to use find_plugin_docfile, please let me know.

ISSUE TYPE
  • Feature Pull Request
ADDITIONAL INFORMATION

Given some inventory plugin

# plugins/inventory/my_inventory.py
from internal_library import ClassThatGeneratesInventory  # no longer have to `noqa: E402` these

class MyInventoryPlugin(BaseInventoryPlugin):
    NAME = "community.core.my_inventory"

    def parse(self, inventory, loader, path, cache=True):
        self._read_config_data(path)
        self._parse_and_validate_options()
        self.get_option("option1")
        # rest of parse() is here

    ...  # other inventory plugin functions not important to the example

and sidecar

# plugins/inventory/my_inventory.yml
DOCUMENTATION:
    module: "community.core.my_inventory"
    short_description: ""
    description: ""
    author: ""
    options:
        option1:
            type: "string"

and inventory plugin config file

# inventory.my_inventory_plugin.config.yml
plugin: collection.core.my_inventory
option1: some value

The output before this change was not having any inventory, because the custom inventory plugin stopped understanding how to parse the config, and this inventory plugin needs to parse the config to fetch inventory

$ ansible-playbook  # forgot rest of command, but uses `community.core.my_inventory` and points at `inventory.my_inventory_plugin.config.yml`
[WARNING]:  * Failed to parse inventory.my_inventory_plugin.config.yml with auto plugin: 'Requested entry (plugin_type: inventory plugin: ansible_collections.community.core.plugins.inventory.my_inventory setting: option1 ) was not defined in configuration.'
[WARNING]: Unable to parse inventory.my_inventory_plugin.config.yml as an inventory source
[WARNING]: No inventory was parsed, only implicit localhost is available
[WARNING]: provided hosts list is empty, only localhost is available. Note that the implicit localhost does not match 'all'

after this change, the plugin loader can use the sidecar's DOCUMENTATION to determine what the options are, and therefore read the plugin config (and therefore populate inventory) again

$ ansible-playbook  # same command as before
... playbook run

@ansibot ansibot added feature This issue/PR relates to a feature request. needs_triage Needs a first human triage before being processed. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels May 1, 2024
@bcoca
Copy link
Member

bcoca commented May 1, 2024

The sidecar feature is not meant for migrating docs from plugins that should have the documentation embeded, but for those that cannot easily do so like tests and filters as they can have multiple plugins per file and also for modules that are not written in python we can avoid the 'python doc file'.

@webknjaz
Copy link
Member

webknjaz commented May 2, 2024

The RHEL failures are unrelated and at to be fixed separately.

@bcoca bcoca removed the needs_triage Needs a first human triage before being processed. label May 2, 2024
@guppy0130
Copy link
Author

The sidecar feature is not meant for migrating docs from plugins that should have the documentation embedded, but for those that cannot easily do so like tests and filters as they can have multiple plugins per file and also for modules that are not written in python we can avoid the 'python doc file'.

I see. I agree that my initial use case/impetus for this PR may not be using the sidecar documentation feature as intended, but I think the PR itself still has merit, unless there's no intention to provide this for other plugin types (if any?)

@bcoca
Copy link
Member

bcoca commented May 2, 2024

i've asked other core team members to weigh in. Personally I prefer to have the docs and code in the same file as it makes it trivial to keep in sync. sidecar was added for those cases that it was not possible to do so and they also happen to be cases in which the docs do not act as configuration (tests, filters and modules).

@felixfontein
Copy link
Contributor

I remember discussions when the sidecar feature was new, there were also other plugin authors/maintainers who would have liked to use sidecar docs for plugins. One big advantage of having the docs in a separate YAML file is that syntax highlighting and editor support for YAML files "just works". (I'm not sure whether the VSCode Ansible plugin handles the YAML-in-Python 'correctly', but even if it does, it won't work in other editors.)

@briantist I think you were also interested in this (but maybe I also misremember, in that case please ignore this ping :) ).

@briantist
Copy link
Contributor

Yes, I am also interested in this, there was at least one other issue for it somewhere.

  • As mentioned, it would be a much nicer editing experience, projects could also enforce their own YAML standards and conventions beyond sanity test rules
  • it's strange to be forced to embed YAML in a Python string for some plugin types, and be forced to make it separate for other plugin types
  • for PowerShell modules, we still have to have a (separate) python file for the sole purpose of embedding the string, but can't use a separate YAML file (I think this is still correct? if not I missed the change)
  • with documentation separate, it's possible to do some CI optimization for documentation-only PRs, for example maybe only building docs and running sanity tests (I know sometimes this is complicated due to configuration in plugins, this is just an example of a line of thinking, not a complete solution)

Personally I prefer to have the docs and code in the same file as it makes it trivial to keep in sync

And I think that's fine, but it is a matter of opinion for whomever is maintaining it. Letting those docs be in a sidecar gives maintainers a choice, and those want to continue to embed them can.

@webknjaz
Copy link
Member

I restarting the CI doesn't help. Please, rebase the PR.

@webknjaz webknjaz added the ci_verified Changes made in this PR are causing tests to fail. label May 10, 2024
@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label May 10, 2024
* if you have an inventory plugin (or other non-test/non-filter plugin,
  I'd assume) that has docs only provided via sidecar, then the plugin's
  config defs need to be populated by the sidecar; otherwise, you're
  forced to have a `DOCUMENTATION` attribute in the inventory plugin
  that duplicates the content of the sidecar
@guppy0130 guppy0130 force-pushed the plugin_loader_populated_by_sidecar branch from 35fd5de to 27c4276 Compare May 11, 2024 16:19
@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label May 11, 2024
@guppy0130
Copy link
Author

I restarting the CI doesn't help. Please, rebase the PR.

rebased with 27c4276

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature This issue/PR relates to a feature request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants