-
Notifications
You must be signed in to change notification settings - Fork 817
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
Remove simple-tcp and simple-udp #1992
Remove simple-tcp and simple-udp #1992
Conversation
This covers: * Moving the windows build scripts from simple-udp to simple-game-server * Moving all the documentation to use simple-game-server (tested this by hand) * Updated all the examples to use simple-game-servers * Removal of both simple-udp and simple-tcp Closes googleforgames#1890
Build Failed 😱 Build Id: 6fcde9ac-bc35-429d-af85-3dc0a6053a50 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
e2e failure:
|
Build Succeeded 👏 Build Id: 940557d8-4d84-4667-81b5-ce64d83a509e 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.
There are a lot of website changes included in your change that aren't guarded to be part of the next release. Many of them seem ok to push now, but there are some that seem like they should wait until the release (e.g. removing references to the old images, since those exist in the current release).
@@ -21,8 +21,6 @@ These are full examples for each of the resource types of Agones | |||
|
|||
These are all examples of simple game server implementations, that integrate the Agones game server SDK. | |||
|
|||
- {{< ghlink href="examples/simple-udp" >}}Simple UDP{{< /ghlink >}} (Go) - simple server and client that send UDP packets back and forth. |
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.
Should this be deleted now or with the next release?
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.
So I didn't see any specific reason not to delete it now, given that it is a duplicated example that has almost identical code already stored in the simple-game-server
. But if you feel strongly we should keep it until next release, I'm happy to do so.
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 was the main place I wanted to gate the change. In the rest of the site it seems ok to flip it over, but removing these links separate from a release (where we remove support for the code) didn't feel quite right.
@@ -34,7 +34,7 @@ def scaleUpFleet(self): | |||
"apiVersion": "agones.dev/v1", | |||
"kind": "Fleet", | |||
"metadata": { | |||
"generateName": "fleet-simple-udp", | |||
"generateName": "simple-game-server", |
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.
simple-game-server-fleet?
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.
Sure. 🤷 I don't think anyone is using this anyway.
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.
Switched to "fleet-simple-game-server" since the previous version started with "fleet"
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 is in the locust dir, I agree that it isn't being used actively (the old udp-server version was 0.5 whereas everywhere else it was 0.21).
Not a big deal, but it doesn't look like you actually pushed a change with the updated fleet name.
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.
huh, I changed it here:
3152f31#diff-29dc460d65f66644f6c926f4f634a479e9167383eaeb8c8078f76090cd391fcbR174
Did I do the wrong file... 🤔 ?
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.
OIC THE SPOT! Pushing up a fix.
Build Succeeded 👏 Build Id: 458b55dc-4020-4df8-be60-3215972bdfad 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:
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: markmandel, 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 |
* Only remove example links on release (had to add `‌` because of strange markdown formatting issues) * Update fleet_autoscaling.py fleet name
3152f31
to
bae35f4
Compare
New changes are detected. LGTM label has been removed. |
Build Succeeded 👏 Build Id: d95dde7c-355e-4ca3-8ffb-71a63033e326 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?
/kind breaking
What this PR does / Why we need it:
This covers:
hand)
Which issue(s) this PR fixes:
Closes #1890
Special notes for your reviewer:
97% of this is replacing
udp-server
withsimple-game-server
Release Notes: Make a note that we're no longer developing simple-udp and simple-tcp, but the previous versions of the example images will continue to be hosted.