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.

Sign up for GitHub

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

Jump to bottom

Use non-ephemeral port for Go SDK conformance test #1095

Merged
merged 1 commit into from
Oct 7, 2019
Merged

Use non-ephemeral port for Go SDK conformance test #1095

merged 1 commit into from
Oct 7, 2019

Conversation

aLekSer
Copy link
Collaborator

@aLekSer aLekSer commented Oct 3, 2019 •
edited
Loading

Add PORT parameter for SDK conformance tests.
Local SDK server could use different ports as well as SDK client.
First step to run SDK tests in parallel.

@aLekSer
Copy link
Collaborator Author

aLekSer commented Oct 3, 2019

/assign @roberthbailey
This is an initial version which uses ENV variable to switch to SDK client to another port. Will use different ports for Node JS and Rust once they would be merged.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 9439a5e8-3015-472d-8077-af24b991b0dd

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

roberthbailey
roberthbailey reviewed Oct 3, 2019
View reviewed changes
Copy link
Member

@roberthbailey roberthbailey left a comment

Choose a reason for hiding this comment

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

It looks like the nodejs conformance tests failed with this change. Do you think removing the --net=host from the docker run of the sidecar might be the culprit?

build/includes/sdk.mk Show resolved Hide resolved
build/includes/sdk.mk Outdated
docker run -p 59357:59357 -e "ADDRESS=" -e "TEST=$(TESTS)" -e "TIMEOUT=$(TIMEOUT)" -e "DELAY=$(DELAY)" \
--net=host $(sidecar_tag)
DOCKER_RUN_ARGS="--net host -e AGONES_SDK_GRPC_PORT=$(PORT) $(DOCKER_RUN_ARGS)" COMMAND=sdktest $(MAKE) run-sdk-command & \
docker run -p $(PORT):59357 -e "ADDRESS=" -e "TEST=$(TESTS)" -e "TIMEOUT=$(TIMEOUT)" -e "DELAY=$(DELAY)" \
Copy link
Member

Choose a reason for hiding this comment

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

So this maps the configured port on the machine to the default port in the container. I think it would be a bit better to do $(PORT):$(PORT) and then pass --grpc-port=$(PORT) to the sidecar container so that we are testing having the sdkserver bind to the configured port as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that's true, performing this change

@aLekSer aLekSer force-pushed the sdk-conformance-diff-ports branch 2 times, most recently from 8341765 to dae88e0 Compare October 7, 2019 14:02
@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 45fdae47-c4b3-419b-83cd-39d5c64b6366

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

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: b7110690-948a-425b-a826-5c85710d1eff

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/1095/head:pr_1095 && git checkout pr_1095
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.1.0-dae88e0

@aLekSer aLekSer marked this pull request as ready for review October 7, 2019 15:31
roberthbailey
roberthbailey reviewed Oct 7, 2019
View reviewed changes
cmd/sdk-server/Dockerfile Outdated
ENTRYPOINT ["/home/agones/sdk-server"]
CMD [ "--grpc-port", "59357" ]
Copy link
Member

Choose a reason for hiding this comment

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

Does it work without this change? The binary already defaults this flag (in code) so it doesn't seem like we should also need to default it in the dockerfile.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let me check your assumption.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No need for this CMD block, removed

@aLekSer
Copy link
Collaborator Author

aLekSer commented Oct 7, 2019

I need to update commit text accordingly

@aLekSer
Use non-ephemeral port for Go SDK conformance test
33304a9 
Add PORT parameter for SDK conformance tests.
Local SDK server could use different ports as well as SDK client.
First step to run SDK tests in parallel.
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 1fc68441-0efc-4713-8314-3d045c6f6ab5

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/1095/head:pr_1095 && git checkout pr_1095
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.1.0-a2c94ef

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 7fa0c03f-c793-4be3-9fd4-e1ed4e5d9d6a

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/1095/head:pr_1095 && git checkout pr_1095
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.1.0-33304a9

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: aLekSer, roberthbailey
To complete the pull request process, please assign
You can assign the PR to them by writing /assign in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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

@roberthbailey roberthbailey merged commit 840e540 into googleforgames:master Oct 7, 2019
@aLekSer aLekSer deleted the sdk-conformance-diff-ports branch October 7, 2019 18:00
@markmandel markmandel added this to the 1.1.0 milestone Oct 22, 2019
@markmandel markmandel added area/tests Unit tests, e2e tests, anything to make sure things don't break kind/cleanup Refactoring code, fixing up documentation, etc labels Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roberthbailey roberthbailey roberthbailey approved these changes

@EricFortin EricFortin Awaiting requested review from EricFortin

@ilkercelikyilmaz ilkercelikyilmaz Awaiting requested review from ilkercelikyilmaz

Assignees

@roberthbailey roberthbailey

Labels
area/tests Unit tests, e2e tests, anything to make sure things don't break kind/cleanup Refactoring code, fixing up documentation, etc lgtm size/S
Projects
None yet
Milestone
1.1.0
Development

Successfully merging this pull request may close these issues.
5 participants
@aLekSer @agones-bot @google-oss-robot @roberthbailey @markmandel