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

ansible-doc: show 'notes', 'seealso', 'requirements', and top-level 'version_added' for role entrypoints #81796

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

Conversation

felixfontein
Copy link
Contributor

SUMMARY

Fixes #81735.

ISSUE TYPE
  • Docs Pull Request
  • Feature Pull Request

@ansibot ansibot added feature This issue/PR relates to a feature request. needs_triage Needs a first human triage before being processed. has_issue needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Sep 27, 2023
@jborean93 jborean93 removed the needs_triage Needs a first human triage before being processed. label Sep 28, 2023
@sivel
Copy link
Member

sivel commented Sep 28, 2023

cc @cidrblock @ssbarnea just an FYI about potential schema changes to role arg specs. This hasn't been merged, but wanted to let you know ahead of time.

@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Oct 11, 2023
@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 Oct 25, 2023
@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Jan 16, 2024
@ansibot ansibot removed the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Jan 19, 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 19, 2024
@felixfontein
Copy link
Contributor Author

This PR currently adds back role attributes (i.e. it fixes #82639).

@ansibot ansibot added 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. labels Feb 1, 2024
@bcoca
Copy link
Member

bcoca commented Feb 1, 2024

i see the value of notes, seealso and version_added, but requirements and attributes per role entry point is not something i think we should incentivize.

@felixfontein
Copy link
Contributor Author

May I ask why? I think both requirements and attributes make perfect sense. I gave an example of attributes in #82639 (comment). And roles also have requirements, similar to plugins and modules, so having these listed in a standard Requirements section instead of sometimes in description, sometimes in notes, and sometimes maybe somewhere else, is IMO a good idea.

@bcoca
Copy link
Member

bcoca commented Feb 2, 2024

requirements and attributes seem to be copies of those already existing for modules/plugins, roles themselves should not create unique ones, specially at the spec level.

I would much prefer being able to see an aggregate of the used plugins requirements/attributes than having yet another facility to list them that is actually detached from the code that is tied to them, leading to double bookeeping and all the issues that entails.

@felixfontein
Copy link
Contributor Author

requirements and attributes seem to be copies of those already existing for modules/plugins

That's very much not the case. For example, a role administering some service or device using the uri module has more requirements than uri's requirements. And whether a role is idempotent is totally unrelated to whether the modules/plugins used by that role are idempotent.

@bcoca
Copy link
Member

bcoca commented Feb 2, 2024

I'm not sure what idempotence has to do with this. As for requirements I would suggest that they are set at the role level, not at the role spec level.

@felixfontein
Copy link
Contributor Author

I'm not sure what idempotence has to do with this.

argument_specs:
  main:
    short_description: Set up GitLab
    ...
    attributes:
      idempotence:
        description: Whether the role is idempotent.
        support: partial
        details: If O(upgrade_to_latest=true) the role is not idempotent.

As for requirements I would suggest that they are set at the role level, not at the role spec level.

Different entrypoints can have different requirements.

@bcoca
Copy link
Member

bcoca commented Feb 2, 2024

requirements: they can, that should not specify this at the entrypoint level, you would want those at the role level for installation

idempotence was not really something i would express as an attribute. Also i would not want to overload the 'attribute' facility to be confused with the module/plugin specific ones, which will have programmatic ties in the future.

@briantist
Copy link
Contributor

briantist commented Feb 2, 2024

I will add that I tried to use requirements recently and was surprised that it didn't work, so I said, "ok, I'll just put it in notes:" and then when that didn't work I gave up and added it to the description:.

I definitely would use these fields in roles, internally I have a bunch of collections and they contain mostly or exclusively roles; the documentation support for these is very important.

I also completely disagree that these fields should not change per entrypoint. I would like to be able to specify them at the role level for roles where they are the same on every entrypoint, but I do have some roles where the requirements and especially notes would differ between them, so that option is also important (otherwise I will be back to overloading description).

I also agree with Felix about attributes: they are absolutely important, and again they should be per-entrypoint. The most useful attribute I can think of off the top of my head is around check mode support, but I see no reason to offer restrictions on them for roles specifically.

idempotence was not really something i would express as an attribute

I'm not sure why, it feels like a natural fit to me.. I would use an idempotence attribute since that's very important information, and it's "enforced" (somewhat) in my own roles via molecule's idempotence test in my CI, but it sounds like this whole concept might be a separate conversation and I don't know if we should derail the discussion of attributes being usable in roles with discussion of this specific attribute idea.

@bcoca
Copy link
Member

bcoca commented Feb 2, 2024

Im' not saying to not allow requirements, I'm saying they make more sense in meta/main.yml than in the role spec, as this is something to inspect on installation. I reiterate the issue with 'double bookeeping' from modules/plugins and the distance between spec and task lists which also contributes to divergence. All of it made worse per spec if there is any overlap between them. I had been toying with a 'role inspector' that would try/attempt to get that information from modules/plugins in use in the role itself.

I understand why you want requirements/attributes per spec, but it goes against the intended design to keep roles small and to task. When we add infrastructure that is only useful for hugely complex and mixed used roles, we are operating against our own intent, we already regret adding tasks/vars/etc_from .

As for attributes for roles , this is not something that has been thought of/discussed/explored, since it was mainly designed for modules/plugins to both inform the user of specific behaviors in relation to the core engine and in the future allow core to 'inspect' these and react accordingly. I really don't see us doing the latter for roles and think descriptions would be enough for this kind of thing. Even if I'm convinced this is a good feature going forward, I would not want to expand attributes use here in this PR, it should be with much more thought and input from the rest of the core team and a project onto itself.

@felixfontein
Copy link
Contributor Author

Even if I'm convinced this is a good feature going forward, I would not want to expand attributes use here in this PR, it should be with much more thought and input from the rest of the core team and a project onto itself.

This PR is not expanding attributes. It is only fixing a bug introduced in another PR.

@bcoca
Copy link
Member

bcoca commented Feb 5, 2024

The bug (mine) was adding attributes to roles in the first place, that was never meant to be the case.

@felixfontein
Copy link
Contributor Author

BTW, ansible-lint already supports seealso and top-level version_added. ansible/ansible-lint#4017 adds notes and requirements.

@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 10, 2024
@felixfontein
Copy link
Contributor Author

I moved the attributes code to #82678. I will rebase with it removed from this PR.

@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 Feb 12, 2024
@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 26, 2024
…' for role entrypoints.

Re-add 'attributes' for role entrypoints.
@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 Mar 22, 2024
@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 Apr 5, 2024
@felixfontein
Copy link
Contributor Author

@bcoca what's the current state on this? Which parts of the PR are ok, which aren't?

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. has_issue stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

role argument spec: allow for notes, seealso, requirements, version_added like with plugins
6 participants