Skip to content

constructed inventory, allow adding variables directly, including ansible_group_priority #81603

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

Draft
wants to merge 24 commits into
base: devel
Choose a base branch
from

Conversation

bcoca
Copy link
Member

@bcoca bcoca commented Aug 30, 2023

implementing #76788

ISSUE TYPE
  • 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. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Aug 30, 2023
@ansibot

This comment was marked as resolved.

@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Aug 31, 2023
@bcoca bcoca removed the needs_triage Needs a first human triage before being processed. label Aug 31, 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 Sep 14, 2023
@acziryak-everhealth
Copy link

I'm waiting on this to get merged and released to enable the inventory setup that I've architected. Anything I can do to get this pushed through?

@bcoca
Copy link
Member Author

bcoca commented Sep 21, 2023

Time, which I don't have much of.

I'm fine if you want to take over this PR and complete it, it at least needs tests. The other thing i was mulling over is making priority templatable or not and injecting keyed_group_name as a variable

@acziryak-everhealth
Copy link

I took a stab at fixing an error I ran into, and adding tests.

I have the changes here in a fork that I made: 0a6fa06

I'm not sure what the workflow would be to get this into the PR. I guess I could open a PR against the branch in your repo, which when merged would reflect in this PR? IDK, I just don't wanna mess anything up workflow-wise for you.

I mean, I haven't ever even contributed code to Ansible before, so I don't know what a typical workflow looks like, much less whatever's going on here.


Re: the templating of the priority variable: I don't think that would address either mine or the original issue, since we were both utilizing it to set a static priority number. Adding that wouldn't solve any issue that's currently been brought up.


I'm definitely lost at what you mean by "injecting keyed_group_name as a variable", or in what way that might be useful. Do you mean to make it a bool, as in "This group has been created by the keyed_groups functionality"? Or something completely different? Either way, I don't think any additional variable is needed to solve the issues that lead to this PR getting created.

Thanks for all the work that you do, and I hope that whatever I can do on this can save you some time and effort.

@bcoca
Copy link
Member Author

bcoca commented Sep 26, 2023

as for workflow, you can add a 'suggested commit' to this PR itself

the templating/keyed_group_name was so you could do the following:

keyed_groups:
  - key: tags
    prefix: tag
    default_value: "running"
    priority: "{{ 10 if 'status' in ansible_keyed_group_name else 50 }}"

assuming tags: stuff, otherstuff, thisstatus, that_status

@acziryak-everhealth
Copy link

I wasn't able to find suggested commits on this PR, so I opened bcoca#13 with my changes. If that's not going to work, hopefully we can figure something else out.

@bcoca bcoca marked this pull request as ready for review October 19, 2023 20:46
@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 Oct 19, 2023
@acidtracks
Copy link

Hello,
do you have an idea when this pr will be merged please?
I need this to merge some multi environments inventories into one.

Really helpfull thanks by advance

@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Oct 26, 2023
@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. labels Oct 26, 2023
@bcoca bcoca requested a review from nitzmahone October 26, 2023 19:30
@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Nov 2, 2023
@ansibot ansibot removed the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Nov 6, 2023
Copy link
Member

@nitzmahone nitzmahone left a comment

Choose a reason for hiding this comment

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

Just a couple typos, otherwise LGTM

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed stale_review Updates were made after the last review and the last review is more than 7 days old. labels Nov 13, 2023
@bcoca bcoca requested a review from nitzmahone November 13, 2023 17:41
@ansibot ansibot added the stale_review Updates were made after the last review and the last review is more than 7 days old. label Nov 13, 2023
bcoca and others added 22 commits June 6, 2025 15:11
* Fix setting group priority and add tests

Add all the tests to ensure the overrides for priority work as
advertised, and fix the set_priority function so that it operates on a
group rather than on a string in `__init__.py`

* Implement ansible_keyed_group_name

Add more comments
Add example
Change where we're setting the group_priority to allow it to be
templated with all available variables and `ansible_keyed_group_name`

* Reverse precedence of combining variables

* Move group priority to after the `add_host` call in case the group does not exist yet
Co-authored-by: Matt Davis <6775756+nitzmahone@users.noreply.github.com>
Co-authored-by: Matt Davis <6775756+nitzmahone@users.noreply.github.com>
Co-authored-by: Matt Davis <6775756+nitzmahone@users.noreply.github.com>
…oup_str_default_value_with_priority.yml

Co-authored-by: Matt Davis <6775756+nitzmahone@users.noreply.github.com>
@bcoca bcoca force-pushed the cgroup_priority branch from ff866f3 to dd10d80 Compare June 6, 2025 19:13
@ansibot ansibot removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html stale_pr This PR has not been pushed to for more than one year. ci_verified Changes made in this PR are causing tests to fail. 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 Jun 6, 2025
@ansibot
Copy link
Contributor

ansibot commented Jun 6, 2025

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

lib/ansible/plugins/inventory/constructed.py:0:0: option-incorrect-version-added: version_added for new option (group_vars) should be '2.19'. Currently StrictVersion ('2.17')

click here for bot help

@webknjaz webknjaz added the ci_verified Changes made in this PR are causing tests to fail. label Jun 9, 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 Jun 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci_verified Changes made in this PR are causing tests to fail. feature This issue/PR relates to a feature request. 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. 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.

7 participants