implement the mig_with_percent submodule #19
Conversation
There was a problem hiding this comment.
Thanks, @namusyaka!
Example modules should not be nested within each other; the Terraform registry only recognizes the modules directly under examples.
| description = "The GCP region where the managed instance group resides." | ||
| } | ||
|
|
||
| variable "instance_template_version0" { |
There was a problem hiding this comment.
Are there more descriptive names for the two template groups rather than 0 and 1? Regardless, the names should be delimited like version_0.
There was a problem hiding this comment.
Changed to initial_version and next_version. Any ideas?
| target_size = "${var.target_size}" | ||
| hostname = "mig-with-percent-simple" | ||
| instance_template_version0 = "${module.instance_template.self_link}" | ||
| instance_template_version1 = "${module.instance_template.another_self_link}" |
There was a problem hiding this comment.
This output is undefined. Similar to the inputs, I suggest we find more meaningful names to differentiate the two self links, like regular_self_link and preemptible_self_link.
There was a problem hiding this comment.
We should keep the regular self_link as self_link but give preemptible a descriptive name.
There was a problem hiding this comment.
I'm thinking this should have regular prefix or not. In my opinion, we can have the regular_ prefix for the consistency according to preemptible_and_regular_instance_templates.
modules/mig_with_percent/main.tf
Outdated
| } | ||
| } | ||
|
|
||
| data "google_compute_zones" "available" {} |
There was a problem hiding this comment.
This should be configured with region = "${var.region}".
There was a problem hiding this comment.
Ack, thanks for pointing this out.
This should also be updated, but I'll do that after merging this series.
|
@namusyaka as you pointed out, other examples in this repository have followed the same nesting pattern so you can keep it as is. The issue with this approach is that the Terraform Registry does not advertise any examples for this module 1 so we should address it in a different pull request. |
|
Since this depends on #18, let's focus on finishing that review before continuing here. |
|
Sure. |
|
Merged master into this branch and upgrade syntax to 0.12. PTAL. |
|
ping? |
@morgante This is a first try to implement the pvm with percent on mig module. I believe we can discuss our design based on these changes.