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

Add inventory plugin iocage #9262

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

vbotka
Copy link
Contributor

@vbotka vbotka commented Dec 16, 2024

SUMMARY

@ansibullbot ansibullbot added feature This issue/PR relates to a feature request inventory inventory plugin new_plugin New plugin plugins plugin (any type) tests tests unit tests/unit labels Dec 16, 2024
@ansibullbot

This comment was marked as resolved.

@ansibullbot ansibullbot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Dec 16, 2024
@ansibullbot ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Dec 16, 2024
@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-10 Automatically create a backport for the stable-10 branch labels Dec 16, 2024
@vbotka vbotka mentioned this pull request Dec 17, 2024
1 task
Copy link
Collaborator

@russoz russoz left a comment

Choose a reason for hiding this comment

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

Hi @vbotka thanks for your contribution

Couple of issues/suggestions.

plugins/inventory/iocage.py Show resolved Hide resolved
plugins/inventory/iocage.py Outdated Show resolved Hide resolved
plugins/inventory/iocage.py Outdated Show resolved Hide resolved
plugins/inventory/iocage.py Outdated Show resolved Hide resolved
plugins/inventory/iocage.py Show resolved Hide resolved
plugins/inventory/iocage.py Outdated Show resolved Hide resolved
plugins/inventory/iocage.py Outdated Show resolved Hide resolved
plugins/inventory/iocage.py Outdated Show resolved Hide resolved
plugins/inventory/iocage.py Outdated Show resolved Hide resolved
plugins/inventory/iocage.py Show resolved Hide resolved
plugins/inventory/iocage.py Outdated Show resolved Hide resolved
plugins/inventory/iocage.py Outdated Show resolved Hide resolved
plugins/inventory/iocage.py Outdated Show resolved Hide resolved
plugins/inventory/iocage.py Outdated Show resolved Hide resolved
plugins/inventory/iocage.py Outdated Show resolved Hide resolved
def test_verify_file(tmp_path, inventory):
file = tmp_path / "foobar.iocage.yml"
file.touch()
assert inventory.verify_file(str(file)) is True
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be nice to have more extensive tests, than this absolutely bare minimum :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm working on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I completed the tests.

vbotka and others added 4 commits December 17, 2024 23:26
* Create get_jails and get_properties in iocage plugin to simplify testing.
* Update test_iocage.py
* Add fixtures iocage_*
@vbotka vbotka requested a review from felixfontein December 19, 2024 07:04
Comment on lines 20 to 22
- By default, O(host) is V(localhost). If O(host) is not V(localhost) it
is expected that the O(user) is able to connect to the O(host) with
SSH and execute the command C(iocage list).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Technically, it is not the O(user) (remote user on the target iocage host) that must be able to connect. This would be more accurate as:
"... expected that the user running Ansible on the controller is able to connect with SSH to the O(host) with O(user) without specifying password or key, and O(user) is able to execute the command C(iocage list)."

Or something along those lines - feel free to wordsmith it to your satisfaction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. I updated the description item

        - By default, O(host) is V(localhost). If O(host) is not V(localhost) it
          is expected that the user running Ansible on the controller can
          connect to the O(host) account O(user) with SSH non-interactively and
          execute the command C(iocage list).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks!

Copy link
Collaborator

@russoz russoz left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-10 Automatically create a backport for the stable-10 branch check-before-release PR will be looked at again shortly before release and merged if possible. feature This issue/PR relates to a feature request has_issue inventory inventory plugin new_plugin New plugin plugins plugin (any type) tests tests unit tests/unit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants