C# SDK Cleanup & Nuget Package #1596

rcreasey · 2020-05-29T01:38:13Z

What type of PR is this?
/kind bug
/kind cleanup
/kind documentation
/kind feature

What this PR does / Why we need it:
Updates the C# SDK with the following:

Adds stubs for Nuget package publishing.
Updates the gRPC requirement to 2.28 due to timeout issues.
Adds an Interface to easily implement mock testing.
Removes the "baked-in" health checks and requires explicit Health pings on instanced clients.
Changes console output to use Microsoft logging classes.
Ran the code through a linter.

Which issue(s) this PR fixes:

Special notes for your reviewer:
This will still require additional work on the Google side to provide necessary credentials to get the nuget package pushed to official repositories.

googlebot · 2020-05-29T01:38:17Z

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

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

rcreasey · 2020-05-29T01:39:29Z

@googlebot I signed it!

googlebot · 2020-05-29T01:39:33Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

sdks/csharp/sdk/AgonesSDK.nuspec

rcreasey · 2020-05-29T01:48:11Z

To build this as a nuget package the command is straight forward:

dotnet pack csharp-sdk.csproj --configuration Release --include-source --output ./pkgs /p:Version=1.0.4
dotnet nuget push /Users/rcreasey/projects/public/agones/sdks/csharp/sdk/pkgs/AgonesSDK.1.0.4.symbols.nupkg <nuget repository credentials>

For further details: https://docs.microsoft.com/en-us/nuget/create-packages/creating-a-package-dotnet-cli

I've tested this internally on our private Nuget repositories and have been running sdk integrated game servers for the last two weeks.

agones-bot · 2020-05-29T01:51:27Z

Build Failed 😱

Build Id: be26b4c7-6889-49b8-a213-d5dff5318608

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

agones-bot · 2020-05-29T01:52:44Z

Build Failed 😱

Build Id: 2e597929-8cfb-48ca-8d6b-c1fb1e52b672

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

markmandel · 2020-05-29T03:58:58Z

Looking at the logs, it looks like make run-sdk-command COMMAND=test SDK_FOLDER=csharp is failing (this might take a while to build, just letting you know)

Something in the SDK tests isn't working.

agones-bot · 2020-05-29T18:41:14Z

Build Succeeded 👏

Build Id: 6c575c63-ef77-42c8-978d-e46e1c9074b6

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.7.0-16cd38f
image: gcr.io/agones-images/agones-sdk:1.7.0-16cd38f
image: gcr.io/agones-images/agones-ping:1.7.0-16cd38f
Linux C++ SDK (build): agonessdk-1.7.0-16cd38f-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.7.0-16cd38f.zip

A preview of the website (the last 30 builds are retained):

https://16cd38f-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1596/head:pr_1596 && git checkout pr_1596
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.7.0-16cd38f

markmandel · 2020-05-29T19:04:44Z

A couple of quick questions:

Just to confirm, is this a breaking change in the SDK? I.e will old integrations still work with this when they upgrade, or will people need to change the code?

Updates the gRPC requirement to 2.28 due to timeout issues.

Can you expand more on this? Maybe this is something we should upgrade across the board.

markmandel · 2020-05-29T19:06:23Z

Updates the gRPC requirement to 2.28 due to timeout issues.

Are there any issues with the generated code output? Doesn't look like there are, as the generated code wasn't touched, but just wanted to be sure.

agones-bot · 2020-05-29T19:28:58Z

Build Succeeded 👏

Build Id: 7471aa09-7604-4e47-bd1a-c4eadba4037a

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.7.0-39ca617
image: gcr.io/agones-images/agones-sdk:1.7.0-39ca617
image: gcr.io/agones-images/agones-ping:1.7.0-39ca617
Linux C++ SDK (build): agonessdk-1.7.0-39ca617-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.7.0-39ca617.zip

A preview of the website (the last 30 builds are retained):

https://39ca617-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1596/head:pr_1596 && git checkout pr_1596
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.7.0-39ca617

rcreasey · 2020-05-29T22:32:59Z

