Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add WebSocket capability to WatchGameServer REST API #1999

Merged
merged 1 commit into from
Feb 23, 2021
Merged

Add WebSocket capability to WatchGameServer REST API #1999

merged 1 commit into from
Feb 23, 2021

Conversation

highlyunavailable
Copy link
Contributor

What type of PR is this?

/kind feature

What this PR does / Why we need it:

This PR adds a websocket proxy for the streaming endpoint /watch/gameserver. This allows clients that do not support reading from HTTP streams before they are complete, but do support WebSocket streaming (e.g. Unreal) to use the /watch/gameserver endpoint to stream changes.

Which issue(s) this PR fixes:

No issue, but I do want to use this to contribute to the Unreal plugin to add game server watch capabilities.

Special notes for your reviewer:

The PR is split up into multiple commits. I'm unsure as to what the correct practice is for go vendoring, but it wouldn't build without me running go mod vendor, so that is included as its own commit.

I'm also not sure how I'd test this - it work in manual tests of course but it doesn't appear there are any REST API tests in the first place so I'm not quite sure where to add them if it's needed.

@google-cla google-cla bot added the cla: yes label Feb 18, 2021
@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: dc3a399c-eec3-4c9c-8900-370d631bb6d2

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

markmandel
markmandel reviewed Feb 19, 2021
View reviewed changes
Copy link
Member

@markmandel markmandel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks cool! 😄 🥳

So in regards to test, there are integration tests - they are bit janky, because reasons.

The entry point for the REST tests are here:

So you can run them locally with make run-sdk-conformance-test-rest

🤔 since the REST sdk test already covers the watch event might want to have a specific test folder just for this websocket watch api, rather than try and integrate it with the REST test, as each integration test suite just does a "did this run at least once" check.

It could likely just be a bash command, if we can connect via curl?

Let me know if that list of comments made sense, and if I can point you in the right direction from there 👍

(Shame Unreal won't let you put in place http request interception middleware. Unity will 🤭 )

site/content/en/docs/Guides/Client SDKs/rest.md Outdated Show resolved Hide resolved
@markmandel
Copy link
Member

Looks like you also ran into some linting issues.

If you want to run our linter locally: make lint will do the job. (All the make commands are run from the build folder)

See: https://github.com/googleforgames/agones/blob/main/build/README.md

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 167791c2-5731-4d78-912c-3f81966362cd

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: fa8ca696-b244-4b09-ae36-3b5c88eeaced

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:

  • git fetch https://github.com/googleforgames/agones.git pull/1999/head:pr_1999 && git checkout pr_1999
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.13.0-502d770

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: f028ae7d-1a72-4d93-a9fb-cdf0e57c800f

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:

  • git fetch https://github.com/googleforgames/agones.git pull/1999/head:pr_1999 && git checkout pr_1999
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.13.0-480dc23

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 68f31e82-e199-4daf-8b59-f536cc349aa8

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 07610ae9-6b8f-4006-b6d1-903a254d1f30

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:

  • git fetch https://github.com/googleforgames/agones.git pull/1999/head:pr_1999 && git checkout pr_1999
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.13.0-ca5e7d1

@highlyunavailable
Copy link
Contributor Author

Thanks for your comments and guidance - I believe I've addressed all the issues found so far. Let me know if you see anything else!

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: ba46569f-79a7-4144-806e-d18e27bff33c

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:

  • git fetch https://github.com/googleforgames/agones.git pull/1999/head:pr_1999 && git checkout pr_1999
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.13.0-d8747ab

@markmandel
Copy link
Member

Haven't forgotten about this - will review tomorrow 👍

@highlyunavailable
Copy link
Contributor Author

No problem! I'm not in a hurry - I can use this code and I've got the Unreal plugin addition written but I have a hard dependency on #1944 being merged before I PR that because I'm on 4.26.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 95ff1b3f-823f-47c3-ba6e-c2b852087acd

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:

  • git fetch https://github.com/googleforgames/agones.git pull/1999/head:pr_1999 && git checkout pr_1999
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.13.0-579cf3d

markmandel
markmandel reviewed Feb 23, 2021
View reviewed changes
Copy link
Member

@markmandel markmandel left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor nit on the new file, but otherwise, this is good to go 👍

test/sdk/websocket-watch/ws-watch-test.go Outdated Show resolved Hide resolved
@highlyunavailable
Add websocket proxy to allow Watching without HTTP partial reads
606eb17 
This allows clients such as Unreal that support websockets but not HTTP
partial reads, to use streams such as the GameServer watch endpoint.
@highlyunavailable
Copy link
Contributor Author

I have also squashed this into a single commit, so it should be good to go now.

markmandel
markmandel approved these changes Feb 23, 2021
View reviewed changes
Copy link
Member

@markmandel markmandel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome.

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: highlyunavailable, markmandel

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@markmandel markmandel added this to the 1.13.0 milestone Feb 23, 2021
@markmandel markmandel added area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc kind/feature New features for Agones labels Feb 23, 2021
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 93fc1854-4deb-4c01-bdef-19f1588d249d

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:

  • git fetch https://github.com/googleforgames/agones.git pull/1999/head:pr_1999 && git checkout pr_1999
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.13.0-606eb17

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 0b4804c3-92ec-4f02-8bcf-585ee52b2bc3

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:

  • git fetch https://github.com/googleforgames/agones.git pull/1999/head:pr_1999 && git checkout pr_1999
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.13.0-8f04bfb

@markmandel markmandel merged commit d38f9cd into googleforgames:main Feb 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markmandel markmandel markmandel approved these changes

@EricFortin EricFortin Awaiting requested review from EricFortin

@roberthbailey roberthbailey Awaiting requested review from roberthbailey

Assignees

@markmandel markmandel

Labels
approved area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc cla: yes kind/feature New features for Agones lgtm size/XXL
Projects
None yet
Milestone
1.13.0
Development

Successfully merging this pull request may close these issues.
4 participants
@highlyunavailable @agones-bot @markmandel @google-oss-robot