Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
More Local Dev Server Support #3252
More Local Dev Server Support #3252
Changes from 6 commits
2203efb
a04cfd2
9d2be3b
df14a37
559901f
d7020c1
1c83925
8ace56c
6c070c4
125bfde
60bc8d9
060e3c8
62cb705
597ecf0
7a6bfb5
b0bc52f
de032f7
3a6036a
6347a99
d94cfe5
608f415
af34674
fddea9d
6dba945
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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'd prefer to keep this section in. Not everyone knows what OS matches to what code names, etc. Better to be explicit.
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 section was still here. It's right above this. I just changed this to be a formatted sentence, rather than half-bullets. I went ahead and changed this mostly back (although, now its a single set of bullets).
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.
Let's move this section either above/below "Running Local Mode in a Container" as "Running Local Mode from Source" - then it's consistent and we don't force new developer to decide which one they want (analysis paralysis) before they have even learned how to use the tool.
We can add a section at the top saying "if you want alternate ways to run the local sdk server, see further down the documentation", or similar.
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.
Moved to last section (before
Next Steps
).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 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've moved "running from source" down to the last section (just above
Next Steps
). Reworded to be a mix of new and old. Feel free to suggest further edits.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.
Tech writer nit: Some things aren't "easily" or "simple" for some people, so better to remove the adjective.
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.
Cleaned up.
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 we have the section above 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.
Cleaned up any
go run
outside of the "run from source" section.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 same as our other docs:
agones/site/content/en/docs/Integration Patterns/allocation-from-fleet.md
Line 28 in 3d57eaa
(This would be at the bottom of the page -- see note on moving below to
local-game-server.md
).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.
Added/updated
Next Steps
onsite/content/en/docs/Guides/Client SDKs/local.md
andsite/content/en/docs/Guides/local-game-server.md
to link to the newsite/content/en/docs/Advanced/out-of-cluster-dev-server.md
page.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.
Just a heads up, our docs publish on merge, so if you have new features that aren't in the current release, please wrap them in a
feature
shortcode like so: https://agones.dev/site/docs/contribute/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.
Added
publishDate
to new page. Wrapped links with{{% feature publishVersion="1.34.0" %}}
.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 feel like to do this cleanly, we should update the sdkserver to also allow command line args for the GameServer name and Pod Namespace -- just to be consistent, and make life easier for end users. Then we don't have to mix args and env vars.
Also would prefer to see these docs using a binary, not running from source.
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 agree that this would be nicer. I can add in the extra flags.
As mentioned in the other comment, I think running from source is nicer than downloaded binaries. But sure, for the sake of documentation consistency, I can update this.
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.
Just to follow up here, this work has been done.
--gameserver-name
and--pod-namespace
can be used as cli args, andGAMESERVER_NAME
andPOD_NAMESPACE
still work as env vars (which are used in k8s). Also updated this to be consistent and use pre-built binary commands.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 when reading the "out of cluster mode" - I automatically assumed I could connect to my local game server binary.... but reading this, it doesn't seem to be the case.
I'm wondering if there's a way we can combine it into one whole process? 🤔 not entirely sure how, but it feels like it's worth exploring. 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 is the case. This is what I do for local testing. I have my custom game server running in Visual Studio, I run the SDK Server on a terminal, and I have it connect to my cluster. This allows me to allocate the game server via normal prod flows (e.g. external calls onto the allocator service) which are then fully integrated with my breakpoint-able game server in VS.
I mean, the steps to do so are mentioned in documentation. One would need to call
kubectl apply -f dev-gameserver.yaml
, then run./sdk-server.linux.amd64 --kubeconfig ...
, then run their custom game server binary. Everything is then connected.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.
A few thoughts on this:
agones.dev/dev-address
GameSever approach exists -- which is kinda why I'm thinking we may want this in https://agones.dev/site/docs/guides/local-game-server/ as well, since one really does lead into the other.agones.dev/dev-address
for you? It's using local kubectl permissions anyway -- save you akubectl apply
? Just trying to streamline development even more.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.
Ummm, this is https://agones.dev/site/docs/guides/local-game-server/ (
site/content/en/docs/Guides/local-game-server.md
). Maybe I'm misunderstanding you? 😅I added links to both pages, this is the one from https://agones.dev/site/docs/guides/local-game-server/ to https://agones.dev/site/docs/guides/client-sdks/local/. In
site/content/en/docs/Guides/Client SDKs/local.md
(line 180) I have the reverse link.That seems like a large expansion of the behavior of the SDK Server. While it obviously isn't my call to say what's best for the codebase, it just seems like that change would be larger and more out of place.
On a slightly separate note, one of the goals here was for this dev flow to be more representative of prod. While we are manually running the SDK Server locally instead of it being created in a sidecar container, it is still running the "out of cluster" mode using the exact same logic flow as it would be in the sidecar. Adding in extra steps/logic of the SDK Server, or changing how the SDK Server and the
GameServer
resource interact, builds less confidence in "what is developed" == "what goes on prod".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.
Sorry, I 100% wasn't clear. I was wondering it the whole new documentation set in
local.md
should be in this documentation - since my gut says that they are likely always going to be used together. WDYT? That also then solves any confusion around connection etc.Also I was providing feedback generally to the docs, not just these specific pages -- understandably confusing.
What do you think of the idea though? Can always be a secondary issue filed for future work, just thought it might be handy.
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 suppose it could go in either place. Personally, I feel like this has more to do with running the local SDK Server (as you need to change the commands used when running it). I definitely think "out of cluster" mode should be mentioned in both spots.
If you're just asking for my opinion, I don't particularly see any use for it (for me). I suppose some might like omitting a
kubectl apply
, but that's not really what I'd be optimizing for. What I'd be looking for is "breakpoint locally but in the most prod-y way possible". As the expected order of lifetimes/creation on prod would be:GameServer
resource, SDK Server binary, custom game server binary, I would want to keep it in that order. This means that the custom game binary, the allocation system, etc, can all be run in a consistent state (code abstracting away from this flow vs prod). Swapping around the order or operations, or changing what creates what, for dev testing (in such an integrated context) doesn't really make sense to me, as it isn't representative of how things will be working at the end.Speaking of "other things that could be done for handy-ness":
As I alluded to in #3252 (comment), I have other desires for potential future PRs. Namely that dev servers wouldn't automatically go from
Creating
toReady
. Instead, they would do a normalCreating
toScheduled
, and would only progress toReady
after the SDK call was made (+ controller state progression) -- all of this would match https://agones.dev/site/docs/reference/gameserver/#gameserver-state-diagram. Anannotation
could be used to "auto-ready" (e.g.agones.dev/auto-ready: true
) to keep the current functionality. Also anannotation
foragones.dev/recycle-gs: true
would be nice to progress fromShutdown
toCreating
/Scheduled
(so when the binary exits, calling SDK Shutdown, theGameServer
would just be reset instead of deleted). Again, all of this is a bit different/extra -- this PR is just trying to add in the finishing touches for a very useful flow that's mostly there already. Lol, I hesitated to really go into all this extra stuff as I don't want it to distract from this (seemingly more straightforward) PR. Hopefully this helps with the perspective that I'm looking at things from.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.
These are cool ideas! I'd love you to write up some issues and designs for these and we can discuss them further.
Thinking about docs organisation some more, https://agones.dev/site/docs/guides/client-sdks/local/ is about a purely local experience (and the simplest to get started with).
This file is about integrating with your online system, likely tied to how you like to run things in production (also it's further down in the docs, so it's assumed by this point, you've managed to work out how the SDK works, done some integration etc).
Jumping from "how do I integrate the SDK" to "now I'm working in a cluster" within the same page as it is now seems to me like a big leap concept with for new user onboarding to me.
So let's move the out-of-cluster documentation into a new section here, and add a link to this doc at the bottom (I'll add a suggestion to
local.md
on what I think will work there), since I think the annotations and the out-of-cluster mode is all interlinked (I don't see one being done without the other), without changing the learning path drastically.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 went ahead and filed #3284.
I've now moved all the "out of cluster" documentation into
site/content/en/docs/Advanced/out-of-cluster-dev-server.md
. This actually allowed me to go much further into various details. I've updatedsite/content/en/docs/Guides/Client SDKs/local.md
andsite/content/en/docs/Guides/local-game-server.md
withNext Steps
that link to out of cluster configuration.