Change to use grpc-dotnet instead of Grpc.Core in C# SDK #3397

yoshd · 2023-09-21T16:52:17Z

What type of PR is this?

Uncomment only one /kind <> line, press enter to put that in a new line, and remove leading whitespace from that line:

/kind breaking
/kind bug

/kind cleanup

/kind documentation
/kind feature
/kind hotfix
/kind release

What this PR does / Why we need it:

Grpc.Core used in the C# SDK is currently deprecated.
https://grpc.io/blog/grpc-csharp-future/

Changed to use grpc-dotnet instead.

Accordingly, code generation for .proto files has been modified to occur at build time, similar to the Rust SDK.
To do this, C#-specific rewritten .proto file was output to the sdks/csharp/sdk folder.

Which issue(s) this PR fixes:

Closes #

Special notes for your reviewer:

yoshd · 2023-09-21T17:41:08Z

sdks/csharp/sdk/AgonesSDK.cs

 using Microsoft.Extensions.Logging;
 using System;
 using System.Threading;
 using System.Threading.Tasks;
+using Grpc.Core;


The namespace Grpc.Core remains, but it is included in grpc-dotnet.

agones-bot · 2023-09-21T18:21:06Z

Build Succeeded 👏

Build Id: ace9678b-3a9e-4208-b627-fedbfa760722

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):

https://564d1ef-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3397/head:pr_3397 && git checkout pr_3397
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.35.0-dev-564d1ef-amd64

agones-bot · 2023-09-22T02:16:24Z

Build Succeeded 👏

Build Id: 64dede6e-da6b-413d-afdc-4eabf3932b60

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):

https://f2d74f8-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3397/head:pr_3397 && git checkout pr_3397
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.35.0-dev-f2d74f8-amd64

agones-bot · 2023-09-22T13:38:44Z

Build Succeeded 👏

Build Id: e120c85b-ce3d-48c0-8edb-586ae79e7161

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):

https://d1660e0-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3397/head:pr_3397 && git checkout pr_3397
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.35.0-dev-d1660e0-amd64

markmandel · 2023-09-22T20:12:28Z

build/build-sdk-images/csharp/gen.sh

+proto=/go/src/agones.dev/agones/proto
+sdk=${proto}/sdk
+googleapis=${proto}/googleapis
+csharp_proto_file_output_dir=/go/src/agones.dev/agones/sdks/csharp/sdk/proto


Curious - does it have to be in sdk/proto? Also wondering if we should update the .gitignore so it doesn't ignore ./proto folder anymore, since the csharp code is now generated at build time.

It doesn't necessarily need to be in sdk/proto, but since only the C# SDK uses this specially rewritten .proto files, I decided to output it there.
Currently it doesn't seem to be ignored by .gitignore, but is there likely to be a problem?

Sorry, I wasn't clear, lemme explain this another way:

Shall we put the proto files in /go/src/agones.dev/agones/sdks/csharp/proto (the sdk seems redundant), which is the same as how we do the same thing with rust https://github.com/googleforgames/agones/tree/main/sdks/rust/proto

I see, I understand!
I did it that way at first, but the actual C# SDK implementation was in sdks/csharp/sdk.
For this reason, although it is redundant, I thought it would be better to put them in the same hierarchy.
WDYT?
https://github.com/googleforgames/agones/tree/main/sdks/csharp/sdk

oooooh! So it is!

I guess what's the usual convention for csharp? Are the protos usually kept with the source code (i.e. sdk) or kept separately (i.e. one level up)?

We should probably just do whatever the regular convention is - I don't actually mind either way.

Normally, the proto file is often shared between the server and client, so I think it should be placed in a separate folder from the source code.
(However, this time it seems to be different in that a proto file exclusively for the C# SDK is generated.)
That said, I agree that it would be better to have a structure similar to most projects.
Now let's modify it to output to sdks/csharp .

Now let's modify it to output to sdks/csharp .

Sorry, wasn't sure I 100% understood.

I think we're saying to leave it at /go/src/agones.dev/agones/sdks/csharp/sdk/proto yes? or no?

Sorry, that's no.
I modify it to csharp/sdk.
And I just committed it.

Got it! That works for me!

agones-bot · 2023-09-22T22:17:47Z

Build Succeeded 👏

Build Id: da80c334-0623-4751-b92c-177a348d3e51

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):

https://c287767-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3397/head:pr_3397 && git checkout pr_3397
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.35.0-dev-c287767-amd64

agones-bot · 2023-09-26T04:30:35Z

Build Succeeded 👏

Build Id: 8f99a110-26fc-413d-ad3d-6ec506eb8d69

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):

https://3e57dc9-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3397/head:pr_3397 && git checkout pr_3397
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.35.0-dev-3e57dc9-amd64

agones-bot · 2023-09-28T18:51:13Z

Build Succeeded 👏

Build Id: 261c389a-8e26-4b9e-9859-aebfe92a5def

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):

