Fix: Boolean flag consistent usage #1295

kwek20 · 2019-01-22T14:05:27Z

Description

Changed arity of rescan, revalidate, remote, zmq-enabled, testnet-no-coo-validation, to 1.

--help not changed due to expected behavior in the help command of a program

Added the following to JCommander/BasicIotaConfig:
--testnet
--fake-config

These options do not change anything but force correct usage of parameters to keep Boolean behavior uniform.

Fixes #930

Following PR need to be merged directly after:
iotaledger/iri-regression-tests#10
iotaledger/tiab#4

Type of change

Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

Unit tests, regression tests, node runs

Checklist:

My code follows the style guidelines for this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

kwek20 · 2019-01-22T14:18:23Z

ERROR com.iota.iri.IRI$IRILauncher - There was a problem parsing commandline arguments: Expected a value after parameter --testnet
The command "bash run_all_stable_tests.sh $VERSION" exited with 255.

--testnet must now have true afterwards

GalRogozinski · 2019-01-28T09:02:30Z

Why did you add fake-config?

kwek20 · 2019-01-28T14:55:32Z

@GalRogozinski We decided that we want the same behavior for all arguments, which also includes this argument (only used twice in test cases, maybe we should remove it in another PR?)

karimodm · 2019-01-29T17:53:59Z

@kwek20 can you merge into this PR karimodm/iri:docker-arity ? So that Docker won't break.

kwek20 · 2019-01-29T18:11:40Z

@karimodm I did, sort of. Also forgot to push that, sort of.
Its all good now!

karimodm

Actual boolean value passed to the new arity = 1 parameters is not honored.

karimodm · 2019-01-29T17:55:50Z

src/main/java/com/iota/iri/conf/BaseIotaConfig.java

+    }
+
+    @JsonIgnore
+    @Parameter(names = {"--fake-config"}, description = Config.Descriptions.TESTNET, arity = 1)


Is the description Config.Descriptions.TESTNET?

Ah no, good catch

karimodm · 2019-01-29T18:41:53Z

src/main/java/com/iota/iri/IRI.java

+         * @param args the list of program startup arguments
+         * @return <code>true</code> if this is testnet, otherwise <code>false</code>
+         */
+        private static boolean isTestnet(String[] args) {


What about parsing the boolean value passed to options that are not --testnet. Is --remote false treated correctly?

No all other parsing happens in JCommander, but this is needed because we instantiate a different config based on testnet, so we have to check that Before using jcommander.. silly

Removing the testnet/mainnet difference is a different issue/pr i think

so we have to check that Before using jcommander.. silly

A bit, but do you have a better idea?

karimodm · 2019-01-29T18:43:55Z

src/main/java/com/iota/iri/conf/BaseIotaConfig.java

@@ -153,7 +170,7 @@ protected void setApiHost(String apiHost) {
    }

    @JsonIgnore
-    @Parameter(names = {"--remote"}, description = APIConfig.Descriptions.REMOTE)
+    @Parameter(names = {"--remote"}, description = APIConfig.Descriptions.REMOTE, arity = 1)


Is not only about setting the arity. If the user passes a false value the functionality should be disabled.

kwek20 · 2019-02-01T22:09:02Z

@karimodm Can you review again please?

GalRogozinski · 2019-02-03T13:38:04Z

Note that due to a checkstyle exception the code doesn't compile

GalRogozinski · 2019-02-03T13:44:24Z

src/main/java/com/iota/iri/IRI.java

+         * @param args the list of program startup arguments
+         * @return <code>true</code> if this is testnet, otherwise <code>false</code>
+         */
+        private static boolean isTestnet(String[] args) {


so we have to check that Before using jcommander.. silly

A bit, but do you have a better idea?

src/main/java/com/iota/iri/conf/BaseIotaConfig.java

GalRogozinski · 2019-02-03T14:22:37Z

src/main/java/com/iota/iri/conf/BaseIotaConfig.java

    protected void setRemote(boolean remote) {
-        this.apiHost = "0.0.0.0";
+        if (!remote) this.apiHost = "0.0.0.0";
+        this.remote = remote;


I had a comment that the remote field is not used.
If you are assigning a value to it then it is kind of confusing imo. It makes it look that it is not only about setting apiHost to 0.0.0.0, but you have a field.

Could we maybe remove the apiHost attribute and make a change in the class using apiHost to check for remote instead and open the listening socket on 0.0.0.0? #makegalhappy

We can change getApiHost to check the remote field.
This means that if the user configures both then remote will always win, which may be the desired behavior.

@kwek20,
your call if you want to do that

src/main/java/com/iota/iri/conf/Config.java

src/test/java/com/iota/iri/conf/ConfigTest.java

karimodm

Just gave an idea on how to turn Gal's comment around. Looks good to me though ;)

karimodm · 2019-02-05T11:03:23Z

src/main/java/com/iota/iri/conf/BaseIotaConfig.java

    protected void setRemote(boolean remote) {
-        this.apiHost = "0.0.0.0";
+        if (!remote) this.apiHost = "0.0.0.0";
+        this.remote = remote;


Could we maybe remove the apiHost attribute and make a change in the class using apiHost to check for remote instead and open the listening socket on 0.0.0.0? #makegalhappy

kwek20 · 2019-02-18T14:48:57Z

@GalRogozinski @karimodm

Right, for now we have the following scenarios (Defaults are bold):
remote=false, apiHost=localhost: open to localhost
remote=false, apiHost=0.0.0.0: open to everyone
remote=true, apiHost=localhost: open to everyone
remote=true, apiHost=0.0.0.0: open to everyone
remote=false, apiHost=[ip-here]: open to [ip-here]
remote=true, apiHost=[ip-here]: open to everyone -> [ip-here] will be ignored due to the user manually setting remote=true. This behavior is unwanted(Not specific enough). But can be avoided by setting --remote-auth or apiHost to a specific IP, or remote=false. A new PR will be created if this behavior needs to be modified.

Travis fails due to run_all_stable_tests.sh not having the --testnet true change yet. How to add this?

GalRogozinski · 2019-02-27T15:22:04Z

Just a reminder to @kwek20
The reason this is not merged is because buildkite and travis fail.
I can take it on myself to fix travis but @kwek20 you have to fix buildkite.

I would say that the easiest way to test for buildkite currently for you is to simply push new commits

Required for changes in iotaledger/iri#1295

Required for changes in iotaledger/iri#1295 Should not be merged before this PR is merged

Required for changes in iotaledger/iri#1295

Required for changes in iotaledger/iri#1295 Should not be merged before this PR is merged

iotaledger/iri#1295 - Setting --remote to 'true'

kwek20 added 8 commits January 21, 2019 15:58

Added arity

3cac89b

Updated tests

cd2a166

updated regression tests

b1fc5fa

Updated testnet config to arity 1

9ed3811

Updated unit tests for testnet

db31d6f

Added testnet true check

5d96d27

Added a testnet setter in baseiotaconfig

23fecab

testnet true flag forced, updated tests and regression

b7f455a

kwek20 requested review from karimodm and alon-e January 22, 2019 14:06

fixed docker according to karimodm@58ba5c7

352143c

Merged dev

d2bce05

karimodm suggested changes Jan 29, 2019

View reviewed changes

Actually set values for the options, fixed description

95f5ff5

kwek20 requested a review from GalRogozinski February 1, 2019 22:09

GalRogozinski suggested changes Feb 3, 2019

View reviewed changes

kwek20 added 4 commits February 4, 2019 18:51

Merge branch 'dev' into boolean-flags

ed15264

Removed fakeconfig, updated remote

bb9769b

fixed checkstyle

fb09af8

Fixed test

28cbec6

karimodm reviewed Feb 5, 2019

View reviewed changes

GalRogozinski added the C-Configuration Component - Configuration label Feb 14, 2019

Changed remote check to geteApiHost

99855c0

jakubcech added this to the 重庆市 milestone Feb 18, 2019

kwek20 added a commit to kwek20/iri-regression-tests that referenced this pull request Mar 7, 2019

Added boolean flag for testnet

63fbcec

Required for changes in iotaledger/iri#1295

kwek20 mentioned this pull request Mar 7, 2019

Added boolean flag for testnet iotaledger/iri-regression-tests#10

Merged

kwek20 added a commit to iotaledger/tiab that referenced this pull request Mar 7, 2019

Added boolean flag for testnet

2d0b83e

Required for changes in iotaledger/iri#1295 Should not be merged before this PR is merged

kwek20 mentioned this pull request Mar 7, 2019

Added boolean flag for testnet iotaledger/tiab#4

Merged

Added testnet flags to machine one

fe3f6a4

kwek20 mentioned this pull request Mar 11, 2019

Boolean configuration flags have undefined behavior #930

Closed

GalRogozinski pushed a commit to iotaledger/iri-regression-tests that referenced this pull request Mar 12, 2019

Added boolean flag for testnet (#10)

9de8eef

Required for changes in iotaledger/iri#1295

iotaledger deleted a comment Mar 12, 2019

kwek20 added 2 commits March 13, 2019 16:06

Removed download for test files

dfd28e8

Also removed unzip, duh

60dbf15

iotaledger deleted a comment Mar 13, 2019

kwek20 changed the title ~~Boolean flags~~ Fix: Boolean flag consistent usage Mar 15, 2019

GalRogozinski pushed a commit to iotaledger/tiab that referenced this pull request Mar 19, 2019

Added boolean flag for testnet (#4)

3433955

Required for changes in iotaledger/iri#1295 Should not be merged before this PR is merged

GalRogozinski approved these changes Mar 19, 2019

View reviewed changes

kwek20 added 2 commits March 26, 2019 14:40

Added missing flags to machine1 test

779ccd3

Added new line..

d9c0130

iotaledger deleted a comment Mar 26, 2019

kwek20 force-pushed the boolean-flags branch from ea86636 to d9c0130 Compare March 27, 2019 17:07

--testnet-no-coo-validation

84df663

iotaledger deleted a comment Mar 27, 2019

Added .txt to snapshot

d8ed890

GalRogozinski merged commit 1d69f63 into iotaledger:dev Apr 1, 2019

jakubcech mentioned this pull request May 27, 2019

Release 1.7.1 #1463

Closed

ahrain added a commit to ahrain/iota-node that referenced this pull request Jun 21, 2019

iotaledger/iri#1295 - Setting --remote to 'true'

8605621

merlinrabens pushed a commit to merlinrabens/iota-node that referenced this pull request Jun 28, 2019

Merge pull request #26 from ahrain/remote-boolean-fix

9320ac6

iotaledger/iri#1295 - Setting --remote to 'true'

kwek20 mentioned this pull request Jul 4, 2019

Error when trying to run iri node iotaledger/compass#132

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Boolean flag consistent usage #1295

Fix: Boolean flag consistent usage #1295

kwek20 commented Jan 22, 2019 •

edited

Loading

kwek20 commented Jan 22, 2019

GalRogozinski commented Jan 28, 2019

kwek20 commented Jan 28, 2019

karimodm commented Jan 29, 2019 •

edited

Loading

kwek20 commented Jan 29, 2019

karimodm left a comment

karimodm Jan 29, 2019

kwek20 Jan 29, 2019

karimodm Jan 29, 2019

kwek20 Jan 29, 2019

GalRogozinski Feb 3, 2019

karimodm Jan 29, 2019

kwek20 commented Feb 1, 2019

GalRogozinski commented Feb 3, 2019

GalRogozinski Feb 3, 2019

GalRogozinski Feb 3, 2019

karimodm Feb 5, 2019

GalRogozinski Feb 5, 2019

karimodm left a comment

karimodm Feb 5, 2019

kwek20 commented Feb 18, 2019 •

edited

Loading

GalRogozinski commented Feb 27, 2019

Fix: Boolean flag consistent usage #1295

Fix: Boolean flag consistent usage #1295

Conversation

kwek20 commented Jan 22, 2019 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist:

kwek20 commented Jan 22, 2019

GalRogozinski commented Jan 28, 2019

kwek20 commented Jan 28, 2019

karimodm commented Jan 29, 2019 • edited Loading

kwek20 commented Jan 29, 2019

karimodm 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

kwek20 commented Feb 1, 2019

GalRogozinski commented Feb 3, 2019

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

karimodm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwek20 commented Feb 18, 2019 • edited Loading

GalRogozinski commented Feb 27, 2019

kwek20 commented Jan 22, 2019 •

edited

Loading

karimodm commented Jan 29, 2019 •

edited

Loading

kwek20 commented Feb 18, 2019 •

edited

Loading