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

Fix. Ansible provisioner shows pending changes each time. Make arguments and envs variables not required #14

Merged
merged 6 commits into from
Oct 13, 2017

Conversation

SweetOps
Copy link
Contributor

@SweetOps SweetOps commented Oct 10, 2017

What

Why

  • Module shouldn't rerun playbook if nothing has changed
  • Keep everything DRY

@SweetOps SweetOps self-assigned this Oct 10, 2017
@SweetOps
Copy link
Contributor Author

screenshot 2017-10-10 13 20 54

screenshot 2017-10-10 13 22 28

@SweetOps SweetOps changed the title Fix issue with recreation and set variables arguments, envs to no required Fix issue with recreation and set variables arguments, envs to non-required Oct 10, 2017
main.tf Outdated
@@ -17,11 +15,11 @@ resource "null_resource" "provisioner" {

triggers {
signature = "${data.archive_file.default.output_md5}"
command = "ansible-playbook ${var.dry_run ? "--check --diff" : ""} ${join(" ", var.arguments)} -e ${join(" -e ", var.envs)} ${var.playbook}"
command = "ansible-playbook ${var.dry_run ? "--check --diff" : ""} ${length(compact(concat(var.arguments))) > 0 ? join(" ", var.arguments) : ""} ${length(compact(concat(var.envs))) > 0 ? "-e" : ""} ${length(compact(concat(var.envs))) > 0 ? join(" -e ", var.envs) : ""} ${var.playbook}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This version is shorter and might be better:

command   = "ansible-playbook ${var.dry_run ? "--check --diff" : ""} ${join(" ", compact(var.arguments))} ${length(compact(var.envs)) > 0 ? "-e" : ""} ${join(" -e ", concat(var.envs))} ${var.playbook}"

}

variable "playbook" {}
variable "playbook" {
default = ""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume, empty default value will cause an error.
playbook path is required.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@comeanother, look at count = "${signum(length(var.playbook)) == 1 ? 1 : 0}"

variables.tf Outdated
}

variable "envs" {
type = "list"
default = [""]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use empty list [] as default value. Without empty strings.

variables.tf Outdated
@@ -1,12 +1,16 @@
variable "arguments" {
type = "list"
default = [""]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use empty list [] as default value. Without empty strings.

@SweetOps
Copy link
Contributor Author

screenshot 2017-10-10 19 31 13

main.tf Outdated
@@ -17,11 +15,11 @@ resource "null_resource" "provisioner" {

triggers {
signature = "${data.archive_file.default.output_md5}"
command = "ansible-playbook ${var.dry_run ? "--check --diff" : ""} ${join(" ", var.arguments)} -e ${join(" -e ", var.envs)} ${var.playbook}"
command = "ansible-playbook ${var.dry_run ? "--check --diff" : ""} ${join(" ", compact(var.arguments))} ${length(compact(var.envs)) > 0 ? "-e" : ""} ${length(compact(var.envs)) > 0 ? join(" -e ", var.envs) : ""} ${var.playbook}"
Copy link
Member

@osterman osterman Oct 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't there be spaces on either side of the -e like we do in the join?

Ah, I see. there are spaces outside of the interpolation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

${length(compact(var.envs)) > 0 ? join(" -e ", var.envs) : ""}
should be
${join(" -e ", compact(var.envs))}

osterman
osterman previously approved these changes Oct 10, 2017
main.tf Outdated
@@ -17,11 +15,11 @@ resource "null_resource" "provisioner" {

triggers {
signature = "${data.archive_file.default.output_md5}"
command = "ansible-playbook ${var.dry_run ? "--check --diff" : ""} ${join(" ", var.arguments)} -e ${join(" -e ", var.envs)} ${var.playbook}"
command = "ansible-playbook ${var.dry_run ? "--check --diff" : ""} ${join(" ", compact(var.arguments))} ${length(compact(var.envs)) > 0 ? "-e" : ""} ${length(compact(var.envs)) > 0 ? join(" -e ", var.envs) : ""} ${var.playbook}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

${length(compact(var.envs)) > 0 ? join(" -e ", var.envs) : ""}
should be
${join(" -e ", compact(var.envs))}

main.tf Outdated
}

provisioner "local-exec" {
command = "ansible-playbook ${var.dry_run ? "--check --diff" : ""} ${join(" ", var.arguments)} -e ${join(" -e ", var.envs)} ${var.playbook}"
command = "ansible-playbook ${var.dry_run ? "--check --diff" : ""} ${join(" ", compact(var.arguments))} ${length(compact(var.envs)) > 0 ? "-e" : ""} ${length(compact(var.envs)) > 0 ? join(" -e ", var.envs) : ""} ${var.playbook}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

${length(compact(var.envs)) > 0 ? join(" -e ", var.envs) : ""}
should be
${join(" -e ", compact(var.envs))}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@comeanother , in this case will get the error with empty var.envs

ansible-playbook: error: -e option requires an argument

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be if var.envs has empty elements like ""?
I assume, ansible-playbook command will fail, because it will be like:

ansible-playbook -e -e var=value

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

look at check conditionlength(compact(var.envs))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's consider the following example:

variable envs {
  default = ["", "TEST=test"]
}

It will pass condition check length(compact(var.envs)). but fail later

ansible-playbook -e -e TEST=test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't prevent all misstakes. If we add compact() for var.envs - using some vars like "dev=name title" will be impossible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@osterman, what do you think about that?

Copy link
Contributor Author

@SweetOps SweetOps Oct 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example if use some vars like "name=${some_words}" it'll break module too :) . What check should be added ? :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

${join(" -e ", compact(var.envs))} works properly.

Example:

variable "envs" {
  default = ["", "test"]
}

variable "dry_run" {
  default = "false"
}

variable "arguments" {
  default = []
}

variable "playbook" {
  default = ""
}

output "b" {
  value = "ansible-playbook ${var.dry_run ? "--check --diff" : ""} ${join(" ", compact(var.arguments))} ${length(compact(var.envs)) > 0 ? "-e" : ""} ${length(compact(var.envs)) > 0 ? join(" -e ", var.envs) : ""} ${var.playbook}"
}

output "c" {
  value = "ansible-playbook ${var.dry_run ? "--check --diff" : ""} ${join(" ", compact(var.arguments))} ${length(compact(var.envs)) > 0 ? "-e" : ""} ${join(" -e ", compact(var.envs))} ${var.playbook}"
}

screenshot from 2017-10-13 15-33-32

@SweetOps
Copy link
Contributor Author

screenshot 2017-10-13 15 41 00

screenshot 2017-10-13 15 41 43

Copy link
Contributor

@const-bon const-bon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Secondary terraform apply still shows changes.
screenshot from 2017-10-13 16-12-16

@const-bon
Copy link
Contributor

Nevermind, I've used wrong branch.

@const-bon const-bon changed the title Fix issue with recreation and set variables arguments, envs to non-required Fix. Ansible provisioner shows pending changes each time. Make arguments and envs variables not required Oct 13, 2017
@const-bon const-bon merged commit 04fb77c into master Oct 13, 2017
@const-bon const-bon deleted the recreate_fix branch October 13, 2017 13:48
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.

3 participants