-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Update lxd connection to use all documented vars for options #3798
Update lxd connection to use all documented vars for options #3798
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution! Can you please add a changelog fragment? Thanks!
@@ -126,7 +126,7 @@ def put_file(self, in_path, out_path): | |||
local_cmd.extend([ | |||
"file", "push", | |||
in_path, | |||
"%s:%s/%s" % (self.get_option("remote"), self._host, out_path) | |||
"%s:%s/%s" % (self.get_option("remote"), self.get_option("remote_addr"), out_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you only replace it here, in line 92 and in line 148?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self._host seems to work fine else where in the plugin (otherwise its usage is just to be appended to verbose/error output), and seeing the inventory alias (e.g. vm1:container1) would be more descriptive in the case a user chooses to use something more elaborate for the inventory alias.
changelogs/fragments/3798-fix-lxd-connection-option-vars-support.yml
Outdated
Show resolved
Hide resolved
changelogs/fragments/3798-fix-lxd-connection-option-vars-support.yml
Outdated
Show resolved
Hide resolved
If nobody complains, I'll merge in a couple of days. |
Backport to stable-3: 💚 backport PR created✅ Backport PR branch: Backported as #3880 🤖 @patchback |
@cavcrosby thanks for your contribution! |
* Update lxd connection to use documented vars * Add a changelog fragment * Add clarification to changelog description * Shorten changelog fragment description (cherry picked from commit 8f6866d)
Backport to stable-4: 💚 backport PR created✅ Backport PR branch: Backported as #3881 🤖 @patchback |
* Update lxd connection to use documented vars * Add a changelog fragment * Add clarification to changelog description * Shorten changelog fragment description (cherry picked from commit 8f6866d)
SUMMARY
The changes below are in attempt to "fix" the mapping of the inventory vars to the LXD connection plugin documented options.
ISSUE TYPE
COMPONENT NAME
plugins/connection/lxd.py
ADDITIONAL INFORMATION
STEPS TO REPRODUCE
The following example assumes there exist a VM called 'vm1' that runs LXD, also this LXD instance manages the container 'container1'.
inventory
vm1:container1 ansible_lxd_remote=x.x.x.x ansible_lxd_host=container1 [lxd_containers] vm1:container1 [all:vars] ansible_user=ansible ansible_ssh_common_args='-o StrictHostKeyChecking=no'
playbook
output
After applying the following changes, running the same playbook
The main one I wanted to see change was interpolation of <instance> (
lxc exec [<remote>:]<instance> ...
) being fromself.get_option("remote_addr")
instead ofself._host
. If an inventory alias is used for a node, then theansible_lxd_host
inventory variable is ignored entirely. That said,ansible_host
worked ok here vsansible_lxd_host
.The others are so the other inventory vars can be mapped according to the other documented options. It could also be that it is desired to still make use of the
play_context
, and that the other specific lxd variables (e.g.ansible_lxd_host
,ansible_lxd_executable
) are not required.