Expose SDK-Server at HTTP+JSON #265
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
markmandel
added
kind/feature
markmandel
commented
Jun 13, 2018
| @@ -56,56 +59,33 @@ var ( | |||
| ) | |||
|
|
|||
| func main() { | |||
There was a problem hiding this comment.
@enocom I refactored this to look more like the controller main, so I could also do the grpc-gateway proxy.
Would love your eyes on it.
|
Build Failed 😱 Build Id: 633054d9-9ed6-4650-8c90-efa45a7c6805 Build Logs |
|
Build Failed 😱 Build Id: ee8a397c-1685-4b97-b393-9a7aeb76b227 Build Logs |
enocom
reviewed
Jun 14, 2018
| runtime.Must(viper.BindEnv(healthInitialDelayFlag)) | ||
| runtime.Must(viper.BindEnv(healthFailureThresholdFlag)) | ||
| runtime.Must(viper.BindPFlags(pflag.CommandLine)) | ||
| ctlConf := parseEnvFlags() |
There was a problem hiding this comment.
This is a nice touch. Makes it easy to quickly read through main and stop on env flags only if it's necessary.
There was a problem hiding this comment.
I just copied the pattern in the other main.go 👍 no credit to me!
enocom
reviewed
Jun 14, 2018
cmd/sdk-server/main.go
Outdated
|
|
||
| // specifically env vars | ||
| gameServerNameEnv = "GAMESERVER_NAME" | ||
| podNamespaceEnv = "POD_NAMESPACE" | ||
|
|
||
| // Flags (that can also be env vars) | ||
| localFlag = "local" | ||
| addressFlag = "address" | ||
| addressFlag = "Address" |
There was a problem hiding this comment.
What's the rationale for this change?
There was a problem hiding this comment.
whoops. Probably a refactor gone awry. Rolled back.
enocom
reviewed
Jun 14, 2018
cmd/sdk-server/main.go
Outdated
|
|
||
| go s.Run(ctx.Done()) | ||
| sdk.RegisterSDKServer(grpcServer, s) | ||
| } | ||
|
|
||
| run(ctx, grpcServer, lis, grpcEndpoint, mux, httpServer) |
There was a problem hiding this comment.
More a matter of style, but I prefer making functions synchronous and then prefacing them with the go keyword.
There was a problem hiding this comment.
Good call. splitting this out into two synchronous functions reads a lot better.
enocom
reviewed
Jun 14, 2018
cmd/sdk-server/main.go
Outdated
| // then the gameserver is being shut down, and we no longer | ||
| // care about running RPC calls. | ||
| grpcServer.Stop() | ||
| conn, err := grpc.DialContext(ctx, grpcEndpoint, grpc.WithBlock(), grpc.WithInsecure()) |
There was a problem hiding this comment.
That grpc.WithBlock is a tricky API isn't it. Nice usage.
enocom
reviewed
Jun 14, 2018
cmd/sdk-server/main.go
Outdated
| } | ||
|
|
||
| // run runs all the services | ||
| func run(ctx context.Context, grpcServer *grpc.Server, lis net.Listener, grpcEndpoint string, |
There was a problem hiding this comment.
One way to get around all the arguments to run would be to introduce a type, maybe sdkServer and attach run to it. But again, style. Not a big deal.
enocom
approved these changes
Jun 14, 2018
markmandel
force-pushed
the
feature/http+json
branch
from
1c9cee4
to
fd87f76
Compare
|
Build Succeeded 👏 Build Id: d2daff07-dbfe-464a-a781-ccfc3c79fcf9 The following development artifacts have been built, and will exist for the next 30 days:
|
markmandel
force-pushed
the
feature/http+json
branch
from
fd87f76
to
78b2e08
Compare
|
@enocom I think I fixed up most of your concerns - PTAL! |
markmandel
force-pushed
the
feature/http+json
branch
from
78b2e08
to
4fba30c
Compare
|
Build Succeeded 👏 Build Id: 00539c28-5d13-4467-b252-0581f2a62435 The following development artifacts have been built, and will exist for the next 30 days:
|
cyriltovena
approved these changes
Jun 17, 2018
| header sdk.grpc.pb.cc | ||
| header sdk.pb.cc | ||
|
|
||
| cd ./google/api/ |
There was a problem hiding this comment.
what is this for ? I didn't know that adding the http gateway will affect the grpc client gen.
There was a problem hiding this comment.
It does because the annotations.proto is included in the sdk.proto file, it generated .cc and .h files that are used in places.
Also it does because the gen is never just the client - it's the client AND the server parts, we just use the client parts -- I don't think there is a way to ask to just generate the client AFAIK. (Also, this grpc cpp stuff will go away eventually, so for now, making it "just work" is more of a priority so we can replace the whole thing, IMHO)
|
|
||
| If client generation is not well supported by gRPC, or if there are other complicating factors, implement the SDK through | ||
| the [REST](../docs/sdk_rest_api.md) HTTP+JSON interface. This could be written by hand, or potentially generated from | ||
| the [Swagger/OpenAPI Spec](../sdk.swagger.json). |
There was a problem hiding this comment.
Should we show an example on how to do that ?
docker run --rm -v ${PWD}:/local swaggerapi/swagger-codegen-cli generate -i /local/sdks/sdk.swagger.json -l cpprest -o /local/out/cpp
Do you plan to add a SDK using the http gateway ?
There was a problem hiding this comment.
Ooh. I like this idea. I'd like to point to a central documentation too - is this it?
https://swagger.io/docs/open-source-tools/swagger-codegen/
Re: to add an SDk using the gateway. I wasn't going to to do it with this PR, I figure Unreal + CPP would end up building on top of this work, and be the examples. We could likely follow the same pattern as the existing SDKs (light wrapper around the possibly generated client), but was waiting to see what an actual implementation would look like. WDYT?
There was a problem hiding this comment.
Yes that's the link, the plan sounds good !
|
Build Succeeded 👏 Build Id: 67931b07-94c5-4745-bf57-4a8f1e7e469b The following development artifacts have been built, and will exist for the next 30 days:
|
Implement grpc-gateway in front of the gRPC based sdk-server, so that it can be access via HTTP+JSON. This includes documentation and a swagger/openapi specificiation. This also has been implemented such that the sdk-server is still a single binary, and as such, the HTTP+JSON interface can still be used for local development. Closes googleforgames#240
markmandel
force-pushed
the
feature/http+json
branch
from
44c0986
to
1fa8ef3
Compare
markmandel
commented
Jun 17, 2018
|
|
||
| Generally the REST interface gets used if gRPC isn't well supported for a given language or platform. | ||
|
|
||
| ## Generating clients |
There was a problem hiding this comment.
@Kuqd I added this section here, PTAL!
(Also, I ran the command to gen the client -- that's pretty impressive!)
|
Build Succeeded 👏 Build Id: fc1ac703-f292-4655-97ac-54cdc3dce2c0 The following development artifacts have been built, and will exist for the next 30 days:
|
cyriltovena
approved these changes
Jun 18, 2018
|
Build Succeeded 👏 Build Id: 7bf4892d-b9ee-49be-9bb0-539d66d9dff3 The following development artifacts have been built, and will exist for the next 30 days:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Implement grpc-gateway in front of the gRPC based sdk-server, so that it can be access via HTTP+JSON.
This includes documentation and a swagger/openapi specification.
This also has been implemented such that the sdk-server is still a single binary, and as such, the HTTP+JSON interface can still be used for local development.
Closes #240