Create and use config.js script merging defaults and custom configuration - Closes #3171 #3373

nazarhussain · 2019-04-16T14:18:18Z

What was the problem?

The config compilation logic for all modules and component was distributed in different places.

How did I fix it?

Created a configurator object which compiles and reuse that logic.

How to test it?

Run all tests

Review checklist

The PR resolves Create and use config.js script merging defaults and custom configuration #3171
All new code is covered with unit tests
All new code was formatted with Prettier
Linting passes
Tests pass
Commit messages follow the commit guidelines
Documentation has been added/updated

nazarhussain · 2019-04-16T14:19:32Z

You can get following output with the new structure, which is way more detailed compared to previous implementation.

➜  lisk-sdk git:(3171-config-generator) ✗ node lisk/src/index.js usage
Your can customize the configuration runtime with following env variables and command 
line options:

--log,-l        LISK_FILE_LOG_LEVEL       components.logger.fileLogLevel
                LISK_REDIS_HOST           components.logger.logFileName
                LISK_CONSOLE_LOG_LEVEL    components.logger.consoleLogLevel
                LISK_CACHE_ENABLED        components.cache.enabled
--redis,-r      LISK_REDIS_HOST           components.cache.host
                LISK_REDIS_PORT           components.cache.port
                LISK_REDIS_DB_NAME        components.cache.db
                LISK_REDIS_DB_PASSWORD    components.cache.password
                LISK_DB_HOST              components.storage.host
                LISK_DB_PORT              components.storage.port
--database,-d   LISK_DB_NAME              components.storage.database
                LISK_DB_USER              components.storage.user
                LISK_DB_PASSWORD          components.storage.password
                LISK_FORGING_DELEGATES    modules.chain.forging.delegates
--snapshot,-s                             modules.chain.loading.snapshotRound
--port,-p       LISK_WS_PORT              modules.chain.network.wsPort
--address,-a    LISK_ADDRESS              modules.chain.network.address
--peers,-x      LISK_PEERS                modules.chain.network.list
--http-port,-h  LISK_HTTP_PORT            modules.http_api.httpPort
--address,-a    LISK_ADDRESS              modules.http_api.address
                LISK_API_PUBLIC           modules.http_api.access.public
                LISK_API_WHITELIST        modules.http_api.access.whiteList
                LISK_FORGING_WHITELIST    modules.http_api.forging.access.whiteList
--network,-n    LISK_NETWORK              NETWORK
--config,-c     LISK_CONFIG_FILE          CUSTOM_CONFIG_FILE

For rest of configuration, please modify those directly to your custom config file.

…compilation logic

…ttern

nazarhussain · 2019-04-18T09:19:17Z

@fchavant @ManuGowda You can run the command to see all the usage of this file.

node lisk/scripts/generate_config.js usage

And run with proper parameters to get the config as console output. Then pipe it to jq or stream to file with > to do any thing...

fchavant

After creating a build I am not able to run generate_config.js usage:

francois@ams1-lisk-build-003:~/tmp/lisk-1.6.0-rc.4-Linux-x86_64$ node ./scripts/generate_config.js usage
/home/francois/tmp/lisk-1.6.0-rc.4-Linux-x86_64/src/helpers/config.js:32
configurator.registerSchema(appSchema);
             ^

TypeError: Cannot read property 'registerSchema' of undefined
    at Object.<anonymous> (/home/francois/tmp/lisk-1.6.0-rc.4-Linux-x86_64/src/helpers/config.js:32:14)
    at Module._compile (internal/modules/cjs/loader.js:701:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:712:10)
    at Module.load (internal/modules/cjs/loader.js:600:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:539:12)
    at Function.Module._load (internal/modules/cjs/loader.js:531:3)
    at Module.require (internal/modules/cjs/loader.js:637:17)
    at require (internal/modules/cjs/helpers.js:22:18)
    at Object.<anonymous> (/home/francois/tmp/lisk-1.6.0-rc.4-Linux-x86_64/scripts/generate_config.js:22:16)
    at Module._compile (internal/modules/cjs/loader.js:701:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:712:10)
    at Module.load (internal/modules/cjs/loader.js:600:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:539:12)
    at Function.Module._load (internal/modules/cjs/loader.js:531:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:754:12)
    at startup (internal/bootstrap/node.js:283:19)

