CLI refactoring #650

AlexandraRoatis · 2018-09-25T14:50:20Z

Description

Using the picocli library for parsing CLI input parameters.
Support for absolute path use for log and database folders unable to set absolute db path #627.
Correct and consistent behavior for the --network and --datadir options Add --network and --datadir options #524.
Advanced Network enum with support for the mastery test net and custom networks.
Compatibility with old kernels when the above options are not used and the config.xml and genesis.json files are encountered at the old location.
Extensive unit tests for the new functionality.

Type of change

Insert x into the following checkboxes to confirm (eg. [x]):

Testing

Please describe the tests you used to validate this pull request. Provide any relevant details for test configurations as well as any instructions to reproduce these results.

Existing and newly added unit tests.
The following manual commands:

# saving initial config (withut ids)
rsync -rv config/ iniconfig/

###### will exit the kernel

### help tests

./aion.sh -n testnet -d data -other
./aion.sh -other
./aion.sh -h

### version tests

./aion.sh -v
./aion.sh --version

### config tests

# compatible with old kernels
rsync -rv iniconfig/mainnet/ config/ #; grep "id" config/config.xml
./aion.sh -c # writes to config/config.xml
grep "id" config/config.xml

# for new kernels
rm -fr config/config.xml config/genesis.json
rm -fr mainnet

rsync -rv iniconfig/mainnet/ config/mainnet/  #; grep "id" config/mainnet/config.xml
./aion.sh -c # writes to config/mainnet/config.xml
grep "id" config/mainnet/config.xml

rsync -rv iniconfig/mainnet/ config/mainnet/ #; grep "id" config/mainnet/config.xml
./aion.sh -c mainnet # writes to config/mainnet/config.xml
grep "id" config/mainnet/config.xml

rsync -rv iniconfig/mainnet/ config/mainnet/ #; grep "id" config/mainnet/config.xml
./aion.sh -c incorrect # writes to config/mainnet/config.xml
grep "id" config/mainnet/config.xml

rm -fr mastery

rsync -rv iniconfig/mastery/ config/mastery/ #; grep "id" config/mastery/config.xml
./aion.sh -c mastery # writes to config/mastery/config.xml
grep "id" config/mastery/config.xml

rsync -rv iniconfig/mastery/ config/mastery/ #; grep "id" config/mastery/config.xml
./aion.sh -c testnet # writes to config/mastery/config.xml
grep "id" config/mastery/config.xml

mkdir mainnet ; mkdir mainnet/config ; rsync -rv iniconfig/mainnet/ mainnet/config/
./aion.sh -c # writes to mainnet/config/config.xml
grep "id" mainnet/config/config.xml
./aion.sh -c mainnet # writes to mainnet/config/config.xml
grep "id" mainnet/config/config.xml
./aion.sh -c incorrect # writes to mainnet/config/config.xml
grep "id" mainnet/config/config.xml

### info tests

# compatible with old kernels
rsync -rv iniconfig/mainnet/ config/
./aion.sh -i # reads config/config.xml

# for new kernels
rm -fr config/config.xml config/genesis.json
rm -fr mainnet

rsync -rv iniconfig/mainnet/ config/mainnet/
./aion.sh -i # reads config/mainnet/config.xml
mkdir mainnet ; mkdir mainnet/config ; rsync -rv iniconfig/mainnet/ mainnet/config/
./aion.sh -i # reads mainnet/config/config.xml

rm -fr mainnet
./aion.sh -i -n mainnet # reads config/mainnet/config.xml
mkdir mainnet ; mkdir mainnet/config ; rsync -rv iniconfig/mainnet/ mainnet/config/
./aion.sh -i -n mainnet # reads mainnet/config/config.xml

rm -fr data/mastery
./aion.sh -i -n testnet -d data # reads config/mastery/config.xml
mkdir data; mkdir data/mastery ; mkdir data/mastery/config ; rsync -rv iniconfig/mainnet/ data/mastery/config/
./aion.sh -i -n testnet -d data # reads data/mastery/config/config.xml

###### will run the kernel

rsync -rv iniconfig/mainnet/ config/
./aion.sh # reads from config/config.xml

rm -fr mainnet config; rsync -rv  iniconfig/ config/
./aion.sh # reads from config/mainnet/config.xml
# do not delete data for next test

./aion.sh # reads from mainnet/config/config.xml

rm -fr data config; rsync -rv  iniconfig/ config/
./aion.sh -n testnet -d data # reads from config/mastery/config.xml
# do not delete data for next test

./aion.sh -n testnet -d data # reads from data/mastery/config/config.xml

###### using data from last execution
./aion.sh -n testnet -d data --dump-blocks

./aion.sh -n testnet -d data -r 100

./aion.sh -n testnet -d data --state TOP

./aion.sh ac -n testnet -d data # store account in data/mastery/keystore

./aion.sh --dump-state -n testnet -d data

Verification

Insert x into the following checkboxes to confirm (eg. [x]):

I have self-reviewed my own code and conformed to the style guidelines of this project.
New and existing tests pass locally with my changes.
I have added tests for my fix or feature.
I have made appropriate changes to the corresponding documentation.
My code generates no new warnings.
Any dependent changes have been made.

aion-kelvin

Do we need to check that the given arguments don't clash? i.e. from looking at the code, I think doing something like ./aion.sh al ac will get past the check for argument parsing, but after it runs the code for create account, it'll just exit before knowing that list accounts was also requested. I think ideally, it should print out the help page and say that you can't do both. Or you could support both, but that would open a complicated can of worms

modAionImpl/src/org/aion/zero/impl/cli/Arguments.java

aion-kelvin · 2018-09-27T19:21:28Z

modAionImpl/src/org/aion/zero/impl/cli/Arguments.java

+        List<String> list = new ArrayList<>();
+
+        int i = 0;
+        while (i < arguments.length) {


optional: for loop seemed more intuitive to me...

aion-kelvin · 2018-09-27T19:29:53Z

modAionImpl/src/org/aion/zero/impl/cli/Cli.java

-                        }
+            // reading from correct config file
+            File configFile = cfg.getExecConfigFile();
+            if (!configFile.exists()) {


if the given config file doesn't exist, can we either:

exit with an error

use the default but also print out a warning that the given input was not valid so we used the initial config (also, is it possible that the default config doesn't exist either?)

I feel like the user could get confused is he/she inputs something invalid and the program silently ignores it and does its own thing

I will explain the reasoning behind that code in the comments below.
First, keep in mind the following 3 points:

there exists a base directory where all the code and the aion.sh reside;

we allow setting an execution path with the --datadir option and/or the --network option;

we must maintain compatibility with old kernel executables.

So, in order to make these features compatible, I've opted for the following:

If I find a config.xml and genesis.json at the old location, I will attempt to execute the CLI command or run the kernel as previous kernels would, in the base directory.

But, if the user calls the kernel with either the --datadir value, --network value or --config value option, then it will take precedence over old executable style and behave in the following way:

we create an execution path which will be either a directory in the base path with the network name (to ensure that if I run different networks from the same base path, they will not clash with eachother) or the directory given in the datadir option plus the network;

we copy the configuration files to the execution path to save the configuration;

we create the keystore, log and database directories, if they do not already exist.

Because of this setup, there are in fact 3 places from where a kernel can read it's configuration:

config/config.xml like old kernels

config/network/config.xml for new kernels with new execution paths

network/config/config.xml for new kernels where the execution path already exists

So, in the case of new kernels I opted for checking 3. and if it doesn't exist going for 2.
This way I can continue an execution I already started as it was set up, but also start from scratch on new paths.

I agree that this will need to be well documented when we introduce the --datadir and --network options.

aion-kelvin

Hey thanks for working on this. The extensive testing looks great. Have some thoughts below (in addition to my earlier one which I accidentally submitted before I was done)

modAionImpl/src/org/aion/zero/impl/cli/Cli.java

aion-kelvin · 2018-09-27T19:55:31Z

modAionImpl/src/org/aion/zero/impl/cli/Cli.java

     */
    private boolean createAccount() {
-        String password = null, password2 = null;
+        String password, password2;
        try (BufferedReader reader = new BufferedReader(new InputStreamReader(System.in))) {


I know you didn't change this part, but while touching this code, would you mind changing that try ( .. ) to:

try( InputStreamReader isr = new InputStreamReader(System.in); BufferedReader reader = new BufferedReader(isr))

Otherwise try-with-resources doesn't know that InputStreamReader also needs to be auto-closed

aion-kelvin · 2018-09-27T19:56:03Z

modAionImpl/src/org/aion/zero/impl/cli/Cli.java

            return false;
        }

-        String password = null, password2 = null;
+        String password, password2;
        try (BufferedReader reader = new BufferedReader(new InputStreamReader(System.in))) {


I know you didn't change this part, but while touching this code, would you mind changing that try ( .. ) to:

try( InputStreamReader isr = new InputStreamReader(System.in); BufferedReader reader = new BufferedReader(isr))

Otherwise try-with-resources doesn't know that InputStreamReader also needs to be auto-closed

aion-kelvin · 2018-09-27T20:09:32Z

modAionImpl/src/org/aion/zero/impl/config/Network.java

+     * @param identifier a positive integer value representing the network identifier
+     * @return a custom network object with the given identifier.
+     */
+    public static Object getCustomNet(int identifier) {


why Object?

aion-kelvin · 2018-09-27T20:21:19Z

modAionImpl/test/org/aion/zero/impl/cli/CliTest.java

+
+    // base paths
+    private static final String BASE_PATH = System.getProperty("user.dir");
+    private static final File MAIN_BASE_PATH = new File(BASE_PATH, "mainnet");


You could also use JUnit's TemporaryFolder to generate a temporary folder and set your base paths to that folder. Then you won't need to manually delete stuff in your @After

I created an issue for this refactoring #660

aion-kelvin · 2018-09-27T20:24:56Z

modAionImpl/test/org/aion/zero/impl/cli/CliTest.java

+    }
+
+    // Methods below taken from FileUtils class
+    private static boolean copyRecursively(File src, File target) {


We should probably look into using Apache Commons IO (or something similar) for these functions instead of just copy pasting them everywhere...

I created an issue for this refactoring #660

modBoot/src/org/aion/Aion.java

aion-kelvin · 2018-09-27T20:28:32Z

modMcf/src/org/aion/mcf/config/Cfg.java

@@ -136,11 +129,242 @@ public void setConsensus(CfgConsensus _consensus) {
        this.consensus = _consensus;
    }

+    /* ------------ execution path management ------------ */


Supporting the old directory structure as well as the new one is definitely convenient and I'm sure users will appreciate it. Is this something we should continue to support into the future though?

I think at some point (not in this PR) we should make a converter that converts the old structure to the new one so we don't have the burden of dealing with the two code paths. The transition could go like this:

Make the script and release it

Add message in kernel so if it detects old structure, print a message saying they need to migrate it using the script at some point, because we'll stop supporting it

Wait a while, deprecate the code that supports it

qoire

LGTM

…fig to given file

…g and unit tests

…test resources

arajasek

Looks good to me!

AlexandraRoatis · 2018-10-02T21:55:22Z

I confirmed that the full list of commands in the PR description works correctly after the bug fixes in 47b2266.

AlexandraRoatis · 2018-10-02T21:59:01Z

Reviewers are encouraged to check the commands as well and perform additional tests, as they see fit ;)

AlexandraRoatis · 2018-10-03T15:17:03Z

@aion-kelvin regarding the ./aion.sh al ac issue we will document the behavior and bounty out the incompatibility checks

AlexandraRoatis added enhancement New feature or request wip Indicates that a PR or issue is Work In Progress, issuer should be notified before any actions feature labels Sep 25, 2018

AlexandraRoatis added this to the 0.3.2 milestone Sep 25, 2018

AlexandraRoatis self-assigned this Sep 25, 2018

AlexandraRoatis requested review from AionJayT, aion-kelvin, arajasek and qoire September 25, 2018 14:50

AlexandraRoatis force-pushed the cli-parameters branch 12 times, most recently from 32ab904 to de09613 Compare September 27, 2018 14:29

AlexandraRoatis requested a review from ali-sharif September 27, 2018 19:20

aion-kelvin reviewed Sep 27, 2018

View reviewed changes

AlexandraRoatis force-pushed the cli-parameters branch 2 times, most recently from f94ed83 to 3f79bbb Compare September 27, 2018 20:12

aion-kelvin reviewed Sep 27, 2018

View reviewed changes

AlexandraRoatis force-pushed the cli-parameters branch from c4b6892 to cb866d3 Compare September 28, 2018 22:13

qoire approved these changes Oct 1, 2018

View reviewed changes

AionJayT approved these changes Oct 1, 2018

View reviewed changes

AlexandraRoatis force-pushed the cli-parameters branch from 2043504 to b7f3ccc Compare October 1, 2018 20:58

AlexandraRoatis added 18 commits October 2, 2018 11:16

debugging and unit tests for config option; functionality to save con…

c6921ec

…fig to given file

debugging and unit tests for network option

4b3f554

reordered -n and -d options; added missing test resources; refactorin…

79e8ead

…g and unit tests

debugging and unit tests for info option

203a3fe

support for absolute paths for the log and database directories #627

7d90274

correct call to CLI from boot given the new implementation

9ecb1c5

adding mastery config and correct conquest genesis

cb389d0

debugging and enhanced unit tests for config option

5bf4f15

debugging and testing creating and listing accounts

582becc

unit tests for exporting private keys

9d67ac3

unit tests for importing private keys

ed07d01

option/directory for custom networks

95cdea2

made CLI tests less verbose; corrected GenesisTestNetJsonTest to use …

d24ef53

…test resources

debugging and refactoring

c9b166c

added option for showing error in case of vm issue

0d55464

corrected try-with-resource calls

60ecad8

refactored network identifier to chainId

fcdc511

added the picocli library to gradle after rebase on master-pre-merge

e401945

AlexandraRoatis force-pushed the cli-parameters branch from b7f3ccc to e401945 Compare October 2, 2018 15:24

arajasek approved these changes Oct 2, 2018

View reviewed changes

bugfixing broken functionality according to the list of scipts in #650

47b2266

AlexandraRoatis removed the wip Indicates that a PR or issue is Work In Progress, issuer should be notified before any actions label Oct 2, 2018

This was referenced Oct 3, 2018

Refactor and expand CliTest.java #660

Closed

unable to set absolute db path #627

Closed

Add --network and --datadir options #524

Closed

AionJayT merged commit fd5721f into master-pre-merge Oct 3, 2018

AlexandraRoatis deleted the cli-parameters branch October 4, 2018 13:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLI refactoring #650

CLI refactoring #650

AlexandraRoatis commented Sep 25, 2018 •

edited

Loading

aion-kelvin left a comment •

edited

Loading

aion-kelvin Sep 27, 2018

aion-kelvin Sep 27, 2018

AlexandraRoatis Sep 27, 2018 •

edited

Loading

AlexandraRoatis Sep 27, 2018 •

edited

Loading

AlexandraRoatis Sep 27, 2018

aion-kelvin left a comment

aion-kelvin Sep 27, 2018

aion-kelvin Sep 27, 2018

aion-kelvin Sep 27, 2018

AlexandraRoatis Sep 28, 2018

aion-kelvin Sep 27, 2018

AlexandraRoatis Oct 3, 2018

aion-kelvin Sep 27, 2018

AlexandraRoatis Oct 3, 2018

aion-kelvin Sep 27, 2018

qoire left a comment

arajasek left a comment

AlexandraRoatis commented Oct 2, 2018 •

edited

Loading

AlexandraRoatis commented Oct 2, 2018

AlexandraRoatis commented Oct 3, 2018

CLI refactoring #650

CLI refactoring #650

Conversation

AlexandraRoatis commented Sep 25, 2018 • edited Loading

Description

Type of change

Testing

Verification

aion-kelvin left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexandraRoatis Sep 27, 2018 • edited Loading

Choose a reason for hiding this comment

AlexandraRoatis Sep 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aion-kelvin 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qoire left a comment

Choose a reason for hiding this comment

arajasek left a comment

Choose a reason for hiding this comment

AlexandraRoatis commented Oct 2, 2018 • edited Loading

AlexandraRoatis commented Oct 2, 2018

AlexandraRoatis commented Oct 3, 2018

AlexandraRoatis commented Sep 25, 2018 •

edited

Loading

aion-kelvin left a comment •

edited

Loading

AlexandraRoatis Sep 27, 2018 •

edited

Loading

AlexandraRoatis Sep 27, 2018 •

edited

Loading

AlexandraRoatis commented Oct 2, 2018 •

edited

Loading