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.

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

Add option to run non-interactively #17

Merged
merged 4 commits into from
May 8, 2019
Merged

Conversation

schemar
Copy link
Contributor

@schemar schemar commented Apr 26, 2019

In order to automatically restart the faucet it must be possible to run it non-interactively.

Also fixed linter setup.

References #16.

Setup was wrong for linting.
@schemar
Copy link
Contributor Author

schemar commented Apr 26, 2019

References #16

@schemar schemar changed the title Fixed setup Add option to run non-interactively Apr 29, 2019
Copy link
Collaborator

@gulshanvasnani gulshanvasnani left a comment

Choose a reason for hiding this comment

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

Nice work 👍
Below comments needs to be addressed :

  1. non-interactive option won't work if different chain's account have different passwords.
  2. Update readme describing non-interactive option.
  3. Found linting issues.

@schemar
Copy link
Contributor Author

schemar commented Apr 30, 2019

Thanks!

  1. non-interactive option won't if different chain's account have different passwords.

True, what do you propose in order to fix this? I can't think of a proper solution for this. 🧐 Except maybe adding the password to the configuration, but I am not sure that the passwords should be stored in a file 🤔
@deepesh-kn @pgev @benjaminbollen ideas how to craft this well?

  1. Update readme describing non-interactive option.

Will update the README 👍

  1. Found linting issues.

Linting will be handled in a separate issue #18.

@gulshanvasnani
Copy link
Collaborator

Sorry that I missed below point to mention :
Change the DEFAULT_PORT to a number greater or equal to 1024. Any number below 1024 results in error with message as Error: listen EACCES: permission denied 0.0.0.0:80

@gulshanvasnani
Copy link
Collaborator

Thanks!

  1. non-interactive option won't if different chain's account have different passwords.

True, what do you propose in order to fix this? I can't think of a proper solution for this. 🧐 Except maybe adding the password to the configuration, but I am not sure that the passwords should be stored in a file 🤔
@deepesh-kn @pgev @benjaminbollen ideas how to craft this well?

One solution could be to accept array of passwords. The sequence of password specified should be same as the chain sequence.
Example:

 ./faucet -p 8084 3 1406 -n test3 test1406

In the above example, password for chain 3 and 1406 is test3 and test1406 respectively.

@schemar
Copy link
Contributor Author

schemar commented Apr 30, 2019

Thanks!

  1. non-interactive option won't if different chain's account have different passwords.

True, what do you propose in order to fix this? I can't think of a proper solution for this. 🧐 Except maybe adding the password to the configuration, but I am not sure that the passwords should be stored in a file 🤔
@deepesh-kn @pgev @benjaminbollen ideas how to craft this well?

One solution could be to accept array of passwords. The sequence of password specified should be same as the chain sequence.
Example:

 ./faucet -p 8084 3 1406 -n test3 test1406

In the above example, password for chain 3 and 1406 is test3 and test1406 respectively.

You cannot have a variable number of arguments to an option, as the command line interpreter wouldn't know where the option ends and the program arguments start. You would need to do it with something like a comma separated string ./faucet --non-interactive "test3,test1406" 3 1406. That could work, but it has a very strong assumption on the order of execution, which I think the executable should not have any knowledge about.

Sorry that I missed below point to mention :
Change the DEFAULT_PORT to a number greater or equal to 1024. Any number below 1024 results in error with message as Error: listen EACCES: permission denied 0.0.0.0:80

It heavily depends on the environment where you run the faucet. Port 80 is the standard port for HTTP servers: https://en.wikipedia.org/wiki/List_of_TCP_and_UDP_port_numbers#Well-known_ports

@pgev
Copy link
Collaborator

pgev commented Apr 30, 2019

Thanks!

  1. non-interactive option won't if different chain's account have different passwords.

True, what do you propose in order to fix this? I can't think of a proper solution for this. 🧐 Except maybe adding the password to the configuration, but I am not sure that the passwords should be stored in a file 🤔
@deepesh-kn @pgev @benjaminbollen ideas how to craft this well?

Passing multiple passwords on a command line is cumbersome and I would propose to avoid it. I would rather propose to pass a password file, which would specify password per each address@chainid in command line. First line in password file corresponds to the first address@chainid in cli, and so on.

One solution could be to accept array of passwords. The sequence of password specified should be same as the chain sequence.
Example:

 ./faucet -p 8084 3 1406 -n test3 test1406

In the above example, password for chain 3 and 1406 is test3 and test1406 respectively.

You cannot have a variable number of arguments to an option, as the command line interpreter wouldn't know where the option ends and the program arguments start. You would need to do it with something like a comma separated string ./faucet --non-interactive "test3,test1406" 3 1406. That could work, but it has a very strong assumption on the order of execution, which I think the executable should not have any knowledge about.

Sorry that I missed below point to mention :
Change the DEFAULT_PORT to a number greater or equal to 1024. Any number below 1024 results in error with message as Error: listen EACCES: permission denied 0.0.0.0:80

It heavily depends on the environment where you run the faucet. Port 80 is the standard port for HTTP servers: https://en.wikipedia.org/wiki/List_of_TCP_and_UDP_port_numbers#Well-known_ports

@schemar
Copy link
Contributor Author

schemar commented May 2, 2019

Passwords are now read from a file. Also fixed some linting.
@pgev / @gulshanvasnani please have a look.

@schemar schemar mentioned this pull request May 2, 2019
Copy link
Collaborator

@gulshanvasnani gulshanvasnani left a comment

Choose a reason for hiding this comment

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

Good work.
LGTM 👍

@schemar
Copy link
Contributor Author

schemar commented May 6, 2019

@pgev do you want to have a look or can we merge?

@pgev pgev merged commit 7289570 into mosaicdao:master May 8, 2019
@schemar schemar deleted the setup branch May 8, 2019 14:48
@0xsarvesh 0xsarvesh self-assigned this May 9, 2019
@0xsarvesh 0xsarvesh removed their assignment May 10, 2019
@abhayks1 abhayks1 assigned abhayks1 and unassigned abhayks1 May 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants