-
Notifications
You must be signed in to change notification settings - Fork 813
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
by default disable agones-metrics nodepools #3672
by default disable agones-metrics nodepools #3672
Conversation
Build Succeeded 👏 Build Id: 2b0026cf-6af8-4593-a2e0-b61d400d5d46 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:
|
Thanks for sending this through! To compete this enhancement, can we also add documentation here for the new parameters please? You can see details on writing docs here: https://agones.dev/site/docs/contribute/ - the new parameters will need to go behind a Also, we'll want a variable here: https://github.com/googleforgames/agones/blob/main/build/includes/google-cloud.mk with it enabled to be on, so that the default test cluster we use for development still has the metric nodes. |
5f8dd01
to
4f78e8d
Compare
@markmandel, |
Build Succeeded 👏 Build Id: 88c60721-8282-4eac-9cde-fa92bfef201f 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.
Small revert, but otherwise, LGTM 👍🏻
// To enable agones-metrics add below command in `terraform apply`: | ||
// -var enable_agones_metrics_nodepool=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.
// To enable agones-metrics add below command in `terraform apply`: | |
// -var enable_agones_metrics_nodepool=true |
I think this can be rolled back - we don't have any other variable specific docs in a comment here.
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.
I think this can be rolled back - we don't have any other variable specific docs in a comment here.
Thanks for the review and suggestions.
Done!
@@ -32,6 +32,7 @@ gcloud-test-cluster: GCP_CLUSTER_NODEPOOL_MIN_NODECOUNT ?= 1 | |||
gcloud-test-cluster: GCP_CLUSTER_NODEPOOL_MAX_NODECOUNT ?= 5 | |||
gcloud-test-cluster: GCP_CLUSTER_NODEPOOL_WINDOWSINITIALNODECOUNT ?= 0 | |||
gcloud-test-cluster: GCP_CLUSTER_NODEPOOL_WINDOWSMACHINETYPE ?= e2-standard-4 | |||
gcloud-test-cluster: GCP_CLUSTER_NODEPOOL_ENABLE_AGONES_METRICS_NODEPOOL ?= 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.
👍🏻
4f78e8d
to
fd2f2b6
Compare
Build Succeeded 👏 Build Id: a70ae89a-936e-4c74-965f-de10883b1585 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: f48f5dea-8c2e-477d-8b66-3df3d1f919a9 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:
|
What type of PR is this?
What this PR does / Why we need it: Since we're not installing Prometheus or Graphana on these clusters. By default,
agones-metrics
nodepools would be disabled as nodes aren't doing anything. However, we can enable it when needed by passing-var enableAgonesMetricsNodepool=true
on theterraform apply
command.Which issue(s) this PR fixes: #3669
Closes #3669
Special notes for your reviewer: