-
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
Use environment variables to dynamically set the grpc port in the golang sdk. #1086
Use environment variables to dynamically set the grpc port in the golang sdk. #1086
Conversation
Build Succeeded 👏 Build Id: 7edbe592-fb1d-48ef-ab27-7542d2f7f964 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.
👍 I'm in 100% agreement on approach here, it's the most robust and flexible. Just think we should remove the fiter on only the gameserver and apply to all.
We should also include some docs on these environment variables - some people have written their own REST API clients, so will want to be able to know what the ports are going forward.
9fea6df
to
2842cc0
Compare
I was waiting for a +1 on the approach before finishing the PR. I've added unit tests and documentation. |
Build Succeeded 👏 Build Id: 73fe4792-e376-43ce-83c7-dfe618b80e7b 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:
|
2842cc0
to
2dd4893
Compare
Build Succeeded 👏 Build Id: dc087191-257a-46c8-9a2d-07d90df864f6 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.
Couple of small things, but otherwise, this looks great 👍
b93b806
to
009ec47
Compare
I found a few more places on the website to update with the new environment variable. I also noticed that https://agones.dev/site/docs/guides/client-sdks/unreal/#settings is rendering the list content incorrectly. It looks correct in my markdown preview window, so I'm not sure what docsy/hugo is doing to it. |
Build Succeeded 👏 Build Id: 3e14458e-ff94-4d8d-944f-e7f2902b846e 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:
|
in the golang sdk.
009ec47
to
bb3c0fb
Compare
Build Succeeded 👏 Build Id: 0c0d2661-3e1c-413b-a667-296ad37a5c0a 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.
LGTM!
Also https://bb3c0fb-dot-preview-dot-agones-images.appspot.com/site/docs/guides/client-sdks/unreal/ looks correct - maybe the updated hugo version fixed it?
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: markmandel, roberthbailey The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
I wrote some code to show what option 1 from #851 (comment) would look like, since it seemed like the best option to me.
I'll add tests and do some more checking if we decide to go this route, but I've already verified:
so it seems pretty robust.