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

Client: Enable to supply labels as inventory-vars #582

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

superstes
Copy link
Contributor

ARA-Labels can currently only be set inside a playbook or as extra-vars.

As Ansible uses the inventory-system to dynamically set variables - it would make sense to allow users to set those labels inside their inventory.
This PR adds exactly that functionality.

@superstes
Copy link
Contributor Author

superstes commented Jan 22, 2025

It was unclear to me how integration tests should be added.
Might make sense to add test-cases for it.

For now I've (only) tested it manually:

  • Add the ara_playbook_labels variable inside host- and/or group-vars
  • Execute jobs
  • Check ARA Server - the labels were added
  • There seem to be an existing handling for duplicated labels

@superstes superstes changed the title client: enable to supply labels as inventory-vars Client: enable to supply labels as inventory-vars Jan 22, 2025
@superstes superstes changed the title Client: enable to supply labels as inventory-vars Client: Enable to supply labels as inventory-vars Jan 22, 2025
Copy link

Build failed.
https://ansible.softwarefactory-project.io/zuul/buildset/65ff36645c594d549f9288154b06b605

✔️ ara-tox-py3 SUCCESS in 2m 37s
✔️ ara-tox-linters SUCCESS in 2m 49s
ara-tox-docs FAILURE in 2m 31s
✔️ ara-basic-ansible-core-devel SUCCESS in 5m 57s (non-voting)
✔️ ara-basic-ansible-10 SUCCESS in 6m 28s
✔️ ara-basic-ansible-core-2.16 SUCCESS in 6m 15s
✔️ ara-basic-ansible-core-2.17 SUCCESS in 5m 59s
✔️ ara-container-images SUCCESS in 12m 24s

@superstes
Copy link
Contributor Author

The docs-test error/warning seems to be unrelated (?):
WARNING: Calling get_html_theme_path is deprecated. If you are calling it to define html_theme_path, you are safe to remove that code.

@superstes
Copy link
Contributor Author

Note: We could of course deduplicate the isinstance and split checks.

Copy link

Build failed.
https://ansible.softwarefactory-project.io/zuul/buildset/dd90cbb1db26406089ee9a87ddb13209

✔️ ara-tox-py3 SUCCESS in 2m 32s
✔️ ara-tox-linters SUCCESS in 2m 10s
ara-tox-docs FAILURE in 2m 01s
✔️ ara-basic-ansible-core-devel SUCCESS in 4m 47s (non-voting)
✔️ ara-basic-ansible-10 SUCCESS in 5m 01s
✔️ ara-basic-ansible-core-2.16 SUCCESS in 4m 58s
✔️ ara-basic-ansible-core-2.17 SUCCESS in 4m 52s
✔️ ara-container-images SUCCESS in 9m 44s

@dmsimard
Copy link
Contributor

Hi, I added a comment relevant to this PR in the issue: #581 (comment)

I'd like that we consider what is the right solution because iterating over every host is expensive.

@dmsimard
Copy link
Contributor

dmsimard commented Feb 8, 2025

I will leave this PR open for now but I would rather not go with the suggested implementation for now.

When I have more time I would like to benchmark this approach to demonstrate my point.

@superstes
Copy link
Contributor Author

If performance of the for-loop is the possible stopper:
Might we be able to use a config-option to opt-in to this functionality?

The labels system might be a lot more useful if inventory-vars could be used 🤔

@dmsimard
Copy link
Contributor

dmsimard commented Mar 4, 2025

If performance of the for-loop is the possible stopper: Might we be able to use a config-option to opt-in to this functionality?

The labels system might be a lot more useful if inventory-vars could be used 🤔

This came back on my radar recently and may be of interest to you: #596

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants