-
Notifications
You must be signed in to change notification settings - Fork 262
Conversation
|
Can one of the admins verify this patch? |
1 similar comment
|
Can one of the admins verify this patch? |
|
ok to test |
|
for some weird reason, jenkins is not recognizing the new parameter. I've set up a custom job and run the rspec there |
cpanato
left a comment
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.
Ran in a side jenkins job and GCP tests passed.this LGTM
|
@squat / @s-urbaniak / @alexsomesan can you guys take a look here and review? |
squat
left a comment
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.
LGTM. two nits
modules/gcp/master-igm/master.tf
Outdated
| instance_template = "${google_compute_instance_template.master-it.self_link}" | ||
| target_pools = ["${var.master_targetpool_self_link}"] | ||
| base_instance_name = "mstr" | ||
| base_instance_name = "${var.cluster_name}-mstr" |
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.
nit: can we make this fully "master" rather than abbreviated to "mstr"? All other platforms and other resources in this platform use the full word.
modules/gcp/worker-igm/worker.tf
Outdated
| instance_template = "${google_compute_instance_template.worker-it.self_link}" | ||
| target_pools = ["${var.worker_targetpool_self_link}"] | ||
| base_instance_name = "wrkr" | ||
| base_instance_name = "${var.cluster_name}-wrkr" |
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.
here too
|
running the gcp tests in the custom job |
google_compute_target_tcp_proxygoogle_compute_region_instance_group_managerNote: This need to be rebased once #2178 gets in