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

Actually check for pct #4

Merged
merged 1 commit into from
Jun 13, 2024
Merged

Actually check for pct #4

merged 1 commit into from
Jun 13, 2024

Conversation

IndrekHaav
Copy link

@IndrekHaav IndrekHaav commented Jun 11, 2024

I noticed that _set_version() claims to check for pct first, but actually checks for lxc (which fails on Proxmox, and therefore the plugin settles on LXC v1). Running a playbook with -vvv confirms that lxc-attach and similar commands are used, instead of the Proxmox pct command.

I'm guessing it's just a typo; in any case this PR should fix it.

@IndrekHaav IndrekHaav marked this pull request as ready for review June 11, 2024 21:31
@jbrubake jbrubake merged commit 71be106 into jbrubake:master Jun 13, 2024
1 of 14 checks passed
@jbrubake
Copy link
Owner

Thanks. Definitely a typo which then made me wonder why the plugin was actually working. It turns out it was finding LXC v1 (lxc-info) and just using that. So I'm not sure this plugin is even that useful as long as the upstream ansible-lxc-ssh keep supporting LXC v1.

Maybe if I take advantage of specific pct functionality it would be useful?

@IndrekHaav
Copy link
Author

For executing commands inside a container, I don't think there is much difference between pct exec and lxc-attach - the former is essentially just a wrapper for the latter.

There are some pct commands that might be useful for Ansible tasks, like lxc push/pull for moving files to/from the container, that don't have direct lxc-* equivalents, but I don't know if those can be implemented in a connection plugin.

@jbrubake
Copy link
Owner

jbrubake commented Jul 8, 2024

I just pushed a quick fix that allows a non-root user to use this plugin. That alone seems reason enough to keep this fork alive so I'll keep working on it as I'm able

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