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.

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

Adding the C# gRPC SDK #1315

Merged
merged 2 commits into from
Mar 31, 2020
Merged

Conversation

Reousa
Copy link
Contributor

@Reousa Reousa commented Feb 4, 2020

A gRPC based SDK for the C# language, compiled against the .NET Standard 2.0.
#884

To do:
[x] Write the SDK's core AgonesSDK wrapper.
[x] Write the docs for it.
[ ] Make it publish as a NuGet package
[x] Fix google/api missing directory issue
[ ] Integrate into the conformance test.
[x] Integrate the code integiry tests.
[x] Integrate into the CI pipeline.

Some issues:

  • The SDK requires the google/api folder to be based in the same directory as sdk.proto as opposed to proto/googleapis/... .
  • I'm unsure where/how the mock-client should be, directory structure wise & CI wise.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@Reousa
Copy link
Contributor Author

Reousa commented Feb 4, 2020

/assign @pooneh-m
Could use some directing, thanks!

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 41b5738e-2db2-4fcb-9ae9-e3d39d59ae7e

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/GoogleCloudPlatform/agones.git pull/1315/head:pr_1315 && git checkout pr_1315
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.3.0-f52be8b

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: ca4e2676-a6dd-4984-a503-af48465b9965

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/GoogleCloudPlatform/agones.git pull/1315/head:pr_1315 && git checkout pr_1315
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.4.0-19a60ef

Copy link
Contributor

@pooneh-m pooneh-m left a comment

Choose a reason for hiding this comment

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

can you reuse annotations.proto and http.proto at here

sdks/csharp/csharp.sln Outdated Show resolved Hide resolved
sdks/csharp/.gitignore Outdated Show resolved Hide resolved
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: ba627446-c719-4446-a362-37ef6822dc9a

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/GoogleCloudPlatform/agones.git pull/1315/head:pr_1315 && git checkout pr_1315
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.3.0-f52be8b

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: cc36bb36-e113-4e09-a8fb-b42faabb4ccc

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/GoogleCloudPlatform/agones.git pull/1315/head:pr_1315 && git checkout pr_1315
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.3.0-4b2331b

@markmandel
Copy link
Member

Thanks so much for putting the work in on this 👍

The biggest concern I have is that this is not integrated into our gRPC SDK build toolchain, which you can find here:
https://github.com/googleforgames/agones/tree/master/build/build-sdk-images

Without this, it's going to be very hard for maintainers to update this client for new functionality. Would it be possible to integrate this into your PR?

@Reousa
Copy link
Contributor Author

Reousa commented Feb 5, 2020

Thanks so much for putting the work in on this 👍

The biggest concern I have is that this is not integrated into our gRPC SDK build toolchain, which you can find here:
https://github.com/googleforgames/agones/tree/master/build/build-sdk-images

Without this, it's going to be very hard for maintainers to update this client for new functionality. Would it be possible to integrate this into your PR?

Happy to contribute! :)

Yeah I was wondering where to get started, thanks for the heads up! I'll get cracking at it this weekend and update the PR when ready 👌

@Reousa
Copy link
Contributor Author

Reousa commented Feb 14, 2020

@markmandel Was wondering how the nuget API key would be integrated into the publish script? I haven't written it yet, is there some environment variable that I should take into account?

Also, for some reason when I run make test-sdk SDK_FOLDER=csharp, the sidecar doesn't seem to be available for connection. Any idea where to look?

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: bf630313-f6d5-484f-9e89-b8c9f99f673c

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

@Reousa
Copy link
Contributor Author

Reousa commented Feb 16, 2020

Update: I've hacked a bit more at it tonight, it seems to be that this readme is outdated (?)

From my understanding so far, test.sh is used to run code integrity tests, while build-sdk-test.sh is the conformance test. However, when I run the latter, I still can't connect to the sidecar?

There's also sdktest.sh which seems to relate to the conformance test, but isn't run when COMMAND=build-sdk-test.

Getting a little lost here, appreciate any direction!

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.

Good digging 👍 and very close! Nice job.

Have a look at these run-sdk-conformance-test-* for example of conformance tests. (it runs the sidecar for you)

Also note - you can do one PR for generating the gRPC Client, and some simple unit tests with some docs.

