UTY 2088: Set external ip from cli #1409

BryanJY-Wong · 2020-06-29T11:33:32Z

Description

Allow the setting of UseExternalIp flag from the cli.

Added new bool field for UseExternalIp in ReceptionistFlow and LocatorFlow
Added appropriate name and defaults in the RuntimeConfig classes RuntimeConfigNames and RuntimeConfigDefaults
Added unit test to check if the UseExternalIp is set if the argument exists and set to default if it does not

Tests

Added and ran unit test in CommandLineConnectionFlowInitializerTests

Documentation

https://documentation.improbable.io/gdk-for-unity/v0.3.8/suggested-edits

Primary reviewers

If your change will take a long time to review, you can name at most two primary reviewers who are ultimately responsible for reviewing this request. @ mention them.

improbable-prow-robot · 2020-06-29T11:33:33Z

Corresponding JIRA ticket: https://improbableio.atlassian.net/browse/UTY-2088

workers/unity/Packages/io.improbable.gdk.core/Worker/ConnectionHandlers/ConnectionFlows.cs

CHANGELOG.md

jamiebrynes7

As it stands, I don't believe our command line parser supports flag style arguments (https://github.com/spatialos/gdk-for-unity/blob/master/workers/unity/Packages/io.improbable.gdk.core/Utility/CommandLineUtility.cs#L83). What will currently happen is the +useExternalIp argument will consume the next argument if used as a flag.

I.e. - UnityClient.exe +workerType UnityClient +useExternalIp +receptionistHost localhost

Would parse as:

workerType -> UnityClient
useExternalIp -> +receptionistHost

That being said, I do agree that this should be a flag style argument.

CHANGELOG.md

workers/unity/Packages/io.improbable.gdk.core/Utility/CommandLineUtility.cs

CHANGELOG.md

workers/unity/Packages/io.improbable.gdk.core/Utility/CommandLineUtility.cs

BryanJY-Wong · 2020-06-30T09:29:54Z

...obable.gdk.core/Tests/Editmode/Connection/CommandLineConnectionParametersInitializerTests.cs