There should be no breaking changes to the gRPC update; there was an issue with our unit tests starting before the agones api endpoint was available (we're packing the dev server as a fixture and delayed the startup to test) and the stream connection wasn't attempting a retry properly. Instead, it was failing an exception in the grpc library with:

System.Net.Internals.SocketExceptionFactory+ExtendedSocketException (111): Connection refused 127.0.0.1:9357

Updating it to the 2.28 release seemed to fix the issue (which was the stable release at the time we were testing).

No agones code gen required.

sdks/csharp/sdk/AgonesSDK.nuspec

agones-bot · 2020-06-01T15:22:38Z

Build Succeeded 👏

Build Id: ee5cf4b5-ba97-4b17-9ee0-194ea2749a8d

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.7.0-fe8edb8
image: gcr.io/agones-images/agones-sdk:1.7.0-fe8edb8
image: gcr.io/agones-images/agones-ping:1.7.0-fe8edb8
Linux C++ SDK (build): agonessdk-1.7.0-fe8edb8-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.7.0-fe8edb8.zip

A preview of the website (the last 30 builds are retained):

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

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1596/head:pr_1596 && git checkout pr_1596
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.7.0-fe8edb8

agones-bot · 2020-06-01T15:25:56Z

Build Failed 😱

Build Id: 32677196-7c4d-4933-86ab-8125097f2404

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

agones-bot · 2020-06-01T18:44:48Z

Build Succeeded 👏

Build Id: 664c52a8-f626-4cd8-a0b4-57d6364edc72

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.7.0-6a8111c
image: gcr.io/agones-images/agones-sdk:1.7.0-6a8111c
image: gcr.io/agones-images/agones-ping:1.7.0-6a8111c
Linux C++ SDK (build): agonessdk-1.7.0-6a8111c-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.7.0-6a8111c.zip

A preview of the website (the last 30 builds are retained):

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

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1596/head:pr_1596 && git checkout pr_1596
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.7.0-6a8111c

markmandel · 2020-06-01T18:52:31Z

@pooneh-m since you are our resident C# expert, would you mind taking a look at these changes?

sdks/csharp/sdk/AgonesSDK.cs

pooneh-m · 2020-06-04T20:04:04Z

sdks/csharp/sdk/AgonesSDK.cs

 			}
 			catch (RpcException ex)
 			{
-				Console.Error.WriteLine(ex.Message);
-				//Should I rethrow the exception?
-				return null;


This is change in the behavior. I agree that this is how it should have been handled originally, but may break a customer use-case that is already depending on this behavior.
@markmandel would it be ok to suddenly throw exception GetGameServerAsync()? How other SDKs are handling this?

Yeah, I think it's reasonable to throw an exception here. I'm also assuming that in C#, exception throwing is the appropriate way to deal with errorrs?

For example, Go returns an error

The only maybe interesting part, is - do we want to throw the GRPC exception, or do we want to wrap it in our own exception type?

Ah, I forgot about this. Yeah, I figured surfacing the exception would be better than just sinking it like it was previously doing. Typically, libraries shouldn't do this and should surface it to the consuming callsite.

As for whether or not to make a custom AgonesException to wrap it in, I'll leave it up to you. I'm fine with handling a gRPC exception in my code.

If there is no dependency to this SDK that this change would break them, it looks good to me. I don't think we need to wrap the exception.

pooneh-m · 2020-06-04T20:10:55Z

Thanks @rcreasey for the changes and for the fix.

sdks/csharp/sdk/AgonesSDK.cs

pooneh-m

Thanks for the changes. The proposed change to RequestTimeoutSec looks good to me.

pooneh-m · 2020-06-05T20:10:47Z

sdks/csharp/sdk/AgonesSDK.cs

 			}
 			catch (RpcException ex)
 			{
-				Console.Error.WriteLine(ex.Message);
-				//Should I rethrow the exception?
-				return null;


If there is no dependency to this SDK that this change would break them, it looks good to me. I don't think we need to wrap the exception.

google-oss-robot · 2020-06-05T20:11:37Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [pooneh-m]

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

google-oss-robot · 2020-06-05T23:02:59Z

New changes are detected. LGTM label has been removed.

agones-bot · 2020-06-05T23:13:50Z

Build Failed 😱

Build Id: ff2ec003-2944-4e98-a0ae-feb1aae11a83

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

agones-bot · 2020-06-05T23:14:08Z

Build Failed 😱

Build Id: c34abefd-1cf8-4d9a-86ca-1f8c40fb42db

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

agones-bot · 2020-06-06T18:37:14Z

Build Failed 😱

Build Id: e7488cd1-5ff9-4e44-a144-73a4fc2ac660

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

agones-bot · 2020-06-06T18:48:03Z

Build Succeeded 👏

Build Id: 85824577-0585-4658-b19b-1b812b23f7e8

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.7.0-94b8803
image: gcr.io/agones-images/agones-sdk:1.7.0-94b8803
image: gcr.io/agones-images/agones-ping:1.7.0-94b8803
Linux C++ SDK (build): agonessdk-1.7.0-94b8803-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.7.0-94b8803.zip

A preview of the website (the last 30 builds are retained):

https://94b8803-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1596/head:pr_1596 && git checkout pr_1596
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.7.0-94b8803

* C# SDK refactoring; preparing for a nuget package * Updating c# sdk documentation * Fixing csharp sdk tests. * Updating nuget version * Updating project version as well * Version matching the nuspec file. * Update sdks/csharp/sdk/AgonesSDK.cs * Properly renaming the RequestTimeout property to RequestTimeoutSec

rcreasey added 2 commits May 28, 2020 18:19

C# SDK refactoring; preparing for a nuget package

422570c

Updating c# sdk documentation

0f322a9

google-oss-robot added the size/L label May 29, 2020

google-oss-robot requested review from markmandel and roberthbailey May 29, 2020 01:38

rcreasey changed the title ~~Csharp nuget~~ C# SDK Cleanup & Nuget Package May 29, 2020

Merge branch 'master' into csharp-nuget

9dfd8ef

rcreasey commented May 29, 2020

View reviewed changes

sdks/csharp/sdk/AgonesSDK.nuspec Outdated Show resolved Hide resolved

rcreasey and others added 3 commits May 29, 2020 11:23

Fixing csharp sdk tests.

48541c1

Updating nuget version per @markmandel feedback

c1932a5

Merge branch 'master' into csharp-nuget

16cd38f

markmandel requested a review from pooneh-m May 29, 2020 19:03

Updating project version as well

39ca617

sbeca reviewed Jun 1, 2020

View reviewed changes

sdks/csharp/sdk/AgonesSDK.nuspec Outdated Show resolved Hide resolved

rcreasey and others added 2 commits June 1, 2020 08:05

Version matching the nuspec file.

fe8edb8

Merge branch 'master' into csharp-nuget

6a8111c

markmandel assigned pooneh-m Jun 1, 2020

markmandel added area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc kind/feature New features for Agones labels Jun 1, 2020

pooneh-m reviewed Jun 4, 2020

View reviewed changes

roberthbailey removed their request for review June 4, 2020 23:27

rcreasey commented Jun 5, 2020

View reviewed changes

sdks/csharp/sdk/AgonesSDK.cs Outdated Show resolved Hide resolved

rcreasey requested a review from pooneh-m June 5, 2020 19:22

pooneh-m approved these changes Jun 5, 2020

View reviewed changes

google-oss-robot added the lgtm label Jun 5, 2020

google-oss-robot added the approved label Jun 5, 2020

Update sdks/csharp/sdk/AgonesSDK.cs

54d475f

google-oss-robot removed the lgtm label Jun 5, 2020

Merge branch 'master' into csharp-nuget

992a924

rcreasey and others added 2 commits June 6, 2020 11:24

Properly renaming the RequestTimeout property to RequestTimeoutSec

e414195

Merge branch 'master' into csharp-nuget

94b8803

pooneh-m merged commit 2486f54 into googleforgames:master Jun 6, 2020

markmandel added this to the 1.7.0 milestone Jun 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C# SDK Cleanup & Nuget Package #1596

C# SDK Cleanup & Nuget Package #1596

rcreasey commented May 29, 2020

googlebot commented May 29, 2020

rcreasey commented May 29, 2020

googlebot commented May 29, 2020

rcreasey commented May 29, 2020 •

edited

Loading

agones-bot commented May 29, 2020

agones-bot commented May 29, 2020

markmandel commented May 29, 2020

agones-bot commented May 29, 2020

markmandel commented May 29, 2020

markmandel commented May 29, 2020

agones-bot commented May 29, 2020

rcreasey commented May 29, 2020 •

edited

Loading

agones-bot commented Jun 1, 2020

agones-bot commented Jun 1, 2020

agones-bot commented Jun 1, 2020

markmandel commented Jun 1, 2020

pooneh-m Jun 4, 2020

markmandel Jun 5, 2020

rcreasey Jun 5, 2020

pooneh-m Jun 5, 2020

pooneh-m commented Jun 4, 2020

pooneh-m left a comment

pooneh-m Jun 5, 2020

google-oss-robot commented Jun 5, 2020

google-oss-robot commented Jun 5, 2020

agones-bot commented Jun 5, 2020

agones-bot commented Jun 5, 2020

agones-bot commented Jun 6, 2020

agones-bot commented Jun 6, 2020

C# SDK Cleanup & Nuget Package #1596

C# SDK Cleanup & Nuget Package #1596

Conversation

rcreasey commented May 29, 2020

googlebot commented May 29, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

rcreasey commented May 29, 2020

googlebot commented May 29, 2020

rcreasey commented May 29, 2020 • edited Loading

agones-bot commented May 29, 2020

agones-bot commented May 29, 2020

markmandel commented May 29, 2020

agones-bot commented May 29, 2020

markmandel commented May 29, 2020

markmandel commented May 29, 2020

agones-bot commented May 29, 2020

rcreasey commented May 29, 2020 • edited Loading

agones-bot commented Jun 1, 2020

agones-bot commented Jun 1, 2020

agones-bot commented Jun 1, 2020

markmandel commented Jun 1, 2020

pooneh-m Jun 4, 2020

Choose a reason for hiding this comment

markmandel Jun 5, 2020

Choose a reason for hiding this comment

rcreasey Jun 5, 2020

Choose a reason for hiding this comment

pooneh-m Jun 5, 2020

Choose a reason for hiding this comment

pooneh-m commented Jun 4, 2020

pooneh-m left a comment

Choose a reason for hiding this comment

pooneh-m Jun 5, 2020

Choose a reason for hiding this comment

google-oss-robot commented Jun 5, 2020

google-oss-robot commented Jun 5, 2020

agones-bot commented Jun 5, 2020

agones-bot commented Jun 5, 2020

agones-bot commented Jun 6, 2020

agones-bot commented Jun 6, 2020

rcreasey commented May 29, 2020 •

edited

Loading

rcreasey commented May 29, 2020 •

edited

Loading