Skip to content
This repository has been archived by the owner on Jun 11, 2024. It is now read-only.

Fix config usage in worker_controller - Closes #2216 #2217

Merged
merged 2 commits into from
Jul 13, 2018

Conversation

shuse2
Copy link
Collaborator

@shuse2 shuse2 commented Jul 10, 2018

What was the problem?

worker_controller.js was using the outdated way of getting the config.

How did I fix it?

Use the latest way of getting the config as the app.js

How to test it?

Observe if the child process is successfully started

Review checklist

'utf8'
);
var AppConfig = require('./helpers/config.js');
var config = AppConfig(require('./package.json'));
Copy link
Contributor

@nazarhussain nazarhussain Jul 11, 2018

Choose a reason for hiding this comment

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

Pass an extra param for false which will make AppConfig not to parse the command line params here.

https://github.com/LiskHQ/lisk/blob/10aac23335a07ddd5f71b9a96b3c195a51f076b8/helpers/config.js#L37-L39

Copy link
Contributor

Choose a reason for hiding this comment

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

There are only three config values, that worker is dependent on configuration.

config.consoleLogLevel
config.fileLogLevel
config.logFileName

If those can be handled by

https://github.com/LiskHQ/lisk/blob/cb12a1e9d4d9d1bf65a7fc5d9427ee37e95d3084/app.js#L484-L491

that will leave no dependency of worker over configuration file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think change the config to send it to child process rather than reading it directly is not that easy since the childProcessOptions are sent async through IPC after starting the process.
In the current state, we need the config file at the initalization

'utf8'
);
var AppConfig = require('./helpers/config.js');
var config = AppConfig(require('./package.json'));
Copy link
Contributor

Choose a reason for hiding this comment

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

There are only three config values, that worker is dependent on configuration.

config.consoleLogLevel
config.fileLogLevel
config.logFileName

If those can be handled by

https://github.com/LiskHQ/lisk/blob/cb12a1e9d4d9d1bf65a7fc5d9427ee37e95d3084/app.js#L484-L491

that will leave no dependency of worker over configuration file.

@MaciejBaj MaciejBaj merged commit 5e5be7a into 1.1.0 Jul 13, 2018
@MaciejBaj MaciejBaj deleted the 2216-fix_worker_controller_config branch July 13, 2018 09:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants