Skip to content

ansible-doc fix YAML examples rendering#84467

Open
bcoca wants to merge 8 commits into
ansible:develfrom
bcoca:examples_doc_fix
Open

ansible-doc fix YAML examples rendering#84467
bcoca wants to merge 8 commits into
ansible:develfrom
bcoca:examples_doc_fix

Conversation

@bcoca
Copy link
Copy Markdown
Member

@bcoca bcoca commented Dec 12, 2024

@ansibot ansibot added bug This issue/PR relates to a bug. needs_triage Needs a first human triage before being processed. labels Dec 12, 2024
@sivel
Copy link
Copy Markdown
Member

sivel commented Dec 12, 2024

In case you want to look at this one too, and only applies to modules:

dummy, errors, traces = parse_yaml(examples_raw,
examples_lineno,
self.name, 'EXAMPLES',
load_all=True,
ansible_loader=True)

That always assumes that EXAMPLES will be a string.

@bcoca
Copy link
Copy Markdown
Member Author

bcoca commented Dec 12, 2024

@sivel, looking at that, it ONLY checks modules, it should really check most plugins ... might need it's own PR

@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Dec 12, 2024
@ansible ansible deleted a comment from ansibot Dec 12, 2024
@ansible ansible deleted a comment from ansibot Dec 12, 2024
@ansible ansible deleted a comment from ansibot Dec 13, 2024
@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Dec 16, 2024
@bcoca bcoca marked this pull request as ready for review December 16, 2024 20:08
@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Dec 16, 2024
@bcoca
Copy link
Copy Markdown
Member Author

bcoca commented Dec 17, 2024

cc @felixfontein

@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Dec 17, 2024
@bcoca bcoca requested review from mattclay and sivel December 17, 2024 15:51
Comment thread changelogs/fragments/doc_examples_fix.yml Outdated
@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 Dec 31, 2024
@ansibot 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 Jan 10, 2025
Comment thread changelogs/fragments/doc_examples_fix.yml Outdated
Comment thread changelogs/fragments/doc_examples_fix.yml Outdated
Comment thread lib/ansible/cli/doc.py
@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Jan 13, 2025
@ansibot ansibot added the stale_review Updates were made after the last review and the last review is more than 7 days old. label Jan 13, 2025
Comment thread changelogs/fragments/doc_examples_fix.yml Outdated
@ansibot ansibot added stale_review Updates were made after the last review and the last review is more than 7 days old. and removed stale_review Updates were made after the last review and the last review is more than 7 days old. labels Jan 13, 2025
Comment thread lib/ansible/cli/doc.py
@bcoca bcoca removed the needs_triage Needs a first human triage before being processed. label Jan 14, 2025
@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Jan 14, 2025
Comment thread changelogs/fragments/doc_examples_fix.yml Outdated
Comment thread changelogs/fragments/doc_examples_fix.yml Outdated
Comment thread lib/ansible/cli/doc.py
else:
try:
text.append(yaml_dump(doc.pop('examples'), indent=2, default_flow_style=False))
text.append(DocCLI._dump_yaml(doc.pop('examples')))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there a reason we're using the AnsibleLoader and AnsibleDumper instead of the normal loader and dumper?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

!vault and !unsafe in examples break otherwise

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Examples should only be accepted as text, not structured data. Attempting to load them as anything other than text loses comments, formatting and !unsafe, as well as complicates the loading and dumping. Using the normal YAML loader and dumper will allow non-text examples to pass through as long as they don't use ansible specific tags. The validate-modules sanity test should be updated to report an error (if it doesn't already) when the examples are not text, so that developers know to correct their examples.

In the future we could implement a feature to allow validating the text examples in whatever format(s) we'd like to support, such as YAML, but that's not something we should try to design now.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There doesn't appear to be a good reason to add support to ansible-doc for dumping Ansible-specific types. Switching to the standard YAML loader for sidecar will avoid the need for that.

Looking at collections on Galaxy, there's no usage of Ansible YAML tags in sidecar docs. Even if someone was using them, the docs are already broken -- either generating tracebacks or rendering incorrectly. So we shouldn't need to worry about introducing new issues by changing the loader.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is not a new feature but fixing the exiting feature to align with what would be the logical expectation, that ansible documentation can handle ansible specific yaml.

Also, I'm sure no one is using a feature that would currently break their docs, that does not mean they have not tried nor want to.

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. stale_review Updates were made after the last review and the last review is more than 7 days old. and removed stale_review Updates were made after the last review and the last review is more than 7 days old. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jan 15, 2025
Comment thread lib/ansible/cli/doc.py
else:
try:
text.append(yaml_dump(doc.pop('examples'), indent=2, default_flow_style=False))
text.append(DocCLI._dump_yaml(doc.pop('examples')))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Examples should only be accepted as text, not structured data. Attempting to load them as anything other than text loses comments, formatting and !unsafe, as well as complicates the loading and dumping. Using the normal YAML loader and dumper will allow non-text examples to pass through as long as they don't use ansible specific tags. The validate-modules sanity test should be updated to report an error (if it doesn't already) when the examples are not text, so that developers know to correct their examples.

In the future we could implement a feature to allow validating the text examples in whatever format(s) we'd like to support, such as YAML, but that's not something we should try to design now.

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. stale_review Updates were made after the last review and the last review is more than 7 days old. and removed stale_review Updates were made after the last review and the last review is more than 7 days old. labels Jan 15, 2025
Comment thread lib/ansible/cli/doc.py Outdated
@ansibot ansibot removed the stale_review Updates were made after the last review and the last review is more than 7 days old. label Jan 17, 2025
@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 Jan 26, 2025
bcoca and others added 8 commits February 3, 2025 14:21
test sidecar inventory plugin with yaml examples
added now required examples to test plugins
fix test to get array length dynamically
this seems legacy copy/paste and not intended for use
with role specs
Co-authored-by: Matt Clay <matt@mystile.com>
Co-authored-by: Matt Clay <matt@mystile.com>
@ansibot ansibot added stale_review Updates were made after the last review and the last review is more than 7 days old. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. 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 Feb 3, 2025
@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 Feb 17, 2025
@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Apr 15, 2025
@ansibot ansibot added the stale_pr This PR has not been pushed to for more than one year. label Feb 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug This issue/PR relates to a bug. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_pr This PR has not been pushed to for more than one year. stale_review Updates were made after the last review and the last review is more than 7 days old.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants