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.

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

`--remote` flag will enable API interface to all hosts #953

Merged
merged 1 commit into from Aug 27, 2018

Conversation

Projects
None yet
4 participants
@GalRogozinski
Contributor

GalRogozinski commented Aug 23, 2018

Description

The --remote flag that opens the api interface to all hosts was accidentally removed. It is now returned

Fixes #952

Type of change

  • Bug fix (a non-breaking change which fixes an issue)

How Has This Been Tested?

Unit tests were added

  • testRemoteFlag() ascertains that api_host == 0.0.0.0
  • testIniParsingTestnet() now makes sure that REMOTE in INI has no effect (This conforms to backwards compatibility)

Checklist:

Please delete items that are not relevant.

  • 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

@GalRogozinski GalRogozinski requested review from karimodm and alon-e Aug 23, 2018

@@ -31,6 +32,8 @@
protected int maxGetTrytes = Defaults.MAX_GET_TRYTES;
protected int maxBodyLength = Defaults.MAX_BODY_LENGTH;
protected String remoteAuth = Defaults.REMOTE_AUTH;
//We don't have a REMOTE config but we have a remote flag. We must add a field for JCommander
private boolean remote;

This comment has been minimized.

@codacy-bot

codacy-bot Aug 23, 2018

This comment has been minimized.

@GalRogozinski

GalRogozinski Aug 27, 2018

Contributor

Must be used becuase of JCommander

@GalRogozinski

GalRogozinski Aug 27, 2018

Contributor

Must be used becuase of JCommander

@@ -207,6 +216,8 @@ public void testIniParsingTestnet() throws Exception {
.append("NUMBER_OF_KEYS_IN_A_MILESTONE = 3").append(System.lineSeparator())
.append("DONT_VALIDATE_TESTNET_MILESTONE_SIG = true").append(System.lineSeparator())
.append("TIPSELECTION_ALPHA = 1.1").append(System.lineSeparator())
//doesn't do anything
.append("REMOTE")

This comment has been minimized.

@alon-e

alon-e Aug 27, 2018

Contributor

I think this will be confusing - there is no real reason why REMOTE should not work in the config file - I would not call that "not backward compatible".

@alon-e

alon-e Aug 27, 2018

Contributor

I think this will be confusing - there is no real reason why REMOTE should not work in the config file - I would not call that "not backward compatible".

This comment has been minimized.

@GalRogozinski

GalRogozinski Aug 27, 2018

Contributor

The thing is that I don't know if it is correct that you have two ways to manipulate API_HOST...
I can totally imagine someone setting his API host to some IP. Later on he will google how to open his node to the world and add the REMOTE option. He won't notice the API_HOST becuase there are so many configs...

The same problem can also exist with flags, but it is less bad since you have to retype them.

@GalRogozinski

GalRogozinski Aug 27, 2018

Contributor

The thing is that I don't know if it is correct that you have two ways to manipulate API_HOST...
I can totally imagine someone setting his API host to some IP. Later on he will google how to open his node to the world and add the REMOTE option. He won't notice the API_HOST becuase there are so many configs...

The same problem can also exist with flags, but it is less bad since you have to retype them.

This comment has been minimized.

@alon-e

alon-e Aug 27, 2018

Contributor

ok, I agree for now.
Can you open an issue about that then?

@alon-e

alon-e Aug 27, 2018

Contributor

ok, I agree for now.
Can you open an issue about that then?

This comment has been minimized.

@GalRogozinski

GalRogozinski Aug 27, 2018

Contributor

I added a comment on #931 regarding this problem.

@GalRogozinski

GalRogozinski Aug 27, 2018

Contributor

I added a comment on #931 regarding this problem.

@alon-e

alon-e approved these changes Aug 27, 2018

@alon-e alon-e merged commit 8d1fb15 into iotaledger:dev Aug 27, 2018

1 of 2 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment