-
Notifications
You must be signed in to change notification settings - Fork 49
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 vars to add .ssh/config and /etc/hosts only optionally #74
Conversation
Fix csmart#73 add options to remove tasks as well Not adding the key to known_hosts would make the cloud-init check fail as it's asking for the key to be verified. Could disable strict host checking in ssh, but for now I'll leave it as it is.
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.
@Enucatl thanks for the contribution! I've made a couple of comments for your consideration.
tasks/hosts-remove.yml
Outdated
@@ -14,6 +14,7 @@ | |||
become: true | |||
delegate_to: "{{ kvmhost }}" | |||
when: | |||
- virt_infra_add_etc_hosts |
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.
@Enucatl similar to ~/.ssh/config
, I guess technically it doesn't hurt to have this task always run, otherwise if the entries happened to be in /etc/hosts
already and then the user sets virt_infra_add_ssh_config: false
they will never get cleaned up on host remove... Or we can just assume users need to go and clean up these files if they remove a VM and have set virt_infra_add_ssh_config: false
. It is nice to run less tasks than necessary... What do you think?
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 my case I started this because I have ~/.ssh/config mounted in a way the doesn't allow root
to edit it (NFS mount with kerberos), so having it run removing a section that doesn't exist (hence doing nothing) would error out.
I thought exactly about the case you mentioned and I considered:
- keeping it as in my change would force the user to keep the flag true before the running with undefined to remove that VM. I convinced myself that it's not too bad. After that they can move onto false with no further issues (...for that VM)
- We could have separate flags for add and remove tasks, so that there is no ambiguity at all. I felt that this was kind of overcomplicating a simple thing. But maybe it works better, especially in large and complicated environments.
I'll close it since I want to make more changes to virt-customize to allow arbitrary flags. |
Fix #73
add options to remove tasks as well
Not adding the key to known_hosts would make the cloud-init check fail as it's asking for the key to be verified.
Could disable strict host checking in ssh, but for now I'll leave it as it is.