-
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
Update the machine type for GKE clusters in build scripts and terraform modules. #2063
Update the machine type for GKE clusters in build scripts and terraform modules. #2063
Conversation
terraform modules.
Build Succeeded 👏 Build Id: 493c84b0-77d9-4bb5-8603-f2d293f29dea 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:
|
@@ -80,6 +80,7 @@ The GKE cluster created from the example configuration will contain 3 Node Pools | |||
|
|||
Configurable parameters: | |||
|
|||
{{% feature expiryVersion="0.14.0" %}} | |||
- project - your Google Cloud Project ID (required) | |||
- name - the name of the GKE cluster (default is "agones-terraform-example") | |||
- agones_version - the version of agones to install (an empty string, which is the default, is the latest version from the [Helm repository](https://agones.dev/chart/stable)) |
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.
You could just put this line for n1-standard-4 in the feature expiry and replace it with the new note.
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 thought that doing it for a single line in a list breaks rendering for the whole list (but I could be wrong). Since the release candidate is tomorrow, I'm leaning towards merging this since it'll be removed soon in any case. WDYT?
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.
That's true for the table, but I am not sure about a list. You can locally verify using make site-server
.
I am fine with either. You already have my approval.
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 did a local test and you are correct - you can just wrap the one list item with the feature expiry.
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.
Since this will be cleaned up in a week and I'd like to get this in before the release candidate gets cut, I'm going to merge as-is.
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.
Looking more closely, while it works, it changes the spacing of the list elements, which I find off-putting. So I think that doing it around the entire list is preferable.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pooneh-m, roberthbailey The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind breaking
What this PR does / Why we need it: Removes all remaining references to n1-standard machine type from the repository.
I've marked this change as breaking since it changes the machine type in the terraform files.
Which issue(s) this PR fixes:
Closes #2055
Special notes for your reviewer: This PR also addresses a comment from #2056 that was made after the PR was merged.