…to 3171-config-generator

fchavant

package.json and package-lock.json in lisk/ are not in sync.

nazarhussain · 2019-04-23T07:24:22Z

@fchavant That change is not introduced by this PR.

fchavant · 2019-04-23T07:26:40Z

@nazarhussain that may be but I cannot create a build and so cannot test config.js

framework/src/controller/helpers/configurator/index.js

shuse2

In general looks good, some minor comments

shuse2 · 2019-04-23T10:11:54Z

framework/src/controller/helpers/configurator/configurator.js

@@ -0,0 +1,148 @@
+const _ = require('lodash');


can we move this folder to
controller/configurator/configurator.js

I am convinced its a utility not a main namespace. As we planned to rename helpers to util then it will look more appropriate.

utility but it's one of the main functionality for controller, and it has specific logic init, so i think it's worth to put it as first citizen.
Also, i want to get rid of helpers or utils in general =)

To me its not a main functionality of the controller, rather add on to simplify configuration management for end user. These functions still exists and must required, call these helpers or util. In any large project you always have such collection of functions. And util is a known convention in NodeJS community so not a bad thing to have.

And wether we treat these as first citizen or not that depends how much we test coverage we have for them.

framework/src/index.js

framework/src/controller/helpers/configurator/configurator.js

shuse2 · 2019-04-23T10:18:09Z

framework/src/controller/helpers/configurator/configurator.js

