platform/gcp: namespacing resources by cluster_name #2167
Conversation
|
Can one of the admins verify this patch? |
1 similar comment
|
Can one of the admins verify this patch? |
squat
left a comment
squat
left a comment
There was a problem hiding this comment.
Looks good over all. I have some questions about consistent naming of resources.
modules/gcp/master-igm/master.tf
Outdated
|
|
||
| resource "google_compute_instance_template" "tectonic-master-it" { | ||
| name = "tectonic-master-it" | ||
| name = "${var.cluster_name}-tectonic-master-it" |
There was a problem hiding this comment.
consider removing -tectonic from the name. none of the other names use it.
modules/gcp/master-igm/master.tf
Outdated
| count = "${var.instance_count}" | ||
| target_size = 1 | ||
| name = "tectonic-master-igm-${count.index}" | ||
| name = "${var.cluster_name}-tectonic-master-igm-${count.index}" |
| resource "google_compute_firewall" "master-ingress-flannel" { | ||
| name = "master-ingress-flannel" | ||
| name = "${var.cluster_name}-master-ingress-flannel" | ||
| network = "${google_compute_network.tectonic-network.name}" |
There was a problem hiding this comment.
we should ensure we only create this resource on clusters that actually use Flannel networking.
There was a problem hiding this comment.
We'll probably move to use routes and kubenet, will handle this in a follow up networking specific PR
modules/gcp/network/loadbalancer.tf
Outdated
| @@ -1,31 +1,43 @@ | |||
| resource "google_compute_target_pool" "tectonic-master-targetpool" { | |||
| name = "tectonic-master-targetpool" | |||
| name = "${var.cluster_name}-tectonic-master-targetpool" | |||
There was a problem hiding this comment.
remove -tectonic from the name for consistency with the other resources and other providers, e.g. https://github.com/coreos/tectonic-installer/blob/master/modules/aws/master-asg/master.tf
modules/gcp/network/loadbalancer.tf
Outdated
|
|
||
| resource "google_compute_target_pool" "tectonic-worker-targetpool" { | ||
| name = "tectonic-worker-targetpool" | ||
| name = "${var.cluster_name}-tectonic-worker-targetpool" |
modules/gcp/network/network.tf
Outdated
| ## A single GCP network | ||
| resource "google_compute_network" "tectonic-network" { | ||
| name = "tectonic-network" | ||
| name = "${var.cluster_name}-tectonic-network" |
There was a problem hiding this comment.
same as above for this whole file
modules/gcp/worker-igm/worker.tf
Outdated
|
|
||
| resource "google_compute_instance_template" "tectonic-worker-it" { | ||
| name = "tectonic-worker-it" | ||
| name = "${var.cluster_name}-tectonic-worker-it" |
platforms/gcp/tectonic.tf
Outdated
| limitations under the License. | ||
| */ | ||
| locals { | ||
| etcd_count = "${var.tectonic_experimental ? 0 : max(var.tectonic_etcd_count, 1)}" |
There was a problem hiding this comment.
this is only used once; is it worth creating this local expression? maybe
enxebre
force-pushed
the
namespacing
branch
2 times, most recently
from
4aaafe8 to
cedd937
Compare
|
Thanks for having a look @squat, removed 'tectonic' from naming for consistency and pointless 'local' |
|
ok to test |
3 participants
No description provided.