Conformance tests, examples, etc can be a separate PR (in fact it's easier to review that way), it doesn't have to come all at once. 👍 (Ditto for publish - we're going to have to rejig that node one anyway, because internal policies)

Sound good?

build/build-sdk-images/csharp/Dockerfile Outdated Show resolved Hide resolved
build/build-sdk-images/csharp/test.sh Outdated Show resolved Hide resolved
sdks/csharp/sdk/AgonesSDK.cs Outdated Show resolved Hide resolved
test/sdk/csharp/test-client/Program.cs Outdated Show resolved Hide resolved
@Reousa Reousa marked this pull request as ready for review February 25, 2020 17:10
@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 542cf023-150f-45f3-9771-9d97dc07acde

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

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: f546e567-a03e-43b4-89e8-b7a5d8d7ed09

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

@Reousa Reousa requested a review from markmandel February 25, 2020 20:13
@Reousa
Copy link
Contributor Author

Reousa commented Mar 14, 2020

Actually this is a good point, we should have the generated code in here, and a gen.sh to run it as required through our dev system when we need to refresh it.

It's automatically generated by the Grpc.Tools package before any sort of build step, should I still include a dedicated gen.sh script?

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 40d52bf9-dc51-4a24-a779-26fb88e132c1

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

@markmandel
Copy link
Member

It's automatically generated by the Grpc.Tools package before any sort of build step, should I still include a dedicated gen.sh script?

I'm not quite sure what this means -- is there something I can read? (remember, I know very little about the C# ecosystem 😭 )

With other languages we bundle the generated code so that our users don't need to generate it. But maybe that doesn't make sense in the C# ecosystem?

@Reousa
Copy link
Contributor Author

Reousa commented Mar 17, 2020

With other languages we bundle the generated code so that our users don't need to generate it. But maybe that doesn't make sense in the C# ecosystem?

Copy that, I feel the same with golang! 😅 😅

As for how gRPC works in C#:
The Grpc.Tools NuGet package contains the protoc and protobuf C# plugin binaries needed to generate the code. Starting from version 1.17 the package also integrates with MSBuild to provide automatic C# code generation from .proto files.

Which means, whenever the C# SDK is built, it will refresh the auto-generated code by default, unless told otherwise.
There's no strict "how it should be done" in C# as far as I've seen in the grpc docs, so I'll just make it work like the other SDKs for ease of use to the maintainers, should be about the same for users. Sounds good?

@markmandel
Copy link
Member

There's no strict "how it should be done" in C# as far as I've seen in the grpc docs, so I'll just make it work like the other SDKs for ease of use to the maintainers, should be about the same for users. Sounds good?

I'm assuming that means that we'll generate the C# code?

That makes sense to me - it also means that the users of the SDK don't need to go hunting for where the proto file is, or install any tooling. 👍

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: d59bcbe6-5caf-4919-b65f-24f610251d98

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

@markmandel
Copy link
Member

  Restore completed in 9.65 sec for /go/src/agones.dev/agones/sdks/csharp/sdk/csharp-sdk.csproj.
/usr/share/dotnet/sdk/2.2.402/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Publish.targets(169,5): error MSB3030: Could not copy the file "obj/Release/netstandard2.0/AgonesSDK.pdb" because it was not found. [/go/src/agones.dev/agones/sdks/csharp/sdk/csharp-sdk.csproj]
/usr/share/dotnet/sdk/2.2.402/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Publish.targets(169,5): error MSB3030: Could not copy the file "obj/Release/netstandard2.0/AgonesSDK.dll" because it was not found. [/go/src/agones.dev/agones/sdks/csharp/sdk/csharp-sdk.csproj]

😞

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: df630ae8-96f4-4360-9826-afa8ab2af677

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

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 6137ac9a-b6c0-457f-8df0-fc907aa4c1aa

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

@Reousa
Copy link
Contributor Author

Reousa commented Mar 27, 2020

@markmandel Hey, I think I've fixed the C# build, gen and test issues. Took a look at the logs, both build & test seems to pass, but the overall tests are failing.

I couldn't pinpoint whether it's related to the C# sdk or not though, could you kindly take a look? 🤔

Once you confirm it works, I'll clean up, rebase & squash.

@markmandel
Copy link
Member

Ooh! Looks like you hit some rate limiting with the website link checker against github. If you rebase against master it should go away. I'll hit "update branch" here, and it should go away.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: fc96c9c1-bd76-4c3d-8453-23393604c1d0

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/GoogleCloudPlatform/agones.git pull/1315/head:pr_1315 && git checkout pr_1315
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.5.0-f650a37

@Reousa
Copy link
Contributor Author

Reousa commented Mar 27, 2020

Ooh! Looks like you hit some rate limiting with the website link checker against github. If you rebase against master it should go away. I'll hit "update branch" here, and it should go away.

Hey, it works at last! 🎉
I'll squash probably later tonight.

On a side note, how can I follow up with plans for agones? I'd like to know when you guys plan out the publish specifics :)

@markmandel
Copy link
Member

On a side note, how can I follow up with plans for agones? I'd like to know when you guys plan out the publish specifics :)

Everything is planned out on Github! Keep track there, and/or come join us in Slack.

The monthly community meetings also are a great place for discussion. Please drop by!

Added a C# wrapper for the Agones gRPC client built against .NET Standard 2.0 & Code Integrity tests built against .NET Core 2.2.

Commit includes:
 - Core C# Wrapper - Fully async & automatic health checks.
 - Agones CI Gen, Build & Test scripts.
 - Docs.
 - Relevant .gitignore modifications.
 - Make targets for C# SDK
 - Configuration specific auto-generate (DebugProtoGen), for development purposes

Resolves: googleforgames#884 , googleforgames#883
@Reousa Reousa requested a review from markmandel March 29, 2020 16:08
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: d87cc941-dc2f-4e5f-97d4-906ed6e91c5e

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/GoogleCloudPlatform/agones.git pull/1315/head:pr_1315 && git checkout pr_1315
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.5.0-8dfd7ff

@markmandel
Copy link
Member

Looks like this is good to go 👍 Let's get this merged!

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: markmandel, pooneh-m, Reousa

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:
  • OWNERS [markmandel,pooneh-m]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 5fdaf281-1e16-41a9-a6f5-f499ad2edf60

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/GoogleCloudPlatform/agones.git pull/1315/head:pr_1315 && git checkout pr_1315
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.5.0-2eeb653

@markmandel markmandel merged commit 0850d9e into googleforgames:master Mar 31, 2020
@markmandel markmandel added this to the 1.5.0 milestone Mar 31, 2020
ilkercelikyilmaz pushed a commit to ilkercelikyilmaz/agones that referenced this pull request Oct 23, 2020
Added a C# wrapper for the Agones gRPC client built against .NET Standard 2.0 & Code Integrity tests built against .NET Core 2.2.

Commit includes:
 - Core C# Wrapper - Fully async & automatic health checks.
 - Agones CI Gen, Build & Test scripts.
 - Docs.
 - Relevant .gitignore modifications.
 - Make targets for C# SDK
 - Configuration specific auto-generate (DebugProtoGen), for development purposes

Resolves: googleforgames#884 , googleforgames#883

Co-authored-by: Mark Mandel <markmandel@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc kind/feature New features for Agones lgtm size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants