-
Notifications
You must be signed in to change notification settings - Fork 11
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
feat: Set ansible_connection to winrm for Windows hosts #75
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! Nice effort. Just a couple of silly suggestions.
plugins/inventory/kubevirt.py
Outdated
@staticmethod | ||
def is_windows(guest_os_info: Dict, annotations: Dict) -> bool: | ||
if (id := guest_os_info.get("id")) is not None: | ||
return id == "mswindows" |
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.
wdyt about making the id mswindows
a const?
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.
Created a constant for it.
plugins/inventory/kubevirt.py
Outdated
ANNOTATION_KUBEVIRT_IO_CLUSTER_PREFERENCE_NAME | ||
) | ||
) is not None: | ||
return cluster_preference.startswith("windows") |
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.
create the var windows = "windows"
to avoid repeating the same string?
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.
I'd like to stick to the way it is. IMO it fits the rest of the code style in the file.
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.
All right!
8f16319
to
92811df
Compare
Looks great IMHO. Thanks a lot |
/hold Waiting for more feedback from Ansible team |
7720613
to
31f1f8a
Compare
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.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lyarwood The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
Added small comment related to connection details for Windows machines
# Set up the connection | ||
service = None | ||
if self.is_windows(vmi_guest_os_info, vmi_annotations): | ||
self.inventory.set_variable(vmi_name, "ansible_connection", "winrm") |
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.
Should we want to hardcode winrm
as the connection for all Windows instances? Windows does support SSH and it might be ideal to provide a way to override. Maybe based on annotation? What about other connection options that we might want to inject for the specific Virtual Machine?
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.
In the next released version with the Constructable
feature fixed (see #77), you can do it with the compose
key in the inventory plugin's configuration.
Example inventory configuration:
---
plugin: kubevirt.core.kubevirt
results in
ansible-inventory -i inventory.kubevirt.yml --list | jq -r '.["_meta"]["hostvars"]["default-testvm"]["ansible_connection"]'
ssh
Adding compose
:
---
plugin: kubevirt.core.kubevirt
compose:
ansible_connection: "'something'"
results in
ansible-inventory -i inventory.kubevirt.yml --list | jq -r '.["_meta"]["hostvars"]["default-testvm"]["ansible_connection"]'
something
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.
compose
takes a Jinja template expression, so you can get quite creative. It should be able to override the value only for specific hosts too.
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.
That sounds great! Thanks for the update on the set of planned features
/lgtm |
Signed-off-by: Felix Matouschek <[email protected]>
This changes the inventory plugin so that it sets the ansible_connection to winrm if it detected a Windows host. If it did not detect a Windows host the ansible_connection is no longer set, so Ansible falls back to its default value of ssh. The detection of SSH services for hosts using winrm is disabled. Signed-off-by: Felix Matouschek <[email protected]>
/lgtm |
/hold cancel |
What this PR does / why we need it:
This changes the inventory plugin so that it sets the ansible_connection to winrm if it detected a Windows host. If it did not detect a Windows host the ansible_connection is no longer set, so Ansible falls back to its default value of ssh.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #33
Special notes for your reviewer:
Release note: