-
Notifications
You must be signed in to change notification settings - Fork 5
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
Auto-apply variables #21
base: master
Are you sure you want to change the base?
Conversation
This reverts commit ca1da60.
1db6989
to
b02c39c
Compare
} | ||
locals { | ||
ansible_env={ | ||
ANSIBLE_REMOTE_PORT = var.host.port |
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.
do we still have a remote ansible? or is this always used locally?
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.
Yes this is for local ansible to know what port to connect to
} | ||
locals { | ||
scripts_enabled = var.is_windows ? false : var.scripts_enabled | ||
} |
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.
add an extra new line at the end of the files, the same for the rest
ANSIBLE_REMOTE_USER = local.ssh_user | ||
ANSIBLE_HOST_KEY_CHECKING = false | ||
} | ||
ansible_command="timeout 4m ansible-playbook -c ssh -i ${data.openstack_networking_floatingip_v2.public.address}," |
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 use the public IP instead of the local _vm private one?
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.
Because otherwise users using terraform on their local laptop wont be able to connect to the vm
content = <<-EOF | ||
#!/bin/bash | ||
if [[ -r '/etc/debian_version' ]];then | ||
apt-get update && apt-get install -y nfs-common cron |
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 won't install nfs if the user/project does not want to use manila service
|
||
EOT | ||
} | ||
provisioner "remote-exec" { |
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.
not local-exec?
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 do have a Proof Of Concept for an ansible playbook. Only downside is that it doesn't really support the "time format string" like * * * * *
and instead needs dedicated variables for minute/hour/day/weekday/...
provisioner "remote-exec" { | ||
inline = [ | ||
"set -e", | ||
"sudo cp /home/${local.ssh_user}/${random_id.obscure[each.key].id}-${each.key}.sh /opt/vsc/cron/${each.key}.sh", |
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.
are these files only accessible by root user?
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.
The files are placed inside the user home directory first with default permissions, and are then immediately copied and chown-ed to root:
"sudo chown root:root /etc/cron.d/${each.key}",
There is a brief window where these files are not owned by root, but considering this user has access to sudo anyway that doesn't really make a difference. Nevertheless with an ansible solution we could place the files in the right directory immediately
@@ -3,14 +3,16 @@ locals { | |||
any_enabled = var.nfs_enabled || var.nginx_enabled | |||
ports = { | |||
ssh = jsondecode(shell_script.port_ssh.output["ports"])[0] | |||
http = var.nginx_enabled ? jsondecode(shell_script.port_http[0].output["ports"])[0] : null | |||
http = var.nginx_enabled ? ( var.alt_http ? jsondecode(shell_script.port_http[0].output["ports"])[0] : 80 ) : null | |||
https = var.nginx_enabled ? ( var.alt_http ? jsondecode(shell_script.port_http[0].output["ports"])[1] : 443) : null |
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.
we should add this in the current docs if we do not have it yet, ports 80 and 443 are open from ugent net
lifecycle { | ||
precondition { | ||
condition = var.public | ||
error_message = ("Cant enable forward on a private instance!") |
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.
Can't
Nice! I have a few questions/comments @GoodOldJack12 . I guess this is backwards incompatible so we should prepare a email for the users before merging this. |
This PR adds a lot of modularity to the module template where it was previously hardcoded or on-boot only:
It also adds more customization options:
Finally it also fixes some issues when using the module as a proper external module.
Some of the features rely on ssh in order to run playbooks/install cron scripts.
Backwards-compatible in most cases but requires user-communication.