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

Simplify the selection of the dynamic port in the nodejs sdk. #1128

Merged
merged 1 commit into from
Oct 21, 2019
Merged

Simplify the selection of the dynamic port in the nodejs sdk. #1128

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 6 additions & 18 deletions sdks/nodejs/spec/agonesSDK.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,28 +32,16 @@ describe('agones', () => {
expect(port).toEqual('59357');
});

it('returns the default port if $AGONES_SDK_GRPC_PORT is not an integer', async () => {
process.env.AGONES_SDK_GRPC_PORT = "random string";
let port = agonesSDK.port;
expect(port).toEqual('59357');
});

it('returns the default port if $AGONES_SDK_GRPC_PORT is to large of an integer', async () => {
process.env.AGONES_SDK_GRPC_PORT = '4455667788';
let port = agonesSDK.port;
expect(port).toEqual('59357');
});

it('returns the default port if $AGONES_SDK_GRPC_PORT is to small of an integer', async () => {
process.env.AGONES_SDK_GRPC_PORT = '0';
it('returns a valid port set in $AGONES_SDK_GRPC_PORT', async () => {
process.env.AGONES_SDK_GRPC_PORT = '6789';
let port = agonesSDK.port;
expect(port).toEqual('59357');
expect(port).toEqual('6789');
});

it('returns the port set in $AGONES_SDK_GRPC_PORT', async () => {
process.env.AGONES_SDK_GRPC_PORT = '6789';
it('returns an invalid port set in $AGONES_SDK_GRPC_PORT', async () => {
process.env.AGONES_SDK_GRPC_PORT = 'foo';
let port = agonesSDK.port;
expect(port).toEqual('6789');
expect(port).toEqual('foo');
});
});

Expand Down
16 changes: 1 addition & 15 deletions sdks/nodejs/src/agonesSDK.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,22 +25,8 @@ class AgonesSDK {
}

get port() {
const defaultPort = '59357';
const port = process.env.AGONES_SDK_GRPC_PORT;
Copy link
Collaborator

Choose a reason for hiding this comment

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

In JavaScript it's common to use || e.g. do return process.env.AGONES_SDK_GRPC_PORT || '59357'
Without examining grpc code too much it seems port should be a number so you could cast using return Number(process.env.AGONES_SDK_GRPC_PORT) || 59357

Copy link
Collaborator

Choose a reason for hiding this comment

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

doh too slow :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm happy to fix it in a follow-up PR. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

#1129

it seems port should be a number

The value is returned and concatenated with 'localhost' so I think returning a string is ok.

if (port === undefined) {
console.log(`Environment variable AGONES_SDK_GRPC_PORT not defined, using default port ${defaultPort}`);
return defaultPort;
}
const portNum = parseInt(port, 10);
if (isNaN(portNum)) {
console.log(`Unable to parse '${port}' defined in AGONES_SDK_GRPC_PORT into an integer`);
return defaultPort;
}
if (portNum < 1 || portNum > 65535) {
console.log(`Invalid port ${portNum} defined in AGONES_SDK_GRPC_PORT. It must be between 1 and 65535`);
return defaultPort;
}
return port;
return port === undefined ? '59357' : port;
}

async connect() {
Expand Down