+		);
+		Object.keys(this.metaInfo).forEach(key => {
+			message.push(
+				`${(this.metaInfo[key].arg || '').padEnd(15)} ${(


why padEnd this? and next one.

To show an aligned output on the console in help command. See the output in start of the PR.

maybe comment or use of constants?

framework/src/components/cache/defaults/config.js

ManuGowda

LGTM, I am just waiting for the build to get consistently green.

fchavant

Can't test inside of a binary build but looks good.

nazarhussain self-assigned this Apr 16, 2019

nazarhussain added the 🏗️ in progress label Apr 16, 2019

nazarhussain force-pushed the 3171-config-generator branch 2 times, most recently from 07c3418 to 1b27382 Compare April 17, 2019 12:58

nazarhussain added 12 commits April 18, 2019 10:01

Update the order of command line arguments to be compatible with arg lib

b2a6dda

Add a basic functionality of configurator to unify the configuraiton …

d6f1ae0

…compilation logic

Use the framework configurator in the application

c06617b

Update help banner for configuraiton

1c628cc

Add helpers/config.js to generate compiled configuration for application

1d928a8

Fix the framework test app to be compatible with new configuration pa…

f32320b

…ttern

Restructure the schema files for controller

04a9bc5

Update app label schema to allow null values

d430d28

Update the usage of new application interface in lisk

c2138ed

Fix test app to follow new app param structure

2143124

Fix the unit tests for schema snapshots

a9dfd85

Fix unit tests for application

7423f8b

nazarhussain force-pushed the 3171-config-generator branch from a310d40 to 7423f8b Compare April 18, 2019 08:03

Remove obsolete snapshots tests

38ba7e8

nazarhussain marked this pull request as ready for review April 18, 2019 09:15

nazarhussain added 👁️ pending review and removed 🏗️ in progress labels Apr 18, 2019

nazarhussain requested review from shuse2, fchavant and ManuGowda April 18, 2019 09:17

fchavant suggested changes Apr 18, 2019

View reviewed changes

shuse2 and others added 3 commits April 18, 2019 12:45

Merge branch 'development' into 3171-config-generator

393521b

Fix the mocha setup file to resuse app configuration

2052952

Merge branch '3171-config-generator' of github.com:LiskHQ/lisk-sdk in…

e3e22a3

…to 3171-config-generator

nazarhussain added 👁️ pending review and removed 🏗️ in progress labels Apr 23, 2019

fchavant suggested changes Apr 23, 2019

View reviewed changes

ManuGowda reviewed Apr 23, 2019

View reviewed changes

framework/src/controller/helpers/configurator/index.js Outdated Show resolved Hide resolved

Merge branch 'development' into 3171-config-generator

8641401

shuse2 suggested changes Apr 23, 2019

View reviewed changes

fchavant reviewed Apr 23, 2019

View reviewed changes

framework/src/components/cache/defaults/config.js Show resolved Hide resolved

nazarhussain added 2 commits April 23, 2019 14:55

Add unit tests for configurator

d2b04e5

Fix directory structure of jest unit tests

31a5ec2

nazarhussain requested a review from ManuGowda April 23, 2019 13:16

ManuGowda reviewed Apr 23, 2019

View reviewed changes

shuse2 previously approved these changes Apr 23, 2019

View reviewed changes

Fix the linting issues

9658a88

nazarhussain dismissed shuse2’s stale review via 9658a88 April 23, 2019 15:52

nazarhussain and others added 4 commits April 23, 2019 19:50

Fix the configurator override order

a0edb7c

Use test app for the network tests

2933444

Add protocolVersion support in the test app

ea7d934

Merge branch 'development' into 3171-config-generator

7e4e470

nazarhussain requested review from ManuGowda, fchavant and shuse2 April 23, 2019 18:53

shuse2 approved these changes Apr 23, 2019

View reviewed changes

ManuGowda approved these changes Apr 23, 2019

View reviewed changes

shuse2 added ✅ ready and removed 👁️ pending review labels Apr 24, 2019

fchavant approved these changes Apr 24, 2019

View reviewed changes

shuse2 merged commit 7c9d9e9 into development Apr 24, 2019

shuse2 deleted the 3171-config-generator branch April 24, 2019 07:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create and use config.js script merging defaults and custom configuration - Closes #3171 #3373

Create and use config.js script merging defaults and custom configuration - Closes #3171 #3373

nazarhussain commented Apr 16, 2019

nazarhussain commented Apr 16, 2019

nazarhussain commented Apr 18, 2019

fchavant left a comment

fchavant left a comment

nazarhussain commented Apr 23, 2019

fchavant commented Apr 23, 2019

shuse2 left a comment

shuse2 Apr 23, 2019

nazarhussain Apr 23, 2019

shuse2 Apr 23, 2019

nazarhussain Apr 23, 2019

shuse2 Apr 23, 2019

nazarhussain Apr 23, 2019

shuse2 Apr 23, 2019

ManuGowda left a comment

fchavant left a comment

Create and use config.js script merging defaults and custom configuration - Closes #3171 #3373

Create and use config.js script merging defaults and custom configuration - Closes #3171 #3373

Conversation

nazarhussain commented Apr 16, 2019

What was the problem?

How did I fix it?

How to test it?

Review checklist

nazarhussain commented Apr 16, 2019

nazarhussain commented Apr 18, 2019

fchavant left a comment

Choose a reason for hiding this comment

fchavant left a comment

Choose a reason for hiding this comment

nazarhussain commented Apr 23, 2019

fchavant commented Apr 23, 2019

shuse2 left a comment

Choose a reason for hiding this comment

shuse2 Apr 23, 2019

Choose a reason for hiding this comment

nazarhussain Apr 23, 2019

Choose a reason for hiding this comment

shuse2 Apr 23, 2019

Choose a reason for hiding this comment

nazarhussain Apr 23, 2019

Choose a reason for hiding this comment

shuse2 Apr 23, 2019

Choose a reason for hiding this comment

nazarhussain Apr 23, 2019

Choose a reason for hiding this comment

shuse2 Apr 23, 2019

Choose a reason for hiding this comment

ManuGowda left a comment

Choose a reason for hiding this comment

fchavant left a comment

Choose a reason for hiding this comment