-
Notifications
You must be signed in to change notification settings - Fork 819
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
Upgrading Terraform to 0.12.3 #899
Upgrading Terraform to 0.12.3 #899
Conversation
Build Succeeded 👏 Build Id: 9534da91-542c-4b3d-bf34-c7de9e0588be The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
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.
This is great! I'm really looking forward to the new and better syntax in terraform 0.12.
build/cluster.tf
Outdated
location = "${var.cluster["zone"]}" | ||
project = "${var.cluster["project"]}" | ||
provider = "google-beta" | ||
enable_legacy_abac = "${var.cluster["legacyAbac"]}" |
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.
Please remove this. legacy ABAC has been disabled for a long time on GKE and I'm removing this option from the various ways to create GKE clusters.
build/cluster.tf
Outdated
location = "${var.cluster["zone"]}" | ||
project = "${var.cluster["project"]}" | ||
provider = "google-beta" | ||
enable_legacy_abac = "${var.cluster["legacyAbac"]}" | ||
|
||
# Setting an empty username and password explicitly disables basic auth |
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.
Please remove the master_auth block. Now that Mark switched GKE to 1.12 the defaults in GKE disable both basic auth and client certs.
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.
Thanks for the information, will update soon
build/cluster.tf
Outdated
node_count = 1 | ||
|
||
node_config { | ||
preemptible = true |
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.
Why are we putting the agones controller onto a preemptible vm?
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.
Nice catch, I think this is a left-over from other Terraform configuration example.
build/includes/terraform.mk
Outdated
@@ -39,7 +39,7 @@ endif | |||
-var "cluster={name=\"$(GCP_CLUSTER_NAME)\", machineType=\"$(GCP_CLUSTER_NODEPOOL_MACHINETYPE)\", \ | |||
zone=\"$(GCP_CLUSTER_ZONE)\", project=\"$(GCP_PROJECT)\", \ | |||
initialNodeCount=\"$(GCP_CLUSTER_NODEPOOL_INITIALNODECOUNT)\", \ | |||
legacyABAC=\"$(GCP_CLUSTER_LEGACYABAC)\"}"' | |||
legacyAbac=\"$(GCP_CLUSTER_LEGACYABAC)\"}"' |
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.
Please remove legacyAbac
build/includes/terraform.mk
Outdated
@@ -65,7 +65,7 @@ endif | |||
-var "cluster={name=\"$(GCP_CLUSTER_NAME)\", machineType=\"$(GCP_CLUSTER_NODEPOOL_MACHINETYPE)\", \ | |||
zone=\"$(GCP_CLUSTER_ZONE)\", project=\"$(GCP_PROJECT)\", \ | |||
initialNodeCount=\"$(GCP_CLUSTER_NODEPOOL_INITIALNODECOUNT)\", \ | |||
legacyABAC=\"$(GCP_CLUSTER_LEGACYABAC)\"}"' | |||
legacyAbac=\"$(GCP_CLUSTER_LEGACYABAC)\"}"' |
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.
Please remove legacyAbac
build/modules/gke/cluster.tf
Outdated
@@ -14,7 +14,7 @@ | |||
|
|||
provider "google-beta" { | |||
version = "~> 2.4" | |||
zone = "${lookup(var.cluster, "zone")}" | |||
zone = "${lookup(var.cluster, "zone")}" |
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.
In the other file you changed places that had previously done a lookup(...)
to a different syntax (${var.cluster["zone"]}
) but this file still uses the lookup(...)
syntax. Can you make this file consistent with build/cluster.tf
?
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.
Yes, for sure
build/modules/gke/cluster.tf
Outdated
provisioner "local-exec" { | ||
command = "${"${format("echo Current variables set as following - name: %s, project: %s, machineType: %s, initialNodeCount: %s, zone: %s, legacyAbac: %s", | ||
provisioner "local-exec" { | ||
command = "${"${format("echo Current variables set as following - name: %s, project: %s, machineType: %s, initialNodeCount: %s, zone: %s, legacyAbac: %s", |
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.
Please remove legacyAbac here too.
build/modules/gke/cluster.tf
Outdated
project = "${lookup(var.cluster, "project")}" | ||
provider = "google-beta" | ||
|
||
# Setting an empty username and password explicitly disables basic auth |
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.
Please remove the whole master_auth block now that we are using 1.12.
86e3185
to
ae087cc
Compare
Build Failed 😱 Build Id: aa404344-3a97-4089-a65c-f8c66460a2d3 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: ff1324a3-af54-49ef-85ec-d471e3052bd6 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
08589ea
to
499e7f5
Compare
Build Failed 😱 Build Id: e023ac31-f81b-4288-b626-c95c06a22199 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: 503eff12-a50d-4df8-bfc7-c427fe432fdb To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: b4af3a6b-9a41-4e57-a797-f083bb3bd2d3 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
7353077
to
e863123
Compare
Build Failed 😱 Build Id: e9e0cc28-e5f5-4f49-8b9e-d4f1a2547b05 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: 829c39d9-1a24-44ed-aee3-1c894f1d18ac To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
So does that mean that this PR is currently not stable? |
Yes, this version is unstable, I am fixing these issues now. |
e863123
to
b82ce74
Compare
Build Succeeded 👏 Build Id: c126bd03-2d17-4313-868f-7b69e532488f The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
build/cluster.tf
Outdated
] | ||
|
||
labels = { | ||
"stable.agones.dev/agones-system" = "true" |
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.
The label (and taint) should just be agones.dev/agones-system
now.
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.
thanks for an update
build/cluster.tf
Outdated
] | ||
|
||
labels = { | ||
"stable.agones.dev/agones-metrics" = "true" |
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.
The label and taint should be agones.dev/agones-metrics
.
build/modules/gke/cluster.tf
Outdated
] | ||
|
||
labels = { | ||
"stable.agones.dev/agones-system" = "true" |
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.
as above, please fix the label / taint names.
build/modules/gke/variables.tf
Outdated
@@ -15,24 +15,31 @@ | |||
# Password for the Kubernetes API. | |||
# Could be defined using GKE_PASSWORD env variable | |||
# or by setting `password="somepass"` string in build/terraform.tfvars | |||
variable "password" {default = ""} | |||
variable "username" {default = "admin"} | |||
variable "password" { |
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.
It should be possible to remove the password and username variables.
c1e3c6c
to
00d17e2
Compare
Build Succeeded 👏 Build Id: 24c8e6ab-ce73-4d64-8822-e6f448fad597 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Build Succeeded 👏 Build Id: 3c705af4-42d5-41c9-81c0-439f3375fec0 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Build Failed 😱 Build Id: 7cac8ab0-45e4-4782-baf4-b79eef0dbd41 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Error in E2E:
|
00d17e2
to
71075ed
Compare
Build Failed 😱 Build Id: e4e2acf3-a52d-491e-9523-15bab2b2c42b To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
@roberthbailey applied all your comments. Now scripts are working.
|
Fixed configuration tf files according to a new format. terraform fmt all files. Update version of aks to 1.12.8.
71075ed
to
b704a8f
Compare
Build Succeeded 👏 Build Id: 241301b5-11c2-421d-99a3-3b299139947e The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Fixed configuration tf files according to a new format.
terraform fmt
all files.Also updated providers versions.
As part of #657