@@ -30,7 +30,6 @@ public void Initialize_should_set_network_protocol(string protocolStr, NetworkCo

        [TestCase("ranket")]
        [TestCase("raknet")]
-        [TestCase("")]


Had to remove this. The initializer now assumes empty string is a result of parsing a flag argument and will throw InvalidOperationException when you try to use TryGetCommandLineValue on it.

jamiebrynes7

LGTM - sorry about the current build flake, if you push an empty commit it will retrigger and should pass:

git commit --allow-empty -m 'force rebuild' && git push origin HEAD

There appears to be a bug where we don't truncate the commit message before using it as a parameter elsewhere

zeroZshadow · 2020-06-30T10:56:43Z

workers/unity/Packages/io.improbable.gdk.core/Utility/CommandLineUtility.cs

@@ -59,6 +59,11 @@ public bool TryGetCommandLineValue<T>(string key, ref T value)
            var desiredType = typeof(T);
            if (arguments.TryGetValue(key, out var strValue))
            {
+                if (string.IsNullOrEmpty(strValue))


I think this will be hit when you do the following as well +someArgument "".
I don't think there is a difference currently between "no" argument and an "empty" argument?

@zeroZshadow Should I allow empty argument to pass or throw a different exception for empty argument?

I think an empty string as a value is valid, so we'd need a different way to differentiate between a flag and an empty value. @jamiebrynes7 @paulbalaji @seanjparker Opinions?

I can just set it to null for flags.

null for flags and string.Empty for truly empty arguments seems reasonable to me

Co-authored-by: Jamie Brynes <[email protected]>

Make exception message clearer by referring to flag argument

…yGetCommandLineValue, also fix some coding style issue

`CommanLineParser` -> `CommandLineParser` Co-authored-by: Jamie Brynes <[email protected]>

jamiebrynes7

Nice job!

zeroZshadow · 2020-07-01T09:44:20Z

workers/unity/Packages/io.improbable.gdk.core/Tests/Editmode/Utility/CommandLineUtilityTest.cs

+        {
+            var args = CommandLineArgs.From(new List<string>
+            {
+                "+key1",


This would be a better test by changing the keys around. An empty one first, and then one with a value.
This way you test that the parser doesn't skip the empty key

paulbalaji

LGTM % Martijn's comment

sonarqubecloud · 2020-07-01T14:50:50Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

100.0% Coverage
0.0% Duplication

BryanJY-Wong self-assigned this Jun 29, 2020

improbable-prow-robot added the jira/UTY label Jun 29, 2020

improbable-prow-robot requested review from jamiebrynes7 and paulbalaji June 29, 2020 11:33

improbable-prow-robot added A: core Area: Core GDK size/M Denotes a PR that changes 40-149 lines, ignoring generated files. labels Jun 29, 2020

BryanJY-Wong requested review from seanjparker, zeroZshadow and austinkelmore June 29, 2020 11:36

paulbalaji reviewed Jun 29, 2020

View reviewed changes

workers/unity/Packages/io.improbable.gdk.core/Worker/ConnectionHandlers/ConnectionFlows.cs Outdated Show resolved Hide resolved

BryanJY-Wong requested a review from paulbalaji June 29, 2020 14:11

improbable-prow-robot added size/S Denotes a PR that changes 15-39 lines, ignoring generated files. and removed size/M Denotes a PR that changes 40-149 lines, ignoring generated files. labels Jun 29, 2020

paulbalaji approved these changes Jun 29, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

jamiebrynes7 suggested changes Jun 29, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

improbable-prow-robot added size/M Denotes a PR that changes 40-149 lines, ignoring generated files. and removed size/S Denotes a PR that changes 15-39 lines, ignoring generated files. labels Jun 29, 2020

BryanJY-Wong commented Jun 29, 2020

View reviewed changes

workers/unity/Packages/io.improbable.gdk.core/Utility/CommandLineUtility.cs Outdated Show resolved Hide resolved

BryanJY-Wong requested review from jamiebrynes7 and paulbalaji June 29, 2020 16:10

zeroZshadow reviewed Jun 29, 2020

View reviewed changes

workers/unity/Packages/io.improbable.gdk.core/Utility/CommandLineUtility.cs Outdated Show resolved Hide resolved

jamiebrynes7 reviewed Jun 29, 2020

View reviewed changes

BryanJY-Wong requested a review from jamiebrynes7 June 29, 2020 17:23

BryanJY-Wong commented Jun 30, 2020

View reviewed changes

jamiebrynes7 approved these changes Jun 30, 2020

View reviewed changes

zeroZshadow reviewed Jun 30, 2020

View reviewed changes

BryanJY-Wong added 3 commits July 1, 2020 09:44

Allow setting of external ip from the cli

713754b

Added changelog entry for PR

9318486

Set UseExternalIp in connection parameters instead of connection flow

8c1d418

BryanJY-Wong and others added 11 commits July 1, 2020 09:44

Remove unused constants and unused includes

065a67e

Changed argument to flag instead of boolean string

594a70f

Update CHANGELOG.md for clarity

b5ec72d

Co-authored-by: Jamie Brynes <[email protected]>

Added support for flag only arguments in the CommandLineParser

0ef889c

Update CommandLineUtility.cs

8ae6e3d

Make exception message clearer by referring to flag argument

Change argument exception to format exception

22fe24f

Cleanup if-else in CommandLineUtility

91d45e4

Change Format Exception when attempting to get flag argument using Tr…

385743a

…yGetCommandLineValue, also fix some coding style issue

Update CHANGELOG.md

bc51717

`CommanLineParser` -> `CommandLineParser` Co-authored-by: Jamie Brynes <[email protected]>

allow empty string args

4e4fadc

Fix initialize flag arg test

3fbed4f

BryanJY-Wong force-pushed the feature/UTY-2088-set-external-ip-from-cli branch from 41bee17 to 3fbed4f Compare July 1, 2020 08:45

BryanJY-Wong requested review from zeroZshadow and jamiebrynes7 July 1, 2020 08:46

jamiebrynes7 approved these changes Jul 1, 2020

View reviewed changes

zeroZshadow approved these changes Jul 1, 2020

View reviewed changes

paulbalaji approved these changes Jul 1, 2020

View reviewed changes

BryanJY-Wong added 2 commits July 1, 2020 15:16

update test

ea89bde

Merge branch 'develop' into feature/UTY-2088-set-external-ip-from-cli

522d075

BryanJY-Wong merged commit 7400dd1 into develop Jul 2, 2020

improbable-prow-robot deleted the feature/UTY-2088-set-external-ip-from-cli branch July 2, 2020 12:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UTY 2088: Set external ip from cli #1409

UTY 2088: Set external ip from cli #1409

BryanJY-Wong commented Jun 29, 2020 •

edited

Loading

improbable-prow-robot commented Jun 29, 2020

jamiebrynes7 left a comment •

edited

Loading

BryanJY-Wong Jun 30, 2020

jamiebrynes7 left a comment

zeroZshadow Jun 30, 2020

BryanJY-Wong Jun 30, 2020

zeroZshadow Jun 30, 2020

BryanJY-Wong Jun 30, 2020

jamiebrynes7 Jun 30, 2020

jamiebrynes7 left a comment

zeroZshadow Jul 1, 2020

paulbalaji left a comment

sonarqubecloud bot commented Jul 1, 2020

UTY 2088: Set external ip from cli #1409

UTY 2088: Set external ip from cli #1409

Conversation

BryanJY-Wong commented Jun 29, 2020 • edited Loading

Description

Tests

Documentation

Primary reviewers

improbable-prow-robot commented Jun 29, 2020

jamiebrynes7 left a comment • edited Loading

Choose a reason for hiding this comment

BryanJY-Wong Jun 30, 2020

Choose a reason for hiding this comment

jamiebrynes7 left a comment

Choose a reason for hiding this comment

zeroZshadow Jun 30, 2020

Choose a reason for hiding this comment

BryanJY-Wong Jun 30, 2020

Choose a reason for hiding this comment

zeroZshadow Jun 30, 2020

Choose a reason for hiding this comment

BryanJY-Wong Jun 30, 2020

Choose a reason for hiding this comment

jamiebrynes7 Jun 30, 2020

Choose a reason for hiding this comment

jamiebrynes7 left a comment

Choose a reason for hiding this comment

zeroZshadow Jul 1, 2020

Choose a reason for hiding this comment

paulbalaji left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jul 1, 2020

BryanJY-Wong commented Jun 29, 2020 •

edited

Loading

jamiebrynes7 left a comment •

edited

Loading