Full support for terraform-0.7.11 by tamsky · Pull Request #171 · Capgemini/kubeform

tamsky · 2016-11-17T20:22:43Z

terraform apply (and destroy) works mostly clean now.

Later steps in the docs, like ansible-playbook doesn't apply cleanly, but I think that's beyond the scope of this PR.

After Capgemini/tf_tls#45 lands,
we can fix the module references
s@github.com/tamsky@github.com/Capgemini@g

builds from changes in #170

In Terraform 0.7.x, template file data sources do not support the 'provisioner' parameter. So instead, we must now use a null_resource to get the discovery URL. And, unfortunately, depending on a null_resource does not mean that you can read the file before it exists.

… of AZs

convert security_groups -> vpc_security_group_ids

enxebre · 2016-11-25T00:35:51Z

hey @tamsky @mrwacky42 thanks a lot for this, I just dropped a few comments.

enxebre · 2016-11-25T00:28:15Z

@@ -59,6 +59,7 @@ for i in $(ls .terraform/modules/*/Makefile); do i=$(dirname $i); make -C $i; do
 ### Provision the cluster infrastructure

 ```


Can we just remove the file from the repo and this will just work when running terraform apply? so no need to manual step?

I think you're correct and it can be removed.

I spoke too soon. It needs to stay.

Explanation is from
337e104

In Terraform 0.7.x, template file data sources do not support the
'provisioner' parameter. So instead, we must now use a null_resource to
get the discovery URL.

And, unfortunately, depending on a null_resource does not mean that you
can read the file before it exists.

Just tested this myself, So we just need to keep a (empty) file named "etcd_discovery_url.txt" in the repo so when running "terraform apply" terraform file() does not complain, then the null resource populates the file right before the template data source uses it.

enxebre · 2016-11-25T00:28:51Z

 variable "organization" { default = "kubeform" }
-variable "region" { default = "eu-west-1" }
-variable "availability_zones" { default = "eu-west-1a,eu-west-1b,eu-west-1c" }
+#variable "region" { default = "eu-west-1" }


Can we remove the comments?

enxebre · 2016-11-25T00:29:30Z

-variable "region" { default = "eu-west-1" }
-variable "availability_zones" { default = "eu-west-1a,eu-west-1b,eu-west-1c" }
+#variable "region" { default = "eu-west-1" }
+#variable "availability_zones" { default = "eu-west-1a,eu-west-1b,eu-west-1c" }


Why dont we keep "eu-west-1a,eu-west-1b,eu-west-1c" for the new definition? why only 2 elements?

I should have used a tfvars file instead of doing it this way.
I thought us-west-2 had only 2 regions, (like us-west-1), but I just found out it has 3.

enxebre · 2016-11-25T00:31:13Z

+        command = "curl -s https://discovery.etcd.io/new?size=${var.masters} > ${var.etcd_discovery_url_file}"
+    }
+
+    # This will regenerate the discovery URL if the cluster size changes


is this meant to work on the fly? shouldn't we need to do something more sophisticated if the number of expected etcd masters increase as per https://coreos.com/etcd/docs/latest/etcd-live-cluster-reconfiguration.html?

This would seem to be beyond the scope of this PR.

If there was a latent issue with the logic in the null_resource.etcd_discovery_url.command can you create an issue for it?

There are comments in my commit message here that explain 337e104

Restated: Terraform generates the dependency graph before it creates the etcd_discovery_url.txt file. I think this will still barf on a fresh checkout without the url file in place.

There is no latent issue. My comment here refers to the fact that you are retriggering the null resource when the size changes to generate a new token (emulating the current behaviour with "template_file") but not sure this is useful. As per the link above, that's not what you need when you modify the size of a running cluster. So I think the "triggers" here could be omitted as it does not really solve anything. It does not hurt either though

enxebre · 2016-11-25T00:33:15Z

@@ -0,0 +1,5 @@
+# these files should be symlinked and pulled out into a separate file from ../main.tf


Why is this?

tamsky · 2016-11-29T18:47:56Z

@enxebre do you have an opinion on whether I should include @methril 's Digital Ocean terraform 0.7.x changes that he's offering in tamsky#1 ?

enxebre · 2016-12-01T18:32:46Z

hey @tamsky let's create a different PR for DigitalOcean.
I just dropped a new comment on the etc discovery manual step but otherwise looks good to me. So Then If we can squash the git history to keep it cleaner I'm more than happy to merge. Thanks a lot for your contributions

tamsky · 2016-12-01T20:59:57Z

I guess I don't know the proper way to squash a PR referencing a remote repo.
I deleted my branch and pushed a new commit with the same branch name.

What's the correct way?

Sharif Nassar and others added 10 commits November 15, 2016 10:37

Fix nested quotes for Terraform 0.7.x

b264a1a

Use template_file as data resource for TF 0.7.x

21439ce

Remove etcd_discovery_url.txt

f9aa334

switch regions

fd852bb

include changes from Capgemini#86

11010dc

separate out vpc_public_cidrs_list and move it next to dependent list…

754750a

… of AZs

aws_instance: fix subnet_id

683f56c

convert security_groups -> vpc_security_group_ids

tag aws_vpc with org Name

0dfaaba

un-fix kubectl_server_ip in module.kube_admin_cert

9dea01b

enxebre reviewed Nov 25, 2016

View reviewed changes

enxebre mentioned this pull request Nov 25, 2016

support terraform-0.7.x Capgemini/tf_tls#45

Open

tamsky added 3 commits November 29, 2016 10:37

docs/aws/public: remove manual steps re: etcd_discovery_url

b9fac49

remove manual steps

4f77240

restore eu-west regions and AZs

8968b02

docs/aws/public: re-add manual provisioning step for etcd_discovery_url

a54a491

tamsky mentioned this pull request Nov 29, 2016

Added terraform-0.7.x support to DigitalOcean. tamsky/kubeform#1

Closed

tamsky added 2 commits December 1, 2016 12:47

remove null_resource etcd_discovery_url trigger

9a1fae4

add empty etcd_discovery_url.txt

99dd8ae

tamsky closed this Dec 1, 2016

tamsky deleted the terraform-0.7.x branch December 1, 2016 20:58

tamsky mentioned this pull request Dec 1, 2016

Update terraform/aws to support terraform version 0.7.x #172

Merged

		@@ -59,6 +59,7 @@ for i in $(ls .terraform/modules/*/Makefile); do i=$(dirname $i); make -C $i; do
		### Provision the cluster infrastructure

		```

		@@ -0,0 +1,5 @@
		# these files should be symlinked and pulled out into a separate file from ../main.tf

Uh oh!

Conversation

tamsky commented Nov 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

enxebre commented Nov 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tamsky commented Nov 29, 2016

Uh oh!

enxebre commented Dec 1, 2016

Uh oh!

tamsky commented Dec 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tamsky commented Nov 17, 2016 •

edited

Loading

tamsky commented Dec 1, 2016 •

edited

Loading