Add support for empty ports #2006

ItsKev · 2021-02-25T17:03:39Z

What type of PR is this?

Uncomment only one /kind <> line, hit 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

What this PR does / Why we need it:

This PR adds support for empty ports.

Which issue(s) this PR fixes:

Closes #749

Special notes for your reviewer:

This is my first time working with GO. If I can improve anything feel free to point me in the right direction.
Somehow the install.yaml shows a lot of changes. Looking through them in GoLand however, does not show me any changes at all on certain lines.

agones-bot · 2021-02-25T17:13:37Z

Build Failed 😱

Build Id: a98e3d35-1c69-4242-88d8-d4259bc66350

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

markmandel · 2021-02-25T17:51:05Z

Looks like Helm is being grumpy with the change to the CRD 😬 it's been doing that as of late with the new schema changes, I can handhold that one through e2e testing.

At first glance, this looks good (I'll do a deeper review at some point) - the only thing I would advocate for - somewhere in https://github.com/googleforgames/agones/blob/main/test/e2e/gameserver_test.go write an e2e test that confirms you can create a GameServer with no ports, and nothing falls over.

Thanks for doing this work!

markmandel · 2021-02-25T19:04:35Z

I'm getting this weird error I get sometimes with Github, and I can't update the branch with main.

So I'll wait for the e2e test to come in and walk it through the e2e tests then.

If you run into issues / would like some help, please feel free to drop into #development on Slack, and ask questions as well!

ItsKev · 2021-02-25T20:22:20Z

Gonna write the e2e test tomorrow 👍

ItsKev · 2021-02-26T07:52:08Z

I've added an e2e test but was not able to test it locally against a Google Cloud cluster. I get the same error while trying to use make install that happens in the tests here.

agones-bot · 2021-02-26T07:59:59Z

Build Failed 😱

Build Id: d2ee40af-27d9-4774-8401-331a393da848

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

agones-bot · 2021-02-26T08:01:26Z

Build Failed 😱

Build Id: 8e99de69-935f-4304-a8f0-92fc7264e1ce

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

markmandel · 2021-03-03T22:47:12Z

This looks good, looks like you are hitting helm/helm#5806 which won't get fixed until K8s 1.20 😕 We're hitting this in a couple of other places.

The workaround locally is to do a full delete of the helm installation before re-installing.

I'll hand hold this through e2e test shortly.

markmandel · 2021-03-03T23:59:12Z

Hey - can you give me permission to update your branch please? I can't update it to master.

ItsKev · 2021-03-04T05:30:20Z

The allow edits by maintainers is not possible for organization repositories.

@markmandel I've invited you to write to the repository. If this is not how you handle things I could fork the project with my private account and create a PR again.

markmandel · 2021-03-05T01:19:07Z

The allow edits by maintainers is not possible for organization repositories.

AAAAAAAAAAAAAAAAAAAAAAAAAAAH! That's why I can't update SOME PR's!!! This has been driving me up the wall for eons.

Don't give me write permissions on your repo -- that's seems icky.

Ping me on Slack when you want to update this PR to master, and I can handhold it through e2e tests 👍

markmandel · 2021-03-05T01:22:09Z

Actually, if you are happy, I'll take the write permissions on the repo -- let me know whichever way works best for you.

agones-bot · 2021-03-05T18:38:50Z

Build Failed 😱

Build Id: b35cb2cb-3e9d-41ee-aaee-171e20711ec5

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

agones-bot · 2021-03-05T21:11:59Z

Build Failed 😱

Build Id: 092ac484-0bfd-43d3-8fb3-52593302817f

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

markmandel · 2021-03-05T21:26:16Z

Can you run make gen-install again please? Your new changes combined with the recent updates to the CRDs because of #2007 aren't in that file. Thanks!

markmandel · 2021-03-06T00:58:36Z

Just a heads up - RC is next Tuesday, but let's see if we can squeeze this in.

agones-bot · 2021-03-06T10:54:50Z

Build Failed 😱

Build Id: ba891ad7-ba8a-4aab-bf7e-01dbf2c94623

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

agones-bot · 2021-03-06T12:49:33Z

Build Failed 😱

Build Id: 5963b5ee-f606-43a4-9f33-3e56b0eb6374

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

agones-bot · 2021-03-06T13:10:39Z

Build Failed 😱

Build Id: 72dc8b53-d10a-43a9-9577-858a7e1376fc

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

agones-bot · 2021-03-06T19:38:30Z

Build Succeeded 👏

Build Id: fef7de53-b71c-4e49-ba3a-9e4fa4be5d38

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

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

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

https://170384f-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2006/head:pr_2006 && git checkout pr_2006
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.13.0-170384f

markmandel

Yay we passed!

Just had a couple of minor things that I think could be cleaned up. Would also like a second opinion on CreateGameServerAndWaitUntilReadyWithOptionalPortCheck 🤔

markmandel · 2021-03-06T20:45:53Z

test/e2e/framework/framework.go

+}
+
+// CreateGameServerAndWaitUntilReadyWithOptionalPortCheck Creates a GameServer and wait for its state to become ready.
+func (f *Framework) CreateGameServerAndWaitUntilReadyWithOptionalPortCheck(ns string, gs *agonesv1.GameServer, portCheck bool) (*agonesv1.GameServer, error) {


I'm trying to decide if CreateGameServerAndWaitUntilReadyWithOptionalPortCheck is necessary.

We already cover this by checking on ports in this test for example (I'm sure there are others).

@roberthbailey WDYT?

I get the impetus to check all other tests, but I'm not sure it's required.

Looking at your new test code, I'm guessing that you added this because without passing in false, line 227 (in the old file) was returning an error, because there was no port.

But the test should be able to inspect the passed in game server and tell if a port should be created (by seeing if spec.Ports is nil). And then you can modify the check for ports to read something like "if the spec has a port and none was allocated, return an error" instead of what we have currently ("assume that there will always be a port so check to see if one was allocated").

https://github.com/googleforgames/agones/blob/main/test/e2e/framework/framework.go#L227 - just for reference 😄

That's a really good point @roberthbailey , that could become:

if len(readyGs.Status.Ports) == len(gsCopy.Spec.Ports) { return nil, fmt.Errorf("Ready GameServer instance has no port: %v", readyGs.Status) }

Don't think we need nil checks either, since len(nil) is 0

Which is far more elegant 👍

I think you mean !=

Also, the error message needs to be updated to print the expected number of ports and the observed number of ports:

if len(readyGs.Status.Ports) != len(gsCopy.Spec.Ports) { return nil, fmt.Errorf("Ready GameServer instance has %d port(s), want %v", readyGs.Status, gsCopy.Spec.Ports) }

Ah yes! I did, excellent catch. I like it!

Thanks for the input. I've updated the PR accordingly. 👍

I had to include a check when the port protocol is ProtocolTCPUDP. Hope this is fine.

markmandel · 2021-03-06T20:46:35Z

test/e2e/gameserver_test.go

+	assert.True(t, valid)
+
+	readyGs, err := framework.CreateGameServerAndWaitUntilReadyWithOptionalPortCheck(framework.Namespace, gs, false)
+	if err != nil {


Nit:

This if err != nil block can be changed to:
require.NoError(err) (I know we have the assert.FailNow() pattern in a bunch of places, but that was before we had access to require 😄 )

markmandel · 2021-03-06T20:50:32Z

test/e2e/gameserver_test.go

+		assert.FailNow(t, "Could not get a GameServer ready", err.Error())
+	}
+
+	assert.Nil(t, readyGs.Spec.Ports)


Just a thought. This might be better, just because nil vs an empty array are both valid return results. Not a strong opinion.

https://pkg.go.dev/github.com/stretchr/testify/assert#Empty

Suggested change

assert.Nil(t, readyGs.Spec.Ports)

assert.Empty(t, readyGs.Spec.Ports)

Yes, you are right. assert.Empty itself does a null check.

Done

agones-bot · 2021-03-08T06:49:09Z

Build Failed 😱

Build Id: f142efd1-ddbe-407f-9764-6ec8338052bc

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

agones-bot · 2021-03-08T17:07:09Z

Build Failed 😱

Build Id: 51581103-5fc9-459f-98c9-8210b64bcb77

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

…rts between the started game server and input game server are equal

agones-bot · 2021-03-09T13:21:22Z

Build Failed 😱

Build Id: 039dc21a-b3f5-4673-87b5-839237a0be75

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

agones-bot · 2021-03-09T13:28:26Z

Build Succeeded 👏

Build Id: 38e323f3-1130-4fe6-9330-9339d9d3fb27

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

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

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

https://17c49a2-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2006/head:pr_2006 && git checkout pr_2006
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.13.0-17c49a2

agones-bot · 2021-03-09T13:42:51Z

Build Failed 😱

Build Id: e27af99f-10d0-4d7f-b96a-b1625cba4c4d

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

…colTCPUDP

agones-bot · 2021-03-09T14:38:29Z

Build Succeeded 👏

Build Id: 4074d155-8364-40ca-9429-d9f1755c5b0f

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

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

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

https://24e7e25-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2006/head:pr_2006 && git checkout pr_2006
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.13.0-24e7e25

ItsKev · 2021-03-09T14:47:20Z

The PR got reviewed and is ready to be merged if my changes are accepted.

/assign @cyriltovena

markmandel

I like it! Let's get this merged!

google-oss-robot · 2021-03-09T17:22:34Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

Add support for empty ports

f657aba

google-cla bot added the cla: yes label Feb 25, 2021

google-oss-robot requested review from cyriltovena and EricFortin February 25, 2021 17:03

google-oss-robot added the size/L label Feb 25, 2021

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

Add e2e test for a gameserver without a port

4c66a32

Merge branch 'main' into main

7134da2

Merge branch 'main' into main

16c53e9

Run make gen-install

fb9edff

google-oss-robot added size/M and removed size/L labels Mar 6, 2021

Fix e2e test

2521908

Add missing file

170384f

markmandel reviewed Mar 6, 2021

View reviewed changes

Add suggestions from review

e0e416f

ItsKev and others added 2 commits March 9, 2021 14:05

Merge branch 'main' into main

17c49a2

Improve CreateGameServerAndWaitUntilReady function to check if the po…

c0bbfc7

…rts between the started game server and input game server are equal

Fix lint

66b50e1

Calculate expectedPortCount to fix port check for port protocol Proto…

24e7e25

…colTCPUDP

google-oss-robot assigned cyriltovena Mar 9, 2021

markmandel approved these changes Mar 9, 2021

View reviewed changes

google-oss-robot assigned markmandel Mar 9, 2021

google-oss-robot added the lgtm label Mar 9, 2021

google-oss-robot added the approved label Mar 9, 2021

markmandel merged commit 9a938b3 into googleforgames:main Mar 9, 2021

markmandel added this to the 1.13.0 milestone Mar 9, 2021

	assert.Nil(t, readyGs.Spec.Ports)
	assert.Empty(t, readyGs.Spec.Ports)

Add support for empty ports #2006

Add support for empty ports #2006

Conversation

ItsKev commented Feb 25, 2021

agones-bot commented Feb 25, 2021

markmandel commented Feb 25, 2021

markmandel commented Feb 25, 2021 • edited Loading

ItsKev commented Feb 25, 2021

ItsKev commented Feb 26, 2021

agones-bot commented Feb 26, 2021

agones-bot commented Feb 26, 2021

markmandel commented Mar 3, 2021 • edited Loading

markmandel commented Mar 3, 2021

ItsKev commented Mar 4, 2021

markmandel commented Mar 5, 2021

markmandel commented Mar 5, 2021

agones-bot commented Mar 5, 2021

agones-bot commented Mar 5, 2021

markmandel commented Mar 5, 2021

markmandel commented Mar 6, 2021

agones-bot commented Mar 6, 2021

agones-bot commented Mar 6, 2021

agones-bot commented Mar 6, 2021

agones-bot commented Mar 6, 2021

markmandel left a 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

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 Mar 8, 2021

agones-bot commented Mar 8, 2021

agones-bot commented Mar 9, 2021

agones-bot commented Mar 9, 2021

agones-bot commented Mar 9, 2021

agones-bot commented Mar 9, 2021

ItsKev commented Mar 9, 2021

markmandel left a comment

Choose a reason for hiding this comment

google-oss-robot commented Mar 9, 2021

markmandel commented Feb 25, 2021 •

edited

Loading

markmandel commented Mar 3, 2021 •

edited

Loading