https://2f2204b-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3397/head:pr_3397 && git checkout pr_3397
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.35.0-dev-2f2204b-amd64

google-oss-prow · 2023-09-28T20:33:04Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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]

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

google-oss-prow · 2023-09-28T20:36:08Z

New changes are detected. LGTM label has been removed.

agones-bot · 2023-09-28T21:43:03Z

Build Succeeded 👏

Build Id: bfaea9d3-c0b2-4189-8195-57a347489e7d

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):

https://6ab40e6-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3397/head:pr_3397 && git checkout pr_3397
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.36.0-dev-6ab40e6-amd64

yoshd added 8 commits September 21, 2023 05:14

Add conformance test implementation for C# SDK

7680635

add successful log

a074e0e

Fix SetLabel/SetAnnotation tests

ff7e37e

Remove garbage

685a5df

Rename sdk test file

513cf29

Fix SetLabel test

efb936f

Merge branch 'main' into add_csharp_conformance_test

870976b

Change to use grpc-dotnet instead of Grpc.Core

e484d1c

google-oss-prow bot added the do-not-merge/work-in-progress label Sep 21, 2023

google-oss-prow bot requested review from aLekSer and markmandel September 21, 2023 16:52

google-oss-prow bot added the size/XXL label Sep 21, 2023

github-actions bot added the kind/other label Sep 21, 2023

yoshd changed the title ~~Change csharp grpc lib~~ Change to use grpc-dotnet instead of Grpc.Core in C# SDK Sep 21, 2023

Remove Grpc.Core.Testing

564d1ef

github-actions bot added the kind/cleanup Refactoring code, fixing up documentation, etc label Sep 21, 2023

yoshd commented Sep 21, 2023

View reviewed changes

yoshd mentioned this pull request Sep 21, 2023

Add conformance test implementation for C# SDK #3392

Merged

Merge branch 'main' into change_csharp_grpc_lib

f2d74f8

yoshd marked this pull request as ready for review September 22, 2023 01:13

google-oss-prow bot removed the do-not-merge/work-in-progress label Sep 22, 2023

google-oss-prow bot requested review from cyriltovena and roberthbailey September 22, 2023 01:14

Move proto file output

d1660e0

markmandel reviewed Sep 22, 2023

View reviewed changes

Fix proto file output destination for C#

c287767

Merge branch 'main' into change_csharp_grpc_lib

3e57dc9

Merge branch 'main' into change_csharp_grpc_lib

2f2204b

markmandel approved these changes Sep 28, 2023

View reviewed changes

google-oss-prow bot assigned markmandel Sep 28, 2023

google-oss-prow bot added the lgtm label Sep 28, 2023

google-oss-prow bot added the approved label Sep 28, 2023

markmandel enabled auto-merge (squash) September 28, 2023 20:34

Merge branch 'main' into change_csharp_grpc_lib

6ab40e6

google-oss-prow bot removed the lgtm label Sep 28, 2023

markmandel merged commit 6869882 into googleforgames:main Sep 28, 2023
3 checks passed

Kalaiselvi84 removed the kind/other label Nov 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change to use grpc-dotnet instead of Grpc.Core in C# SDK #3397

Change to use grpc-dotnet instead of Grpc.Core in C# SDK #3397

yoshd commented Sep 21, 2023 •

edited

yoshd Sep 21, 2023

agones-bot commented Sep 21, 2023

agones-bot commented Sep 22, 2023

agones-bot commented Sep 22, 2023

markmandel Sep 22, 2023 •

edited

yoshd Sep 22, 2023

markmandel Sep 22, 2023

yoshd Sep 22, 2023

markmandel Sep 22, 2023

yoshd Sep 22, 2023

markmandel Sep 22, 2023

yoshd Sep 22, 2023

markmandel Sep 22, 2023

agones-bot commented Sep 22, 2023

agones-bot commented Sep 26, 2023

agones-bot commented Sep 28, 2023

google-oss-prow bot commented Sep 28, 2023

google-oss-prow bot commented Sep 28, 2023

agones-bot commented Sep 28, 2023

Change to use grpc-dotnet instead of Grpc.Core in C# SDK #3397

Change to use grpc-dotnet instead of Grpc.Core in C# SDK #3397

Conversation

yoshd commented Sep 21, 2023 • edited

Choose a reason for hiding this comment

agones-bot commented Sep 21, 2023

agones-bot commented Sep 22, 2023

agones-bot commented Sep 22, 2023

markmandel Sep 22, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agones-bot commented Sep 22, 2023

agones-bot commented Sep 26, 2023

agones-bot commented Sep 28, 2023

google-oss-prow bot commented Sep 28, 2023

google-oss-prow bot commented Sep 28, 2023

agones-bot commented Sep 28, 2023

yoshd commented Sep 21, 2023 •

edited

markmandel Sep 22, 2023 •

edited